Improve unit testing by SeanBryan51 · Pull Request #99 · CABLE-LSM/benchcab

SeanBryan51 · 2023-06-27T06:01:10Z

Currently, benchcab does not have extensive coverage in its unit tests. Parts of the code that have poor test coverage are dependent on system interaction (e.g. environment modules, running subprocess commands (SVN, build scripts, executables, PBS).

This change refactors and adds unit testing for these parts of the code base. The pytest code coverage is now at 89%.

Move the code from the benchcab.run_cable_site module to the benchcab.task module. This is done so that we reduce coupling between modules that use fluxnet tasks. This also has the benefit that we can test the two modules easily by sharing the same test setup functions. We move the code from the benchcab.run_comparison module to benchcab.task module for the same reasons.

Unit tests now check exception messages and standard output produced by benchcab for both non-verbose and verbose modes. Beware: standard output messages have been updated in some areas. These changes will need to be updated in the documentation. Changes have now been updated.

Refactor the default build so that environment modules are loaded via python instead of writing module load statements into the temporary build script. This is done to reduce the complexity of the unit tests.

The stderr output from subprocess commands are now redirected to stdout so that stderr is suppressed in non-verbose mode.

Fixes #58 #22 #86 #102

Currently, benchcab does not have extensive coverage in its unit tests. Parts of the code that have poor test coverage are dependent on system interaction (e.g. environment modules, running subprocess commands (SVN, build scripts, executables, PBS). This change refactors and adds unit testing for these parts of the code base. Move the code from the benchcab.run_cable_site module to the benchcab.task module. This is done so that we reduce coupling between modules that use fluxnet tasks. This also has the benefit that we can test the two modules easily by sharing the same test setup functions. We move the code from the benchcab.run_comparison module to benchcab.task module for the same reasons. Unit tests now check exception messages and standard output produced by benchcab for both non-verbose and verbose modes. **Beware:** standard output messages have been updated in some areas. These changes will need to be updated in the documentation. Refactor the default build so that environment modules are loaded via python instead of writing module load statements into the temporary build script. This is done to reduce the complexity of the unit tests. The stderr output from subprocess commands are now redirected to stdout so that stderr is suppressed in non-verbose mode. Fixes #58 #22 #86

This change refactors the code to be more object oriented so that we can better support mocking via dependency injection rather than resorting to the `unittest.mock.patch` function. This allows us to write unit tests that are simpler and that preserve the API layer (as opposed to using `unittest.mock.patch` which breaks the API layer). Fixes #102

codecov · 2023-07-13T02:09:41Z

Codecov Report

Merging #99 (8acd685) into master (0eb94c3) will increase coverage by 21.97%.
The diff coverage is 89.43%.

@@             Coverage Diff             @@
##           master      #99       +/-   ##
===========================================
+ Coverage   66.29%   88.26%   +21.97%     
===========================================
  Files          20       26        +6     
  Lines        1062     1364      +302     
===========================================
+ Hits          704     1204      +500     
+ Misses        358      160      -198

Impacted Files	Coverage Δ
benchcab/benchtree.py	`100.00% <ø> (+1.96%)`	⬆️
benchcab/benchcab.py	`35.06% <43.37%> (+8.06%)`	⬆️
benchcab/comparison.py	`61.36% <61.36%> (ø)`
benchcab/task.py	`83.00% <78.18%> (-6.01%)`	⬇️
benchcab/environment_modules.py	`84.61% <81.81%> (+9.61%)`	⬆️
tests/common.py	`88.57% <87.87%> (-11.43%)`	⬇️
benchcab/repository.py	`98.76% <98.76%> (ø)`
benchcab/bench_config.py	`100.00% <100.00%> (ø)`
benchcab/internal.py	`90.47% <100.00%> (+31.90%)`	⬆️
benchcab/utils/logging.py	`100.00% <100.00%> (ø)`
... and 12 more

ccarouge

There are a few things but everything should be straight forward. Feel free to refuse on anything I've put in.

ccarouge · 2023-07-13T03:41:46Z

+    """Returns a PBS job that executes all computationally expensive commands.
+
+    This includes things such as running CABLE and running bitwise comparison jobs
+    between model output files. The PBS job script is written to the current
+    working directory as a side effect.
+    """


Suggested change

"""Returns a PBS job that executes all computationally expensive commands.

This includes things such as running CABLE and running bitwise comparison jobs

between model output files. The PBS job script is written to the current

working directory as a side effect.

"""

"""Returns the text for a PBS job script that executes all computationally expensive commands.

This includes things such as running CABLE and running bitwise comparison jobs

between model output files.

"""

Fixed in 95f1871

ccarouge · 2023-07-13T04:40:39Z

+    stdout_file = bitwise_cmp_dir / "mock_comparison_task_name.txt"
+
+    # Failure case: test failed comparison check (files differ)
+    mock_subprocess = MockSubprocessWrapper()
+    mock_subprocess.error_on_call = True
+    task = get_mock_comparison_task(subprocess_handler=mock_subprocess)


Suggested change

stdout_file = bitwise_cmp_dir / "mock_comparison_task_name.txt"

# Failure case: test failed comparison check (files differ)

mock_subprocess = MockSubprocessWrapper()

mock_subprocess.error_on_call = True

task = get_mock_comparison_task(subprocess_handler=mock_subprocess)

# Failure case: test failed comparison check (files differ)

mock_subprocess = MockSubprocessWrapper()

mock_subprocess.error_on_call = True

task = get_mock_comparison_task(subprocess_handler=mock_subprocess)

stdout_file = bitwise_cmp_dir / f"{task.task_name}.txt"

Simply moves the def. of stdout_file after creating the task and re-use task_name. Makes it easier to understand where the name of the stdout_file comes from and avoids breaking the test by changing get_mock_comparison_task().

Fixed in 95f1871

ccarouge · 2023-07-13T04:42:17Z

+    task = get_mock_comparison_task(subprocess_handler=mock_subprocess)
+    task.run()
+    with open(stdout_file, "r", encoding="utf-8") as file:
+        assert file.read() == "mock standard output"


Suggested change

assert file.read() == "mock standard output"

assert file.read() == mock_subprocess.stdout

Fixed in 95f1871

ccarouge · 2023-07-13T05:12:01Z

+                verbose=verbose,
+            )
+
+    def custom_build(self, verbose=False) -> None:


I have moved the issue to work on merging this with the default build to the top of the list. This way we don't have to worry about this method here.

ccarouge · 2023-07-13T05:16:47Z

+    mock_subprocess = MockSubprocessWrapper()
+    mock_subprocess.stdout = "mock standard output"
+    repo = get_mock_repo(mock_subprocess)
+    assert repo.svn_info_show_item("some-mock-item") == "mock standard output"


Suggested change

assert repo.svn_info_show_item("some-mock-item") == "mock standard output"

assert repo.svn_info_show_item("some-mock-item") == mock_subprocess.stdout

Fixed in 95f1871

ccarouge · 2023-07-13T06:15:18Z

+                project=self.config["project"], modules=self.config["modules"]
+            )
+
+    def _validate_environment(self, project: str, modules: list):


We could write tests for this. pytest allows to mark tests and choose to run only some. So we could check all the failures on GitHub (which are the most interesting tests actually) and add a "Gadi-only" test for success that we disable from the automatic testing on GitHub.

I am happy to leave it for another pull request.

Yep that is possible. Although I would prefer if we didn't have Gadi specific tests (this wouldn't count towards code coverage). I actually think it would be straight forward to refactor _validate_environment so that it is testable using dependency injection. But we can probably save this for later 😄

ccarouge · 2023-07-13T06:21:10Z

+        if not self.modules_handler.module_is_loaded("nccmp"):
+            self.modules_handler.module_load(
+                "nccmp"
+            )  # use `nccmp -df` for bitwise comparisons


Suggested change

if not self.modules_handler.module_is_loaded("nccmp"):

self.modules_handler.module_load(

"nccmp"

) # use `nccmp -df` for bitwise comparisons

if not self.modules_handler.module_is_loaded("nccmp/1.8.5.0"):

self.modules_handler.module_load(

"nccmp/1.8.5.0"

) # use `nccmp -df` for bitwise comparisons

Always specify the version of the module when loading a module. Other versions can be installed and default versions can change with time.

Fixed in 95f1871

ccarouge · 2023-07-13T06:23:14Z

+    assert buf.getvalue() == (
+        "Creating PBS job script to run FLUXNET tasks on compute "
+        f"nodes: {internal.QSUB_FNAME}\n"
+        "Error when submitting job to NCI queue\nmock standard output\n"


Suggested change

"Error when submitting job to NCI queue\nmock standard output\n"

f"Error when submitting job to NCI queue\n{mock_subprocess.stdout}\n"

Fixed in 95f1871

ccarouge · 2023-07-13T06:37:18Z

+    patch_namelist(nml_path, {"cable": {"file": "/path/to/file", "bar": 123}})
+    assert f90nml.read(nml_path) == {
+        "cable": {
+            "file": "/path/to/file",
+            "bar": 123,
+        }
+    }



Can you put the patch dictionary in a variable and reuse that variable in the assert? Instead of rewriting the whole dictionary.

Fixed in 95f1871

ccarouge · 2023-07-13T06:40:40Z

+        assert atts["cable_branch"] == "mock standard output"
+        assert atts["svn_revision_number"] == "mock standard output"


Suggested change

assert atts["cable_branch"] == "mock standard output"

assert atts["svn_revision_number"] == "mock standard output"

assert atts["cable_branch"] == mock_subprocess.stdout

assert atts["svn_revision_number"] == mock_subprocess.stdout

Fixed in 95f1871

Co-authored-by: Claire Carouge <claire.carouge@anu.edu.au>

ccarouge

Finally, done. That took more time than expected but I think it was worth it, for benchcab and for us.

Currently, the CABLE executable is run from the current working directory instead of the task directory by using the absolute path to the executable. However, we should be running the executable from the task directory (otherwise CABLE will complain it cannot find the soil and PFT namelist files). This was a bug that was introduced in the #99. This change makes it so we change into the task directory before running the CABLE executable. Fixes #123

SeanBryan51 linked an issue Jun 27, 2023 that may be closed by this pull request

Improve unit test coverage #86

Closed

SeanBryan51 force-pushed the 86-improve-unit-test-coverage branch from f8a064d to 8f80a70 Compare June 27, 2023 06:56

SeanBryan51 changed the title ~~86 improve unit test coverage~~ Improve unit testing Jun 27, 2023

SeanBryan51 force-pushed the 86-improve-unit-test-coverage branch from 8f80a70 to e2813ac Compare June 27, 2023 07:26

SeanBryan51 marked this pull request as ready for review June 27, 2023 07:34

SeanBryan51 linked an issue Jun 28, 2023 that may be closed by this pull request

Science configurations appear twice in global attributes of netcdf output files #22

Closed

SeanBryan51 requested a review from ccarouge June 28, 2023 01:12

ccarouge reviewed Jun 29, 2023

View reviewed changes

ccarouge reviewed Jun 30, 2023

View reviewed changes

SeanBryan51 force-pushed the 86-improve-unit-test-coverage branch 2 times, most recently from 88f2575 to 0e016ee Compare July 4, 2023 03:33

SeanBryan51 requested a review from ccarouge July 4, 2023 05:53

Add requested changes from code review

865da7a

SeanBryan51 force-pushed the 86-improve-unit-test-coverage branch from 0e016ee to 865da7a Compare July 4, 2023 11:13

SeanBryan51 force-pushed the 86-improve-unit-test-coverage branch from 73e04f0 to 5eb457c Compare July 12, 2023 06:48

SeanBryan51 force-pushed the 86-improve-unit-test-coverage branch from 5eb457c to ecc7940 Compare July 12, 2023 07:00

Merge branch 'master' into 86-improve-unit-test-coverage

06c2c61

ccarouge requested changes Jul 13, 2023

View reviewed changes

SeanBryan51 and others added 2 commits July 14, 2023 14:05

Add requested changes from code review

95f1871

Co-authored-by: Claire Carouge <claire.carouge@anu.edu.au>

Update default values for config parameters

8acd685

SeanBryan51 linked an issue Jul 14, 2023 that may be closed by this pull request

Decouple bitwise comparison tasks from fluxsite tasks #102

Closed

SeanBryan51 requested a review from ccarouge July 14, 2023 05:13

ccarouge approved these changes Jul 14, 2023

View reviewed changes

SeanBryan51 merged commit 6983bdc into master Jul 14, 2023

SeanBryan51 deleted the 86-improve-unit-test-coverage branch July 14, 2023 07:00

SeanBryan51 mentioned this pull request Aug 3, 2023

CABLE exe should be run from the task directory #123

Closed

SeanBryan51 mentioned this pull request Aug 4, 2023

Run executable from task dir #124

Merged

	assert file.read() == "mock standard output"
	assert file.read() == mock_subprocess.stdout

	assert repo.svn_info_show_item("some-mock-item") == "mock standard output"
	assert repo.svn_info_show_item("some-mock-item") == mock_subprocess.stdout

	"Error when submitting job to NCI queue\nmock standard output\n"
	f"Error when submitting job to NCI queue\n{mock_subprocess.stdout}\n"

		assert atts["cable_branch"] == "mock standard output"
		assert atts["svn_revision_number"] == "mock standard output"

Conversation

SeanBryan51 commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ccarouge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccarouge left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SeanBryan51 commented Jun 27, 2023 •

edited

Loading

codecov Bot commented Jul 13, 2023 •

edited

Loading