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

Automatically read test-case config files #100

Merged
merged 5 commits into from
Apr 30, 2021

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Apr 30, 2021

This means test cases don't need to override the configure() method if all they want to do is load the config file <test_case>.cfg in the test-case package (directory).

Land-ice and ocean test case and test groups have been updated to no longer load the test case's config file (since this will have already happened automatically). For now, this doesn't reduce code very much because in all of these cases, the configure() method (or shared configure function at the test group level) did more than just load the test case's config options. But it is likely to make things simpler in the future.

closes #72

xylar added 3 commits April 30, 2021 15:29
This means test cases don't need to override the configure() method
if all they want to do is load the config file <test_case>.cfg in
the test-case package (directory).
They no longer need to load their config files, since this will
happen automatically.
These no longer need to load test cases' config files because
this happens automatically.
@xylar xylar self-assigned this Apr 30, 2021
@xylar xylar added clean-up python package DEPRECATED: PRs and Issues involving the python package (master branch) labels Apr 30, 2021
@xylar xylar requested a review from matthewhoffman April 30, 2021 13:50
@xylar
Copy link
Collaborator Author

xylar commented Apr 30, 2021

Testing

I ran the sia_integration and nightly test suites against a baseline from master on my laptop (Ubuntu, gun). All tests pass (except the usual failure in the RK4 restart test unrelated to this PR).

I verified that all affected config files were identical between the baseline and the work directory produced from this branch.

@xylar
Copy link
Collaborator Author

xylar commented Apr 30, 2021

@matthewhoffman, this was a feature you suggested in our discussion during your review of #28. Would you be willing to review? It would be sufficient for you to just review by inspection, relying on my tests that the change performs as expected. It would be great if you wanted to run tests of your own.

Copy link
Member

@matthewhoffman matthewhoffman left a comment

Choose a reason for hiding this comment

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

@xylar , this looks nice in making the test case .cfg behavior match that of the other objects. I see how in practice it only eliminates a bit of code, but I still like the symmetry of it. I'm approving by inspection.

I have a minor suggestion to somewhere document when/how this bit of 'magic' happens. One possibility is in the description of the testcase class' configure method, so that if someone goes to override it, they'll be reminded of how the .cfg loading occurs:

diff --git a/compass/testcase/__init__.py b/compass/testcase/__init__.py
index 69dcc354..edf3e0b7 100644
--- a/compass/testcase/__init__.py
+++ b/compass/testcase/__init__.py
@@ -113,7 +113,9 @@ class TestCase:
         Modify the configuration options for this test case. Test cases should
         override this method if they want to add config options specific to
         the test case, e.g. from a config file stored in the test case's python
-        package
+        package.  If a test case overrides this method, it should assume that
+        the <test_case.name>.cfg file in its package has already been added
+        to the config parser prior to the `configure` method call.
         """
         pass

xylar added 2 commits April 30, 2021 19:46
Clarify that the config file <self.name>.cfg has already been
loaded so this doesn't need to be doine here.
@xylar
Copy link
Collaborator Author

xylar commented Apr 30, 2021

@matthewhoffman, thanks for the review. I have added this to the docstring and also something similar to the documentation of configure().

@xylar xylar merged commit 0676f88 into MPAS-Dev:master Apr 30, 2021
@xylar xylar deleted the automate_configs_in_test_cases branch April 30, 2021 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up python package DEPRECATED: PRs and Issues involving the python package (master branch)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate config files in test cases
2 participants