Skip to content

Commit

Permalink
Fix Common class ignoring other branches of multiple inheritance tree
Browse files Browse the repository at this point in the history
`Common` was not calling `super().__init__()` method, which means mixin
classes might have been excluded from the chain of `__init__()` calls.
Fixing that, and adding a "root" class to handle calls to
`object.__init__()`.
  • Loading branch information
happz authored and psss committed Feb 20, 2023
1 parent 17a0def commit 9e112d7
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 26 deletions.
34 changes: 18 additions & 16 deletions docs/classes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,24 @@ Basic

The ``Common`` class is the parent of most of the available
classes, it provides common methods for logging, running commands
and workdir handling. The ``Core`` class together with its child
classes ``Test``, ``Plan`` and ``Story`` cover the
:ref:`specification`::

Common
├── Core
│   ├── Plan
│   ├── Story
│   └── Test
├── Clean
├── Guest
├── Phase
├── Run
├── Status
├── Step
└── Tree
and workdir handling. The ``_CommonBase`` class is an actual root
of the class tree, makes sure the inheritance works correctly.
The ``Core`` class together with its child classes ``Test``,
``Plan`` and ``Story`` cover the :ref:`specification`::

_CommonBase
└── Common
├── Core
│   ├── Plan
│   ├── Story
│   └── Test
├── Clean
├── Guest
├── Phase
├── Run
├── Status
├── Step
└── Tree


Phases
Expand Down
48 changes: 47 additions & 1 deletion tests/unit/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import tmt.utils
from tmt.utils import (Command, Common, GeneralError, Path, ShellScript,
StructuredField, StructuredFieldError,
WaitingIncomplete, WaitingTimedOutError,
WaitingIncomplete, WaitingTimedOutError, _CommonBase,
duration_to_seconds, listify, public_git_url,
validate_git_status, wait)

Expand Down Expand Up @@ -860,3 +860,49 @@ def test_import_member_no_such_class():
tmt.utils.GeneralError,
match=r"No such member 'NopeDoesNotExist' in module 'tmt\.steps\.discover'."):
tmt.plugins.import_member('tmt.steps.discover', 'NopeDoesNotExist')


def test_common_base_inheritance(root_logger):
""" Make sure multiple inheritance of ``Common`` works across all branches """

class Mixin(_CommonBase):
def __init__(self, **kwargs):
super().__init__(**kwargs)

assert kwargs['foo'] == 'bar'

# Common first, then the mixin class...
class ClassA(Common, Mixin):
def __init__(self, **kwargs):
super().__init__(**kwargs)

assert kwargs['foo'] == 'bar'

# and also the mixin first, then the common.
class ClassB(Mixin, Common):
def __init__(self, **kwargs):
super().__init__(**kwargs)

assert kwargs['foo'] == 'bar'

# Make sure both "branches" of inheritance tree are listed,
# in the correct order.
assert ClassA.__mro__ == (
ClassA,
Common,
Mixin,
_CommonBase,
object
)

assert ClassB.__mro__ == (
ClassB,
Mixin,
Common,
_CommonBase,
object
)

# And that both classes can be instantiated.
ClassA(logger=root_logger, foo='bar')
ClassB(logger=root_logger, foo='bar')
2 changes: 1 addition & 1 deletion tmt/export/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def __call__(self, collection: List[ExportableT], keys: Optional[List[str]] = No
pass


class Exportable(Generic[ExportableT]):
class Exportable(Generic[ExportableT], tmt.utils._CommonBase):
""" Mixin class adding support for exportability of class instances """

# Declare export plugin registry as a class variable, but do not initialize it. If initialized
Expand Down
62 changes: 54 additions & 8 deletions tmt/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,48 @@ class CommandOutput(NamedTuple):
stderr: Optional[str]


class Common:
class _CommonBase:
"""
A base class for **all** classes contributing to "common" tree of classes.
All classes derived from :py:class:`Common` or mixin classes used to enhance
classes derived from :py:class:`Common` need to have this class as one of
its most distant ancestors. They should not descend directly from ``object``
class, ``_CommonBase`` needs to be used instead.
Our classes and mixins use keyword-only arguments, and with mixins in play,
we do not have a trivial single-inheritance tree, therefore it's not simple
to realize when a ``super().__init__`` belongs to ``object``. To deliver
arguments to all classes, our ``__init__()`` methods must accept all
parameters, even those they have no immediate use for, and propagate them
via ``**kwargs``. Sooner or later, one of the classes would try to call
``object.__init__(**kwargs)``, but this particular ``__init__()`` accepts
no keyword arguments, which would lead to an exception.
``_CommonBase`` sits at the root of the inheritance tree, and is responsible
for calling ``object.__init__()`` *with no arguments*. Thanks to method
resolution order, all "branches" of our tree of common classes should lead
to ``_CommonBase``, making sure the call to ``object`` is correct. To behave
correctly, ``_CommonBase`` needs to check which class is the next in the MRO
sequence, and stop propagating arguments.
"""

def __init__(self, **kwargs: Any) -> None:
mro = type(self).__mro__
# ignore[name-defined]: mypy does not recognize __class__, but it
# exists and it's documented.
# https://peps.python.org/pep-3135/
# https://github.com/python/mypy/issues/4177
parent = mro[mro.index(__class__) + 1] # type: ignore[name-defined]

if parent is object:
super().__init__()

else:
super().__init__(**kwargs)


class Common(_CommonBase):
"""
Common shared stuff
Expand Down Expand Up @@ -459,6 +500,15 @@ def __init__(
if context is provided.
"""

super().__init__(
parent=parent,
name=name,
workdir=workdir,
context=context,
relative_indent=relative_indent,
logger=logger,
**kwargs)

# Use lowercase class name as the default name
self.name = name or self.__class__.__name__.lower()
self.parent = parent
Expand Down Expand Up @@ -3668,7 +3718,7 @@ def wait(
continue


class ValidateFmfMixin:
class ValidateFmfMixin(_CommonBase):
"""
Mixin adding validation of an fmf node.
Expand Down Expand Up @@ -3707,11 +3757,7 @@ def __init__(
if not skip_validation:
self._validate_fmf_node(node, raise_on_validation_error, logger)

# ignore[call-arg]: pypy is not aware of this class being a
# mixin, therefore it cannot allow keyword arguments, because
# object.__init__() allows none. But it's fine, as we will never
# instantiate this class itself.
super().__init__(node=node, logger=logger, **kwargs) # type: ignore[call-arg]
super().__init__(node=node, logger=logger, **kwargs)


# A type representing compatible sources of keys and values.
Expand Down Expand Up @@ -3882,7 +3928,7 @@ def normalize_shell_script_list(
f"Field can be either string or list of strings, '{type(value).__name__}' found.")


class NormalizeKeysMixin:
class NormalizeKeysMixin(_CommonBase):
"""
Mixin adding support for loading fmf keys into object attributes.
Expand Down

0 comments on commit 9e112d7

Please sign in to comment.