Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose "key address" to normalization callbacks #1869

Merged
merged 5 commits into from
Jun 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions tests/core/fmf-id/test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ rlJournalStart

rlRun -s "tmt plan -vvvv show /plan-with-invalid-ref" 2
rlAssertGrep "warn: /plan-with-invalid-ref:discover .* is not valid under any of the given schemas" $rlRun_LOG
rlAssertGrep "Failed to load step data for DiscoverFmfStepData: The 'ref' field must be a string, got 'int'." $rlRun_LOG
rlAssertGrep "Failed to load step data for DiscoverFmfStepData: Field 'DiscoverFmfStepData:ref' must be a string, 'int' found." $rlRun_LOG

rlRun -s "tmt plan -vvvv show /remote-plan-with-valid-ref"

Expand All @@ -28,7 +28,8 @@ rlJournalStart
rlAssertGrep "some-package" $rlRun_LOG

rlRun -s "tmt test -vvvv show /test-with-invalid-ref" 2
rlAssertGrep "The 'ref' field must be a string, got 'int'." $rlRun_LOG
rlAssertGrep "warn: /test-with-invalid-ref:require .* is not valid under any of the given schemas" $rlRun_LOG
rlAssertGrep "Field 'ref' must be unset or a string, 'int' found." $rlRun_LOG
rlPhaseEnd

rlPhaseStartCleanup
Expand Down
5 changes: 4 additions & 1 deletion tests/unit/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,10 @@ def test_link():
note=fmf_id['note'])]

# Invalid links and relations
with pytest.raises(SpecificationError, match='Invalid link'):
with pytest.raises(
SpecificationError,
match="Field 'link' must be a string, a fmf id or a list of their combinations,"
" 'int' found."):
Links(data=123)
with pytest.raises(SpecificationError, match='Multiple relations'):
Links(data=dict(verifies='one', blocks='another'))
Expand Down
38 changes: 18 additions & 20 deletions tests/unit/test_dataclasses.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,16 @@ def test_sanity():


def test_field_normalize_callback(root_logger: tmt.log.Logger) -> None:
def _normalize_foo(raw_value: Any, logger: tmt.log.Logger) -> int:
def _normalize_foo(key_address: str, raw_value: Any, logger: tmt.log.Logger) -> int:
if raw_value is None:
return None

try:
return int(raw_value)

except ValueError as exc:
raise tmt.utils.SpecificationError(
"Field 'foo' can be either unset or integer,"
f" '{type(raw_value).__name__}' found.") from exc
raise tmt.utils.NormalizationError(key_address, raw_value, 'unset or an integer') \
from exc

@dataclasses.dataclass
class DummyContainer(SerializableContainer):
Expand All @@ -39,35 +38,34 @@ class DummyContainer(SerializableContainer):
data = DummyContainer()
assert data.foo == 1

dataclass_normalize_field(data, 'foo', None, root_logger)
dataclass_normalize_field(data, ':foo', 'foo', None, root_logger)
assert data.foo is None

dataclass_normalize_field(data, 'foo', 2, root_logger)
dataclass_normalize_field(data, ':foo', 'foo', 2, root_logger)
assert data.foo == 2

dataclass_normalize_field(data, 'foo', '3', root_logger)
dataclass_normalize_field(data, ':foo', 'foo', '3', root_logger)
assert data.foo == 3

with pytest.raises(
tmt.utils.SpecificationError,
match=r"Field 'foo' can be either unset or integer, 'str' found."):
dataclass_normalize_field(data, 'foo', 'will crash', root_logger)
match=r"Field ':foo' must be unset or an integer, 'str' found."):
dataclass_normalize_field(data, ':foo', 'foo', 'will crash', root_logger)

assert data.foo == 3


def test_field_normalize_special_method(root_logger: tmt.log.Logger) -> None:
def normalize_foo(cls, raw_value: Any, logger: tmt.log.Logger) -> int:
def normalize_foo(cls, key_address: str, raw_value: Any, logger: tmt.log.Logger) -> int:
if raw_value is None:
return None

try:
return int(raw_value)

except ValueError as exc:
raise tmt.utils.SpecificationError(
"Field 'foo' can be either unset or integer,"
f" '{type(raw_value).__name__}' found.") from exc
raise tmt.utils.NormalizationError(key_address, raw_value, 'unset or an integer') \
from exc

@dataclasses.dataclass
class DummyContainer(SerializableContainer):
Expand All @@ -81,19 +79,19 @@ class DummyContainer(SerializableContainer):
data = DummyContainer()
assert data.foo == 1

dataclass_normalize_field(data, 'foo', None, root_logger)
dataclass_normalize_field(data, ':foo', 'foo', None, root_logger)
assert data.foo is None

dataclass_normalize_field(data, 'foo', 2, root_logger)
dataclass_normalize_field(data, ':foo', 'foo', 2, root_logger)
assert data.foo == 2

dataclass_normalize_field(data, 'foo', '3', root_logger)
dataclass_normalize_field(data, ':foo', 'foo', '3', root_logger)
assert data.foo == 3

with pytest.raises(
tmt.utils.SpecificationError,
match=r"Field 'foo' can be either unset or integer, 'str' found."):
dataclass_normalize_field(data, 'foo', 'will crash', root_logger)
match=r"Field ':foo' must be unset or an integer, 'str' found."):
dataclass_normalize_field(data, ':foo', 'foo', 'will crash', root_logger)

assert data.foo == 3

Expand All @@ -110,9 +108,9 @@ class DummyContainer(SerializableContainer):

foo_metadata.normalize_callback = MagicMock()

dataclass_normalize_field(data, 'foo', 'will crash', root_logger)
dataclass_normalize_field(data, ':foo', 'foo', 'will crash', root_logger)

foo_metadata.normalize_callback.assert_called_once_with('will crash', root_logger)
foo_metadata.normalize_callback.assert_called_once_with(':foo', 'will crash', root_logger)
data._normalize_foo.assert_not_called()


Expand Down
49 changes: 29 additions & 20 deletions tmt/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,8 @@ def from_spec(cls, raw: _RawFmfId) -> 'FmfId':
# TODO: with mandatory validation, this can go away.
ref = raw.get('ref', None)
if not isinstance(ref, (type(None), str)):
raise tmt.utils.SpecificationError(
f"The 'ref' field must be a string, got '{type(ref).__name__}'.")
# TODO: deliver better key address
raise tmt.utils.NormalizationError('ref', ref, 'unset or a string')

fmf_id = FmfId()

Expand Down Expand Up @@ -374,8 +374,8 @@ def from_spec(cls, raw: _RawDependencyFmfId) -> 'DependencyFmfId': # type: igno
# TODO: with mandatory validation, this can go away.
ref = raw.get('ref', None)
if not isinstance(ref, (type(None), str)):
raise tmt.utils.SpecificationError(
f"The 'ref' field must be a string, got '{type(ref).__name__}'.")
# TODO: deliver better key address
raise tmt.utils.NormalizationError('ref', ref, 'unset or a string')

fmf_id = DependencyFmfId()

Expand Down Expand Up @@ -486,6 +486,7 @@ def dependency_factory(raw_dependency: Optional[_RawDependencyItem]) -> Dependen


def normalize_require(
key_address: str,
raw_require: Optional[_RawDependency],
logger: tmt.log.Logger) -> List[Dependency]:
"""
Expand All @@ -503,7 +504,13 @@ def normalize_require(
if isinstance(raw_require, str) or isinstance(raw_require, dict):
return [dependency_factory(raw_require)]

return [dependency_factory(require) for require in raw_require]
if isinstance(raw_require, list):
return [dependency_factory(require) for require in raw_require]

raise tmt.utils.NormalizationError(
key_address,
raw_require,
'a string, a library, a file or a list of their combinations')


def assert_simple_dependencies(
Expand Down Expand Up @@ -537,7 +544,7 @@ def assert_simple_dependencies(
CoreT = TypeVar('CoreT', bound='Core')


def _normalize_link(value: _RawLinks, logger: tmt.log.Logger) -> 'Links':
def _normalize_link(key_address: str, value: _RawLinks, logger: tmt.log.Logger) -> 'Links':
return Links(data=value)


Expand All @@ -560,7 +567,8 @@ class Core(
enabled: bool = True
order: int = field(
default=DEFAULT_ORDER,
normalize=lambda raw_value, logger: DEFAULT_ORDER if raw_value is None else int(raw_value))
normalize=lambda key_address, raw_value, logger:
DEFAULT_ORDER if raw_value is None else int(raw_value))
link: Optional['Links'] = field(
default=None,
normalize=_normalize_link)
Expand All @@ -570,10 +578,11 @@ class Core(
normalize=tmt.utils.normalize_string_list)
tier: Optional[str] = field(
default=None,
normalize=lambda raw_value, logger: None if raw_value is None else str(raw_value))
normalize=lambda key_address, raw_value, logger:
None if raw_value is None else str(raw_value))
adjust: Optional[List[_RawAdjustRule]] = field(
default_factory=list,
normalize=lambda raw_value, logger: [] if raw_value is None
normalize=lambda key_address, raw_value, logger: [] if raw_value is None
else ([raw_value] if not isinstance(raw_value, list) else raw_value))

_KEYS_SHOW_ORDER = [
Expand Down Expand Up @@ -902,10 +911,12 @@ class Test(
# `test` is mandatory, must exist, so how to initialize if it's missing :(
test: Optional[ShellScript] = field(
default=None,
normalize=lambda raw_value, logger: None if raw_value is None else ShellScript(raw_value))
normalize=lambda key_address, raw_value, logger:
None if raw_value is None else ShellScript(raw_value))
path: Optional[Path] = field(
default=None,
normalize=lambda raw_value, logger: None if raw_value is None else Path(raw_value))
normalize=lambda key_address, raw_value, logger:
None if raw_value is None else Path(raw_value))
framework: str = "shell"
manual: bool = False
require: List[Dependency] = field(
Expand Down Expand Up @@ -1496,9 +1507,8 @@ def _get_environment_vars(self, node: fmf.Tree) -> EnvironmentType:
# Environment variables from files
environment_files = node.get("environment-file") or []
if not isinstance(environment_files, list):
raise tmt.utils.SpecificationError(
f"The 'environment-file' should be a list. "
f"Received '{type(environment_files).__name__}'.")
raise tmt.utils.NormalizationError(
f'{self.name}:environment-file', environment_files, 'unset or a list of paths')
combined = tmt.utils.environment_files_to_dict(
filenames=environment_files,
root=Path(node.root) if node.root else None,
Expand Down Expand Up @@ -2045,8 +2055,8 @@ class Story(
title: Optional[str] = None
priority: Optional[StoryPriority] = field(
default=None,
normalize=lambda raw_value,
logger: None if raw_value is None else StoryPriority(raw_value))
normalize=lambda key_address, raw_value, logger:
None if raw_value is None else StoryPriority(raw_value))

_KEYS_SHOW_ORDER = [
'summary',
Expand Down Expand Up @@ -3483,10 +3493,9 @@ def __init__(self, *, data: Optional[_RawLinks] = None):

# TODO: this should not happen with mandatory validation
if data is not None and not isinstance(data, (str, dict, list)):
raise tmt.utils.SpecificationError(
"Invalid link specification "
"(should be a string, fmf id or list of their combinations), "
f"got '{type(data).__name__}'.")
# TODO: deliver better key address, needs to know the parent
raise tmt.utils.NormalizationError(
'link', data, 'a string, a fmf id or a list of their combinations')

# Nothing to do if no data provided
if data is None:
Expand Down
4 changes: 2 additions & 2 deletions tmt/libraries/beakerlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -265,9 +265,9 @@ def fetch(self) -> None:
raise tmt.utils.GeneralError(
f"Library '{self.name}' not found in '{self.repo}'.")
self.require = tmt.base.normalize_require(
library_node.get('require', []), self.parent._logger)
f'{self.name}:require', library_node.get('require', []), self.parent._logger)
self.recommend = tmt.base.normalize_require(
library_node.get('recommend', []), self.parent._logger)
f'{self.name}:recommend', library_node.get('recommend', []), self.parent._logger)

# Create a symlink if the library is deep in the structure
# FIXME: hot fix for https://github.com/beakerlib/beakerlib/pull/72
Expand Down
3 changes: 2 additions & 1 deletion tmt/steps/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,8 @@ def _update_data_from_options(self, keys: Optional[List[str]] = None) -> None:
if value is None or value == [] or value == () or value is False:
continue

tmt.utils.dataclass_normalize_field(self.data, keyname, value, self._logger)
tmt.utils.dataclass_normalize_field(
self.data, f'{self.name}:{keyname}', keyname, value, self._logger)

def wake(self) -> None:
"""
Expand Down
4 changes: 2 additions & 2 deletions tmt/steps/discover/fmf.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,14 @@ class DiscoverFmfStepData(tmt.steps.discover.DiscoverStepData):
# TODO: with mandatory validation, this can go away.
def _normalize_ref(
self,
key_address: str,
value: Optional[Any],
logger: tmt.log.Logger) -> Optional[str]:
if value is None:
return None

if not isinstance(value, str):
raise tmt.utils.SpecificationError(
f"The 'ref' field must be a string, got '{type(value).__name__}'.")
raise tmt.utils.NormalizationError(key_address, value, 'a string')

return value

Expand Down
14 changes: 8 additions & 6 deletions tmt/steps/discover/shell.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class TestDescription(
# soon as possible - nobody want's to keep two very same lists of attributes.
test: ShellScript = field(
default=ShellScript(''),
normalize=lambda raw_value, logger: ShellScript(raw_value),
normalize=lambda key_address, raw_value, logger: ShellScript(raw_value),
serialize=lambda test: str(test),
unserialize=lambda serialized_test: ShellScript(serialized_test)
)
Expand All @@ -51,11 +51,12 @@ class TestDescription(
order: int = field(
# TODO: ugly circular dependency (see tmt.base.DEFAULT_ORDER)
default=50,
normalize=lambda raw_value, logger: 50 if raw_value is None else int(raw_value)
normalize=lambda key_address, raw_value, logger:
50 if raw_value is None else int(raw_value)
)
link: Optional[tmt.base.Links] = field(
default=None,
normalize=lambda raw_value, logger: tmt.base.Links(data=raw_value),
normalize=lambda key_address, raw_value, logger: tmt.base.Links(data=raw_value),
# Using `to_spec()` on purpose: `Links` does not provide serialization
# methods, because specification of links is already good enough. We
# can use existing `to_spec()` method, and undo it with a simple
Expand All @@ -74,11 +75,12 @@ class TestDescription(
)
tier: Optional[str] = field(
default=None,
normalize=lambda raw_value, logger: None if raw_value is None else str(raw_value)
normalize=lambda key_address, raw_value, logger:
None if raw_value is None else str(raw_value)
)
adjust: Optional[List[tmt.base._RawAdjustRule]] = field(
default=None,
normalize=lambda raw_value, logger: [] if raw_value is None else (
normalize=lambda key_address, raw_value, logger: [] if raw_value is None else (
[raw_value] if not isinstance(raw_value, list) else raw_value
)
)
Expand Down Expand Up @@ -149,7 +151,7 @@ def to_spec(self) -> Dict[str, Any]:
class DiscoverShellData(tmt.steps.discover.DiscoverStepData):
tests: List[TestDescription] = field(
default_factory=list,
normalize=lambda raw_value, logger: [
normalize=lambda key_address, raw_value, logger: [
TestDescription.from_spec(raw_datum, logger)
for raw_datum in cast(List[Dict[str, Any]], raw_value)
],
Expand Down
9 changes: 4 additions & 5 deletions tmt/steps/prepare/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -473,11 +473,10 @@ class PrepareInstallData(tmt.steps.prepare.PrepareStepData):
multiple=True,
help='Package name or path to rpm to be installed.',
# PrepareInstall supports *simple* requirements only
normalize=lambda value, logger: tmt.base.assert_simple_dependencies(
tmt.base.normalize_require(value, logger),
normalize=lambda key_address, value, logger: tmt.base.assert_simple_dependencies(
tmt.base.normalize_require(key_address, value, logger),
"'install' plugin support simple packages only, no fmf links are allowed",
logger
),
logger),
serialize=lambda packages: [package.to_spec() for package in packages],
unserialize=lambda serialized: [
tmt.base.DependencySimple.from_spec(package)
Expand Down Expand Up @@ -522,7 +521,7 @@ class PrepareInstallData(tmt.steps.prepare.PrepareStepData):
)


@tmt.steps.provides_method('install')
@ tmt.steps.provides_method('install')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, nope, that does not look like an intention. It shouldn't have any effect, but it's not supposed to happen.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we can fix with some other patch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the fix: #2128

class PrepareInstall(tmt.steps.prepare.PreparePlugin):
"""
Install packages on the guest
Expand Down
6 changes: 3 additions & 3 deletions tmt/steps/provision/testcloud.py
Original file line number Diff line number Diff line change
Expand Up @@ -635,9 +635,9 @@ def go(self) -> None:
if value is not None:
try:
setattr(data, int_key, int(value))
except ValueError:
raise tmt.utils.SpecificationError(
f"Value '{value}' cannot be converted to int for '{int_key}' attribute.")
except ValueError as exc:
raise tmt.utils.NormalizationError(
f'{self.name}:{int_key}', value, 'an integer') from exc

for key, value in data.items():
if key == 'memory':
Expand Down
Loading