From bf5e48841652d8d3aa57b382af84c2059b6ed04a Mon Sep 17 00:00:00 2001 From: Javier Otero <71280927+jjotero@users.noreply.github.com> Date: Mon, 29 Mar 2021 13:05:23 +0200 Subject: [PATCH 1/5] Make required vars required --- reframe/core/pipeline.py | 18 ++++++++++++------ reframe/frontend/loader.py | 5 ++--- .../resources/checks/bad/invalid_check.py | 2 -- unittests/resources/checks/emptycheck.py | 2 -- unittests/resources/checks/frontend_checks.py | 2 -- unittests/resources/checks_unlisted/good.py | 2 -- .../resources/checks_unlisted/kbd_interrupt.py | 2 -- 7 files changed, 14 insertions(+), 19 deletions(-) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index e38365b171..e9c758aac1 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -211,7 +211,7 @@ def pipeline_hooks(cls): #: by this test. #: #: :type: :class:`List[str]` - #: :default: ``None`` + #: :default: ``required`` #: #: .. note:: #: .. versionchanged:: 2.12 @@ -223,7 +223,10 @@ def pipeline_hooks(cls): #: .. versionchanged:: 3.3 #: Default value changed from ``[]`` to ``None``. #: - valid_prog_environs = variable(typ.List[str], type(None), value=None) + #: .. versionchanged:: 3.6 + #: Default value changed from ``None`` to ``required``. + #: + valid_prog_environs = variable(typ.List[str]) #: List of systems supported by this test. #: The general syntax for systems is ``[:]``. @@ -236,7 +239,10 @@ def pipeline_hooks(cls): #: .. versionchanged:: 3.3 #: Default value changed from ``[]`` to ``None``. #: - valid_systems = variable(typ.List[str], type(None), value=None) + #: .. versionchanged:: 3.6 + #: Default value changed from ``None`` to ``required``. + #: + valid_systems = variable(typ.List[str]) #: A detailed description of the test. #: @@ -604,7 +610,7 @@ def pipeline_hooks(cls): #: :: #: #: self.sanity_patterns = sn.assert_true(1) - sanity_patterns = variable(_DeferredExpression, type(None), value=None) + sanity_patterns = variable(_DeferredExpression) #: Patterns for verifying the performance of this test. #: @@ -1520,11 +1526,11 @@ def check_sanity(self): sn.assert_eq(self.job.exitcode, 0, msg='job exited with exit code {0}') ] - if self.sanity_patterns is not None: + if hasattr(self, 'sanity_patterns'): sanity_patterns.append(self.sanity_patterns) self.sanity_patterns = sn.all(sanity_patterns) - elif self.sanity_patterns is None: + elif not hasattr(self, 'sanity_patterns'): raise SanityError('sanity_patterns not set') with osext.change_dir(self._stagedir): diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index 7036d58495..947cfbab9c 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -81,10 +81,9 @@ def _validate_check(self, check): name = type(check).__name__ checkfile = os.path.relpath(inspect.getfile(type(check))) - required_attrs = ['sanity_patterns', - 'valid_systems', 'valid_prog_environs'] + required_attrs = ['valid_systems', 'valid_prog_environs'] for attr in required_attrs: - if getattr(check, attr) is None: + if not hasattr(check, attr): getlogger().warning( f'{checkfile}: {attr!r} not defined for test {name!r}; ' f'skipping...' diff --git a/unittests/resources/checks/bad/invalid_check.py b/unittests/resources/checks/bad/invalid_check.py index e6a9ab159e..ec4a20c2f5 100644 --- a/unittests/resources/checks/bad/invalid_check.py +++ b/unittests/resources/checks/bad/invalid_check.py @@ -4,7 +4,6 @@ # SPDX-License-Identifier: BSD-3-Clause import reframe as rfm -import reframe.utility.sanity as sn @rfm.simple_test @@ -12,7 +11,6 @@ class SomeTest(rfm.RegressionTest): def __init__(self): self.valid_systems = [] self.valid_prog_environs = [] - self.sanity_patterns = sn.assert_true(1) class NotATest: diff --git a/unittests/resources/checks/emptycheck.py b/unittests/resources/checks/emptycheck.py index b3d284c3fe..a57a4b2c44 100644 --- a/unittests/resources/checks/emptycheck.py +++ b/unittests/resources/checks/emptycheck.py @@ -4,7 +4,6 @@ # SPDX-License-Identifier: BSD-3-Clause import reframe as rfm -import reframe.utility.sanity as sn @rfm.simple_test @@ -12,4 +11,3 @@ class EmptyTest(rfm.RegressionTest): def __init__(self): self.valid_systems = [] self.valid_prog_environs = [] - self.sanity_patterns = sn.assert_true(1) diff --git a/unittests/resources/checks/frontend_checks.py b/unittests/resources/checks/frontend_checks.py index ebc746856e..ac2004b16a 100644 --- a/unittests/resources/checks/frontend_checks.py +++ b/unittests/resources/checks/frontend_checks.py @@ -243,7 +243,6 @@ class TestWithGenerator(rfm.RunOnlyRegressionTest): def __init__(self): self.valid_systems = ['*'] self.valid_prog_environs = ['*'] - self.sanity_patterns = sn.assert_true(1) def foo(): yield True @@ -258,7 +257,6 @@ class TestWithFileObject(rfm.RunOnlyRegressionTest): def __init__(self): self.valid_systems = ['*'] self.valid_prog_environs = ['*'] - self.sanity_patterns = sn.assert_true(1) with open(__file__) as fp: pass diff --git a/unittests/resources/checks_unlisted/good.py b/unittests/resources/checks_unlisted/good.py index a53db2a784..905d1af6df 100644 --- a/unittests/resources/checks_unlisted/good.py +++ b/unittests/resources/checks_unlisted/good.py @@ -8,7 +8,6 @@ # import reframe as rfm -import reframe.utility.sanity as sn # We just import this individually for testing purposes from reframe.core.pipeline import RegressionTest @@ -18,7 +17,6 @@ class _Base(RegressionTest): def __init__(self): self.valid_systems = ['*'] self.valid_prog_environs = ['*'] - self.sanity_patterns = sn.assert_true(1) @rfm.parameterized_test(*((x, y) for x in range(3) for y in range(2))) diff --git a/unittests/resources/checks_unlisted/kbd_interrupt.py b/unittests/resources/checks_unlisted/kbd_interrupt.py index 3ed4d3803f..f02bd6b345 100644 --- a/unittests/resources/checks_unlisted/kbd_interrupt.py +++ b/unittests/resources/checks_unlisted/kbd_interrupt.py @@ -11,7 +11,6 @@ # import reframe as rfm -import reframe.utility.sanity as sn @rfm.simple_test @@ -22,7 +21,6 @@ def __init__(self): self.valid_systems = ['*'] self.valid_prog_environs = ['*'] self.tags = {self.name} - self.sanity_patterns = sn.assert_true(1) @rfm.run_before('setup') def raise_keyboard_interrupt(self): From fd4c50fdccad650ec6f6a52887e019981a52fa70 Mon Sep 17 00:00:00 2001 From: Javier Otero <71280927+jjotero@users.noreply.github.com> Date: Mon, 29 Mar 2021 14:09:03 +0200 Subject: [PATCH 2/5] Cleanup default settings --- reframe/core/pipeline.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index e9c758aac1..fbac9bddaf 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -248,7 +248,7 @@ def pipeline_hooks(cls): #: #: :type: :class:`str` #: :default: ``self.name`` - descr = variable(str, type(None), value=None) + descr = variable(str) #: The path to the source file or source directory of the test. #: @@ -343,7 +343,7 @@ def pipeline_hooks(cls): #: #: :type: :class:`str` #: :default: ``os.path.join('.', self.name)`` - executable = variable(str, type(None), value=None) + executable = variable(str) #: List of options to be passed to the :attr:`executable`. #: @@ -835,14 +835,12 @@ def _rfm_init(self, name=None, prefix=None): self.name = name # Pass if descr is a required variable. - with contextlib.suppress(AttributeError): - if self.descr is None: - self.descr = self.name + if not hasattr(self, 'descr'): + self.descr = self.name # Pass if the executable is a required variable. - with contextlib.suppress(AttributeError): - if self.executable is None: - self.executable = os.path.join('.', self.name) + if not hasattr(self, 'executable'): + self.executable = os.path.join('.', self.name) self._perfvalues = {} From 3059c5463f248b57f233522a0593e71557b61ffa Mon Sep 17 00:00:00 2001 From: Javier Otero <71280927+jjotero@users.noreply.github.com> Date: Mon, 29 Mar 2021 15:37:08 +0200 Subject: [PATCH 3/5] Update docstrings --- reframe/core/pipeline.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index fbac9bddaf..7d01f91a97 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -225,7 +225,6 @@ def pipeline_hooks(cls): #: #: .. versionchanged:: 3.6 #: Default value changed from ``None`` to ``required``. - #: valid_prog_environs = variable(typ.List[str]) #: List of systems supported by this test. @@ -241,7 +240,6 @@ def pipeline_hooks(cls): #: #: .. versionchanged:: 3.6 #: Default value changed from ``None`` to ``required``. - #: valid_systems = variable(typ.List[str]) #: A detailed description of the test. @@ -592,17 +590,16 @@ def pipeline_hooks(cls): #: Refer to the :doc:`ReFrame Tutorials ` for concrete usage #: examples. #: - #: If set to :class:`None`, a sanity error will be raised during sanity - #: checking. + #: If not set a sanity error will be raised during sanity checking. #: #: :type: A deferrable expression (i.e., the result of a :doc:`sanity - #: function `) or :class:`None` - #: :default: :class:`None` + #: function `) + #: :default: :class:`required` #: #: .. note:: #: .. versionchanged:: 2.9 #: The default behaviour has changed and it is now considered a - #: sanity failure if this attribute is set to :class:`None`. + #: sanity failure if this attribute is set to :class:`required`. #: #: If a test doesn't care about its output, this must be stated #: explicitly as follows: @@ -610,6 +607,9 @@ def pipeline_hooks(cls): #: :: #: #: self.sanity_patterns = sn.assert_true(1) + #: + #: .. versionchanged:: 3.6 + #: The default value has changed from ``None`` to ``required``. sanity_patterns = variable(_DeferredExpression) #: Patterns for verifying the performance of this test. From 77882731a50cb4ad43cbc1bc12197f745e20a845 Mon Sep 17 00:00:00 2001 From: Javier Otero <71280927+jjotero@users.noreply.github.com> Date: Mon, 29 Mar 2021 15:46:02 +0200 Subject: [PATCH 4/5] Remove contextlib import from the pipeline --- reframe/core/pipeline.py | 1 - 1 file changed, 1 deletion(-) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 7d01f91a97..0113945e04 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -13,7 +13,6 @@ ] -import contextlib import functools import glob import inspect From cfeaefd246a4a7509f9b3994bd2a76b9c48d0344 Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Mon, 29 Mar 2021 16:47:51 +0200 Subject: [PATCH 5/5] Add unit test --- unittests/test_pipeline.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index 4334bb9096..1382772d6b 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -240,6 +240,23 @@ def __init__(self): _run(MyTest(), *local_exec_ctx) +def test_run_only_set_sanity_in_a_hook(local_exec_ctx): + @fixtures.custom_prefix('unittests/resources/checks') + class MyTest(rfm.RunOnlyRegressionTest): + executable = './hello.sh' + executable_opts = ['Hello, World!'] + local = True + valid_prog_environs = ['*'] + valid_systems = ['*'] + + @rfm.run_after('run') + def set_sanity(self): + self.sanity_patterns = sn.assert_found( + r'Hello, World\!', self.stdout) + + _run(MyTest(), *local_exec_ctx) + + def test_run_only_no_srcdir(local_exec_ctx): @fixtures.custom_prefix('foo/bar/') class MyTest(rfm.RunOnlyRegressionTest):