Cleanup, adding unit tests for `ccpp_track_variables.py`, activate "chunked data" CI test, add missing MPI target in src/CMakeLists.txt #555

mkavulich · 2024-04-01T17:31:02Z

This PR makes several related changes:

Add unit tests for the ccpp_track_variables.py script, and run them automatically in CI, as requested in Implement CI for ccpp_track_variables.py #511. In addition, some cleanup changes were made to ccpp_track_variables.py to make implementing the CI test easier.
In implementing these new CI tests, I realized that the new "chunked_data" CI tests were not actually run; this PR will activate them.
Finally, there was a single change to ccpp_prebuild.py to make the import_config() routine fail "correctly" when the config file is not present: previous behavior was an exception due to mismatched return values.

User interface changes?: No

Fixes: #511

Testing:
test removed: None
unit tests: Adds test_track_variables.py to CI. Also, activates
system tests: I am not sure what this means 🤷
manual testing: Ran new tests manually with pytest

…'t get debug test working so far

gold2718

I don't want to hold up this PR but I think docstrings for each test function would add helpful documentation of the tests and logical test coverage.

gold2718 · 2024-04-04T20:14:03Z

scripts/ccpp_prebuild.py

@@ -70,7 +70,7 @@ def import_config(configfile, builddir):
    if not os.path.isfile(configfile):
        logging.error("Configuration file {0} not found".format(configfile))
        success = False
-        return
+        return(success, config)


Why the tuple? Doesn't Python do that anyway? Is that a style thing and if so, do we want to be consistent?

@gold2718 Sorry, I'm not sure what you mean. This function as called from main() expects two return values, and so currently when this block of code is executed (when there is no configuration file) it fails with a TypeError because it is not returning the expected values.

> ../scripts/ccpp_track_variables.py -s test_track_variables/suite_small_suite.xml -c test_track_variables/ccpp_prebuild_config -m test_track_variables/ -v air_pressure ERROR:root:Configuration file test_track_variables/ccpp_prebuild_config not found Traceback (most recent call last): File "/glade/derecho/scratch/kavulich/CCPP/track_variables_unit_test/ccpp-framework/test_prebuild/../scripts/ccpp_track_variables.py", line 217, in <module> track_variables(args.sdf,args.metadata_path,args.config,args.variable,args.debug) File "/glade/derecho/scratch/kavulich/CCPP/track_variables_unit_test/ccpp-framework/test_prebuild/../scripts/ccpp_track_variables.py", line 191, in track_variables (success, config) = import_config(config, None) TypeError: cannot unpack non-iterable NoneType object

This change fixes the error.

Sorry for the lack of clarity. All I was saying is that the parentheses are not needed so I was wondering why you use them. The same goes below where you call this function.
Is this a style thing we want to adopt? A quick look shows that the prebuild code seems to consistently use the parentheses syntax while the capgen code is . . . . mixed (I'm sure for good reasons :).

Thanks for the clarification. I did not realize this until you brought this up, but parentheses are not strictly necessary for tuples, so this would be purely stylistic. Since the existing return tuples use parens I'll stick with that for now, especially since PEP8 doesn't seem to specify a preference from what I can tell.

gold2718 · 2024-04-04T20:16:43Z

scripts/ccpp_track_variables.py


-    (success, config) = import_config(args.config, None)
+    (success, config) = import_config(config, None)


You also do not need the tuple to unpack the variables. Same questions as above.

But it doesn't hurt, or? If so, then does it matter?

gold2718 · 2024-04-04T20:19:35Z

test_prebuild/test_track_variables.py

+from ccpp_track_variables import track_variables
+
+def test_successful_match(capsys):
+    expected_output = """For suite test_track_variables/suite_small_suite.xml, the following schemes (in order for each group) use the variable air_pressure:


A doc string explaining the purpose of the test (what is being tested, test limits, etc.) would be helpful here and for all the test functions below.

climbfuji

Approving based on the CI tests all passing and the changes requested not absolutely necessary (but it would be nice to have a bit of documentation for the newly added track_variables test).

climbfuji · 2024-04-16T02:53:02Z

test_prebuild/test_track_variables.py

+from ccpp_track_variables import track_variables
+
+def test_successful_match(capsys):
+    expected_output = """For suite test_track_variables/suite_small_suite.xml, the following schemes (in order for each group) use the variable air_pressure:


climbfuji · 2024-04-16T02:53:32Z

scripts/ccpp_track_variables.py


-    (success, config) = import_config(args.config, None)
+    (success, config) = import_config(config, None)


But it doesn't hurt, or? If so, then does it matter?

mkavulich · 2024-04-16T14:06:27Z

@climbfuji @gold2718 I agree we should have some docstrings here before merge, I'll work on that today.

…al test

mkavulich · 2024-04-17T20:18:47Z

@climbfuji @gold2718 I have added the requested docstrings, as well as updating the existing file-level docstring and adding a call to sys.exit() for the case where the test is not run with pytest. Let me know if you'd like any further changes/updates.

climbfuji

This is great, thanks! One typo left to correct.

climbfuji · 2024-04-17T20:32:47Z

test_prebuild/test_track_variables.py

@@ -43,6 +47,11 @@ def test_successful_match(capsys):
        i+=1

 def test_successful_match_with_subcycles(capsys):
+    """Tests whether test_track_variables.py produces expected output from sample suite and
+       metadata files for a case with a successful match(user provided a variable that exists


Suggested change

metadata files for a case with a successful match(user provided a variable that exists

metadata files for a case with a successful match (user provided a variable that exists

mwaxmonsky

Looks great! Just had one minor change in mind that might be useful but please feel free to disregard if it's not ideal.

test_prebuild/test_track_variables.py

mkavulich · 2024-04-19T01:50:09Z

@climbfuji @mwaxmonsky Thanks for the reviews, I have adopted your suggested changes.

grantfirl · 2024-04-22T21:12:55Z

@mkavulich Can you pull #556 into this so that I can add the latest hash of this branch to NOAA-EMC/fv3atm#816.

mkavulich · 2024-04-22T21:52:59Z

@grantfirl Done

mkavulich added 8 commits March 27, 2024 15:53

Activate chunked data CI test

df36dd2

Fix error of mismatched return values in prebuild

0e0bfb3

Convert main program of ccpp_track_variables to be importable

5ec9f95

Remove duplicate and misleading log messages

67dd0b6

First attempt at pytest unit testing for ccpp_track_variables.py. Can…

68b9003

…'t get debug test working so far

add rest of the tests...debug still failing though...

1a0a354

Giving up on debug test for now

e382e6b

Remove "push" rule in prep for opening PR

830f796

mkavulich requested review from climbfuji, gold2718, dustinswales, mwaxmonsky, peverwhee and grantfirl as code owners April 1, 2024 17:31

gold2718 approved these changes Apr 4, 2024

View reviewed changes

climbfuji approved these changes Apr 16, 2024

View reviewed changes

mkavulich added 2 commits April 17, 2024 13:50

Include proper docstrings for new test file, as well as each individu…

c615827

…al test

Have script exit with error code 1 if invoked without pytest

55972ff

climbfuji requested changes Apr 17, 2024

View reviewed changes

mwaxmonsky reviewed Apr 18, 2024

View reviewed changes

Adopt reviewer suggestions

0c65498

climbfuji approved these changes Apr 19, 2024

View reviewed changes

Add missing MPI target in src/CMakeLists.txt

9b94679

mkavulich mentioned this pull request Apr 22, 2024

Add missing MPI target in src/CMakeLists.txt #556

Closed

mkavulich changed the title ~~Cleanup, adding unit tests for ccpp_track_variables.py, activate "chunked data" CI test~~ Cleanup, adding unit tests for ccpp_track_variables.py, activate "chunked data" CI test, add missing MPI target in src/CMakeLists.txt Apr 22, 2024

BrianCurtis-NOAA mentioned this pull request Apr 30, 2024

fixed excessive evaporation when both innerloop and mraerosol=T ufs-community/ufs-weather-model#2221

Merged

14 tasks

grantfirl mentioned this pull request Apr 30, 2024

Fixed excessive evaporation when both innerloop=T and mraerosol=T NOAA-EMC/fv3atm#816

Merged

grantfirl approved these changes May 1, 2024

View reviewed changes

grantfirl merged commit 741212e into NCAR:main May 1, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup, adding unit tests for `ccpp_track_variables.py`, activate "chunked data" CI test, add missing MPI target in src/CMakeLists.txt #555

Cleanup, adding unit tests for `ccpp_track_variables.py`, activate "chunked data" CI test, add missing MPI target in src/CMakeLists.txt #555

mkavulich commented Apr 1, 2024

gold2718 left a comment

gold2718 Apr 4, 2024

mkavulich Apr 15, 2024

gold2718 Apr 16, 2024

mkavulich Apr 16, 2024

gold2718 Apr 4, 2024

climbfuji Apr 16, 2024

gold2718 Apr 4, 2024

climbfuji Apr 16, 2024

climbfuji left a comment

climbfuji Apr 16, 2024

climbfuji Apr 16, 2024

mkavulich commented Apr 16, 2024

mkavulich commented Apr 17, 2024

climbfuji left a comment

climbfuji Apr 17, 2024

mwaxmonsky left a comment

mkavulich commented Apr 19, 2024

grantfirl commented Apr 22, 2024

mkavulich commented Apr 22, 2024


		(success, config) = import_config(args.config, None)
		(success, config) = import_config(config, None)

	metadata files for a case with a successful match(user provided a variable that exists
	metadata files for a case with a successful match (user provided a variable that exists

Cleanup, adding unit tests for ccpp_track_variables.py, activate "chunked data" CI test, add missing MPI target in src/CMakeLists.txt #555

Cleanup, adding unit tests for ccpp_track_variables.py, activate "chunked data" CI test, add missing MPI target in src/CMakeLists.txt #555

Conversation

mkavulich commented Apr 1, 2024

gold2718 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

climbfuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkavulich commented Apr 16, 2024

mkavulich commented Apr 17, 2024

climbfuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaxmonsky left a comment

Choose a reason for hiding this comment

mkavulich commented Apr 19, 2024

grantfirl commented Apr 22, 2024

mkavulich commented Apr 22, 2024

Cleanup, adding unit tests for `ccpp_track_variables.py`, activate "chunked data" CI test, add missing MPI target in src/CMakeLists.txt #555

Cleanup, adding unit tests for `ccpp_track_variables.py`, activate "chunked data" CI test, add missing MPI target in src/CMakeLists.txt #555