Adopt subprocess fixture for testing. #391

MatthewHambley · 2025-02-28T13:21:06Z

Rather than variations of the same hand-mocking of shelling out to a tool this change takes advantage of an existing fixture.

It also pulls in a fixture to create a fake filing system in memory which should be faster and less prone to faults based on what the developer has in their file tree.

# Conflicts: # source/fab/tools/compiler.py # tests/unit_tests/steps/test_link.py # tests/unit_tests/tools/test_compiler.py # tests/unit_tests/tools/test_compiler_wrapper.py # tests/unit_tests/tools/test_linker.py

hiker

I only got through parts of it, will try to continue either later today or tomorrow.

hiker · 2025-03-06T02:32:13Z

source/fab/artefacts.py

@@ -82,7 +82,7 @@ def add(self, collection: Union[str, ArtefactSet],
        self[collection].update(files)

    def update_dict(self, collection: Union[str, ArtefactSet],
-                    key: str, values: Union[str, Iterable]):
+                    key: Optional[str], values: Union[str, Iterable]):
        '''For ArtefactSets that are a dictionary of sets: update


You know that better than me: it appears that only one test supplies a 'None' as key. If None is otherwise not expected, I would suggest to change the test instead of allowing None here.

hiker · 2025-03-06T02:33:08Z

source/fab/dep_tree.py

@@ -98,7 +98,7 @@ def from_dict(cls, d):
        return result


-def extract_sub_tree(source_tree: Dict[Path, AnalysedDependent], root: Path, verbose=False)\
+def extract_sub_tree(source_tree: Mapping[Path, AnalysedDependent], root: Path, verbose=False)\


Just for my own education: what is the advantage of using Mapping? I thought mapping is more generic than Dict, so if we only expect dict, shouldn't we use dict?

hiker · 2025-03-06T02:33:42Z

source/fab/steps/__init__.py

@@ -79,7 +79,8 @@ def run_mp_imap(config, items, func, result_handler):
        result_handler(analysis_results)


-def check_for_errors(results, caller_label=None):
+def check_for_errors(results: Iterable[Union[str, Exception]],


Thanks for adding more type hints!

hiker · 2025-03-06T02:37:39Z

source/fab/tools/compiler.py

@@ -56,6 +56,7 @@ def __init__(self, name: str,
                 compile_flag: Optional[str] = None,
                 output_flag: Optional[str] = None,
                 openmp_flag: Optional[str] = None,
+                 version_argument: Optional[str] = None,


I have been thinking that I should have added this - thanks!

hiker · 2025-03-06T02:40:13Z

source/fab/tools/compiler.py

        self._version_regex = version_regex

    @property
    def mpi(self) -> bool:
-        ''':returns: whether this compiler supports MPI or not.'''
+        '''Returns whether this compiler supports MPI or not.'''


Why did you remove the ::? Using :returns: means that if we ever automatically create a reference guide using sphinx or so, the return value will be properly documented
(see e.g. https://psyclone-ref.readthedocs.io/en/latest/autogenerated/psyclone.core.signature.html#psyclone.core.signature.Signature.is_structure)

hiker · 2025-03-06T02:40:21Z

source/fab/tools/compiler.py

@@ -82,7 +84,7 @@ def openmp(self) -> bool:

    @property
    def openmp_flag(self) -> str:
-        ''':returns: the flag to enable OpenMP.'''
+        '''Returns the flag to enable OpenMP.'''


hiker · 2025-03-06T02:43:10Z

source/fab/tools/compiler_wrapper.py

@@ -163,15 +162,15 @@ def compile_file(self, input_file: Path,
            # which also supports the syntax_only flag anyway)
            self._compiler = cast(FortranCompiler, self._compiler)
            self._compiler.compile_file(input_file, output_file, openmp=openmp,
-                                        add_flags=self.flags + add_flags,
+                                        add_flags=self._flags + add_flags,


I would prefer using the implemented property, not the private attribute. While this has a tiny bit of overhead, the advantage is that this can be overwritten by a derived class. Now, I find overwriting a property bad tbh, and I believe I fix this properly in the compiler profile support PR, where I turns this into a real function get_flags() (iirc)

hiker · 2025-03-06T02:43:33Z

source/fab/tools/compiler_wrapper.py

                                        syntax_only=syntax_only,
                                        )
        else:
            if syntax_only is not None:
                raise RuntimeError(f"Syntax-only cannot be used with compiler "
                                   f"'{self.name}'.")
            self._compiler.compile_file(input_file, output_file, openmp=openmp,
-                                        add_flags=self.flags+add_flags
+                                        add_flags=self._flags+add_flags


Ditto - I would prefer this to stay, so I have less conflicts to resolve later ;)

hiker · 2025-03-06T02:46:32Z

source/fab/tools/tool.py

@@ -182,7 +182,7 @@ def run(self,
            res = subprocess.run(command, capture_output=capture_output,
                                 env=env, cwd=cwd, check=False)
        except FileNotFoundError as err:
-            raise RuntimeError(f"Command '{command}' could not be "
+            raise RuntimeError(f"Command '{' '.join(command)}' could not be "


I think we should stay with the original version. Since our subprocess does not use a shell, that means that a space in a parameter will potentially result in errors (e.g. ["-l /some_lib"] will not be recognised by the linker). While I actually prefer the new look, and it would make it easier for other develoeprs, this error message will then totally hide what the problem is, which will lead to frustrating debugging.
Alternatively, I would be happy to use a shell in subprocess, but you didn't want to do that.

hiker · 2025-03-06T02:47:38Z

source/fab/tools/tool_repository.py

@@ -75,7 +75,7 @@ def __init__(self):
        # Add the common shells. While Fab itself does not need this,
        # it is a very convenient tool for user configuration (e.g. to
        # query nc-config etc)
-        for shell_name in ["sh", "bash", "ksh", "dash"]:
+        for shell_name in ["sh", "bash", "ksh", "dash", 'zsh']:


OK, I am totally confused now. I am just preparing the shell PR (that you have already reviewed) to go into main, and one comment from you was to only support sh. So I have modified this branch before submitting it to only support sh, but you are adding more shells. What do you prefer? I am happy to go either way

hiker

Still more to come, sorry

hiker · 2025-03-06T05:01:37Z

tests/__init__.py

Is that file required?

hiker · 2025-03-06T05:07:16Z

tests/conftest.py

+from fab.tools.tool_box import ToolBox
+
+
+def not_found_callback(process):


Could you please add some comments to all these functions.

hiker · 2025-03-06T05:20:40Z

tests/system_tests/psyclone/test_psyclone_system_test.py

        psyclone_tool = Psyclone()
        psyclone_tool._version = (2, 4, 0)
        psyclone_tool._is_available = True

        mock_transformation_script = mock.Mock(return_value=__file__)
        with mock.patch('fab.tools.psyclone.Psyclone.run') as mock_run_command:
            mock_transformation_script.return_value = Path(__file__)
-            psyclone_tool.process(api=psyclone_lfric_api,
+            psyclone_tool.process(api='dynamo0.3',


While I am ok with removing the psyclone_lfric_api definintion (which allowed them all to be changed at one location), you then need to switch to lfric, not dynamo0.3, which is deprecated.

hiker · 2025-03-06T05:20:53Z

tests/system_tests/psyclone/test_psyclone_system_test.py

@@ -219,7 +227,7 @@ def test_transformation_script(self, psyclone_lfric_api):
            mock_transformation_script.assert_called_once_with(Path(__file__), None)
            # check transformation_script is passed to psyclone command with '-s'
            mock_run_command.assert_called_with(
-                additional_parameters=['-api', psyclone_lfric_api,
+                additional_parameters=['-api', 'dynamo0.3',


hiker · 2025-03-06T05:34:00Z

tests/unit_tests/steps/test_compile_c.py

+    mp_common_args = Mock(config=config)
+    with raises(RuntimeError) as err:
+        _compile_file((Mock(), mp_common_args))
+    assert str(err.value) == "Unexpected tool 'some C compiler' of category " \


pep8 (https://peps.python.org/pep-0008/#maximum-line-length):
"Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation."

hiker · 2025-03-06T05:34:42Z

tests/unit_tests/steps/test_compile_c.py

+        """
+        config, _ = content
+
+        monkeypatch.setenv('CFLAGS', '-Denv_flag')


Oh, I didn't know that - that's neat :)

hiker · 2025-03-06T13:27:35Z

tests/system_tests/zero_config/test_zero_config.py

-        kwargs = {'project_label': 'fortran explicit gfortran', 'fab_workspace': tmp_path, 'multiprocessing': False}
+        ToDo: Fragility due to assumption of source donor.
+        """
+        pytest.importorskip('clang', reason="Missing libclang bindings.")


hiker

OK, I am stopping here and pass it back to you (extra long weekend) - main reason: could you explain to me the advantage of using fs? I have the feeling I am missing something :) So basically, it is a temporary directory, that is just automatically cleared at the end of a test. In my (PSyclone-heavy ;) ) experience, I often use the files in tmp to debug failing tests. So if files are instead created in tmp, I can't debug them to see e.g. what might be wrong. We also have a fixture for changing into tmpdir, which seems to be the equivalent of fs, except in case of failures you can analyse the files. I would also prefer slightly smaller PRs ;) I am aware that this is already a split of an even larger one, but I am sure I missed a fs fixture a couple of times before wondering what that might be ;) I'll pass it back to you at least for clarification of fs, and pick it up next week.

hiker · 2025-03-07T12:25:04Z

tests/unit_tests/steps/test_archive_objects.py

+    Tests the archive step.
+    """
+    def test_for_exes(self, stub_tool_box: ToolBox,
+                      fs, fake_process: FakeProcess) -> None:


Not your fault of course - but that is a HORRIBLE name. Took me forever to find out what on earth fs might even be. Luckily there was one test that typed it :)

hiker · 2025-03-07T12:31:35Z

Oh, also bringing it up to recent develop would be great - the psyclone changes in master are causing the conflict ;)

MatthewHambley added 23 commits February 6, 2025 14:52

Start adopting pytest_subprocess.

8348e09

Compiler wrapper tests updated.

9e2418f

Linker tests updated.

f241299

Preprocessor tests updated.

4c27db5

PSyclone tests updated.

3a52cd9

RSync tests updated.

cf8c5f1

Shell tests updated.

5eb540b

Tool tests updated and convenience functions introduced.

a13bbd2

Tool box tests updated.

87384a2

Tool repository tests updated.

b34d1dd

Versioning tests updated.

8af4c25

Step module tests modified.

acfc389

Type hinting.

34039ae

Styling.

c071259

Include Fake FS in dependency list.

6986a19

Merge remote-tracking branch 'origin/develop' into SubprocessFixture

fa2cb44

# Conflicts: # source/fab/tools/compiler.py # tests/unit_tests/steps/test_link.py # tests/unit_tests/tools/test_compiler.py # tests/unit_tests/tools/test_compiler_wrapper.py # tests/unit_tests/tools/test_linker.py

Fx up merge.

70f3b0d

Regressed overexcited adoption of Python 3.12 feature.

6d1de84

I don't think we care about warnings in this case.

4610c97

Is fake_process/fs ordering important?

2908146

No need for separate "check" and "tedependency collections.

68b718e

No need to test on unsupported versions.

6647b65

Try to ensure fakeness of filesystem.

8e6463b

MatthewHambley marked this pull request as ready for review March 4, 2025 11:58

MatthewHambley requested review from a team and MGEX82 and removed request for a team March 4, 2025 11:58

yaswant requested a review from hiker March 4, 2025 14:04

MatthewHambley removed the request for review from MGEX82 March 4, 2025 14:17

hiker reviewed Mar 6, 2025

View reviewed changes

hiker requested changes Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt subprocess fixture for testing. #391

Adopt subprocess fixture for testing. #391

MatthewHambley commented Feb 28, 2025

hiker left a comment

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker left a comment

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker Mar 6, 2025

hiker left a comment

hiker Mar 7, 2025

hiker commented Mar 7, 2025

		from fab.tools.tool_box import ToolBox


		def not_found_callback(process):

Adopt subprocess fixture for testing. #391

Are you sure you want to change the base?

Adopt subprocess fixture for testing. #391

Conversation

MatthewHambley commented Feb 28, 2025

hiker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiker commented Mar 7, 2025