-
Notifications
You must be signed in to change notification settings - Fork 156
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
add unit test to confirm step and pipeline __all__ #8140
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8140 +/- ##
=======================================
Coverage 75.24% 75.24%
=======================================
Files 470 470
Lines 38421 38422 +1
=======================================
+ Hits 28909 28910 +1
Misses 9512 9512
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
341f69b
to
a713971
Compare
check that these contain all subclasses of JwstStep (for step) and JwstPipeline (for pipeline)
a713971
to
5db2493
Compare
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 OK to me (?) Given that all of the CI tests on this PR passed, I guess that means these 2 new tests work correctly. Or do these unit tests not get run by the CI's?
Thanks for giving this a review! The 2 new tests run in the github CI. As #8137 fixed the issue the new tests pass. To see if this would have caught the issue I made a test PR (#8142) that reverted #8137 and the CI for that test PR showed a failure in the new |
def _get_subclass_names(class_, ignore=None, ignore_tests=True): | ||
""" | ||
Iterate through names of all subclasses (recursively) of a class ignoring | ||
all subclasses with names in ignore (and ignoring any subclasses of | ||
ignored classes). Also optionally ignore all classes defined in tests modules. | ||
""" | ||
if ignore is None: | ||
ignore = set() | ||
for subclass in class_.__subclasses__(): | ||
if subclass.__name__ in ignore: | ||
continue | ||
if ignore_tests and (subclass.__module__ == 'local' or 'tests' in subclass.__module__.split('.')): | ||
continue | ||
yield subclass.__name__ | ||
yield from _get_subclass_names(subclass, ignore, ignore_tests) |
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.
There's something very similar to this function in
Lines 261 to 307 in b762c66
def find_suffixes(): | |
"""Find all possible suffixes from the jwst package | |
Returns | |
------- | |
suffixes: set | |
The set of all programmatically findable suffixes. | |
Notes | |
----- | |
This will load all of the `jwst` package. Consider if this | |
is worth doing dynamically or only as a utility to update | |
a static list. | |
""" | |
from jwst.stpipe import Step | |
from jwst.stpipe.utilities import all_steps | |
jwst = import_module('jwst') | |
jwst_fpath = path.split(jwst.__file__)[0] | |
# First traverse the code base and find all | |
# `Step` classes. The default suffix is the | |
# class name. | |
suffixes = set( | |
klass_name.lower() | |
for klass_name, klass in all_steps().items() | |
) | |
# Instantiate Steps/Pipelines from their configuration files. | |
# Different names and suffixes can be defined in this way. | |
# Note: Based on the `collect_pipeline_cfgs` script | |
config_path = path.join(jwst_fpath, 'pipeline') | |
for config_file in listdir(config_path): | |
if config_file.endswith('.cfg'): | |
try: | |
step = Step.from_config_file( | |
path.join(config_path, config_file) | |
) | |
except Exception as err: | |
logger.debug(f'Configuration {config_file} failed: {str(err)}') | |
else: | |
suffixes.add(step.name.lower()) | |
if step.suffix is not None: | |
suffixes.add(step.suffix.lower()) | |
# That's all folks | |
return list(suffixes) |
where independently the idea is to find all suffixes of all steps, but of course it doesn't take into account anything in jwst.stpipe.integration
, which it should. It also is only run during a test to verify the static list is up-to-date. Better to have it in test infrastructure.
We may want refactor the suffix handling so it doesn't depend on .cfg files. And I have a good reason to refactor suffix handling anyway so that it is more dynamic - handling suffixes of other non-jwst
package steps that are registered through stpipe entry points, but doing so without having to import all the JwstStep and JwstPipeline subclasses to do so. I will work on this. @hbushouse
As discussed in #8137 (see #8137 (comment))
This PR adds 2 unit tests to:
JwstStep
are listed injwst.step.__all__
(except forJwstPipeline
which is a subclass ofJwstStep
)JwstPipeline
are listed injwst.pipeline.__all__
This PR is currently not based of main so this PR can be evaluated for it's ability to catch the omission fixed in #8137 Once this is approved the PR can be rebased prior to merge.EDIT: today I learned thatactions/checkout
merges the source PR branch into the target so it doesn't matter that this branch is out-of-date, the tests will still run with the changes in #8137 I opened a test PR #8142 that reverts #8137 and shows a failure in theStep
checking unit test added with this PR due to a missingEmiCorrStep
. See: https://github.com/spacetelescope/jwst/actions/runs/7224942055/job/19687349741?pr=8142#step:10:441MasterBackgroundMosStep
appears to be a special case of aPipeline
being treated as aStep
jwst/jwst/master_background/master_background_mos_step.py
Line 20 in 2650df7
It appears this change was introduced in #5317
The unit test specially handles
MasterBackgroundMosStep
(treating it like aStep
instead of aPipeline
).Checklist for maintainers
CHANGES.rst
within the relevant release sectionHow to run regression tests on a PR