-
Notifications
You must be signed in to change notification settings - Fork 4
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
Rework test suite base class #181
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, a few copy and paste issues but it's a lot cleaner.
Ensure that the verified test variables are not changed by tests and return a copy of the frozen set.
Sweeping rework of the SMRFTestCase class, breaking the setUpClass into chunks to enable overwriting of inheriting classes. Also adds a new method to copy the SMRF wide base config and adapt for specific cases.
Change tests to use the configured output file from the .ini instead of manually mapping that again.
There were two configs for RME test data and only the one under the main test folder was used. Replace the existing one with the base. Brings it in line with Lakes setup.
Start a 'basin' folder under the tests that holds the test data for different basins. This also adds the `basin_dir` property to the SMRFTestCase class.
PR feedback.
Make the gold dir a property on the test case. This voids the need to pass in the gold and comparison file, where only the path are different.
os.path.join(self.gold, 'air_temp.nc'), | ||
os.path.join(self.output, 'air_temp.nc') | ||
) | ||
self.compare_netcdf_files('air_temp.nc') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean!
Declare threading config on the base class for use in inheriting test setups to avoid threading debug adventures.
Everything was commented out.
Did some updates after resolving all the merge conflicts with latest master. Mostly organization. |
'threading': True, | ||
'max_queue': 1, | ||
'time_out': 5 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Central config for test with threading
smrf/tests/test_threading.py
Outdated
s = SMRF(self.base_config) | ||
@staticmethod | ||
def get_variables(config): | ||
s = SMRF(config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think there was a bug here, where the passed in config was not used and the base without threading instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was attempting to run it not threaded first then threaded to see if any of the variables changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep the test or nuke this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, I'm on the fence. It's useful in the sense it makes us aware of what is in the queue. But it's a lot of extra code for that check.
Or can we move the assert_thread_variables
here for a quick test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's sound like a perfect place for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to have the threading config centralized. Just removed a thread variable from the queue and the tests failed on the length count!
""" | ||
Test for all defined BASE_THREAD_VARIABLES, not accounting for config | ||
dependent additions. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I came up with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
""" | ||
Test for all defined BASE_THREAD_VARIABLES, not accounting for config | ||
dependent additions. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Sneaked quite some test structure changes into here and this should set a good foundation for the suite going forward.
Significant change is two fold:
SMRFTestCase
base class to use more methods and properties to enable easier overwrite for different config setups. This also added a method to copy the base config to ensure the initial copy will not be changed.basins
folder. This separates test data from actual test logic.