DM-13849: Convert all ap_verify test data to obs_test #31
Conversation
There was a problem hiding this comment.
Everything here is clear and seems like it will work, but I'm a bit nervous about moving the tests to be entirely based on obs_test instead of real images from a real camera. I liked having the test-based confirmation that nothing we did broke a real (albeit finicky) decam dataset. Do we have any plans to make sure ap_verify plays well with other obs_ packages?
tests/test_dataset.py
Outdated
| try: | ||
| lsst.utils.getPackageDir(cls.testDataset) | ||
| except pexExcept.NotFoundError: | ||
| raise unittest.SkipTest(cls.testDataset + ' not set up') | ||
|
|
||
| # Hack the config for testing purposes | ||
| # Note that Config.instance is supposed to be immutable, so some code may break | ||
| Config.instance._allInfo['datasets.' + cls.datasetKey] = cls.testDataset |
There was a problem hiding this comment.
Your comments here don't exactly inspire confidence. What's "some code may break" all about?
There was a problem hiding this comment.
Config.instance loads itself from ap_verify's config files before it gets used (at import time, I think?). The assumption is that Config.instance simply reflects the contents of those files, i.e., it does not change within a program run.
So you could imagine that some code (yet to be written) reads something from Config.instance before DatasetTestSuite.setUpClass runs, then Config.instance gets modified, then the code reads Config.instance later and gets an inconsistent result, which causes it to do something crazy (kind of literally). This could be code in or called by a different test, so it's not possible to control it.
The alternative is actually putting ap_verify_testdata in the config file. However, it would then be usable by ap_verify itself, and even show up as one of the available datasets in the -h text. A hack that only affects the tests but not the program itself seems like the lesser evil, especially since it means that test_dataset.py can't be broken by stuff missing from the config file.
There was a problem hiding this comment.
Huh. That is a somewhat interesting hypothetical case! I didn't have the context that Config.instance isn't "normally" accessed from a place like this. Perhaps instead of "so some code may break" you could say "so if the config has changed since import there may be a discrepancy."
There was a problem hiding this comment.
Not sure what you mean by "normally accessed" -- any part of the ap_verify codebase is free to access Config.instance at will; that's what it's for.
How about:
Note that Config.instance is supposed to be immutable, so, depending on initialization order, this modification may cause other tests to see inconsistent config values
python/lsst/ap/verify/dataset.py
Outdated
| try: | ||
| datasetPackage = self._getDatasetInfo()[datasetId] | ||
| except KeyError: | ||
| datasetPackage = self._getDatasetInfo()[datasetId] |
There was a problem hiding this comment.
You're reasonably confident that trying to get a non-existent dataset will return None and won't fail in some other way?
There was a problem hiding this comment.
This is an apparently deliberate deviation of daf.persistence.Policy from dict-like behavior. 😞
Since it's undocumented behavior, I guess I could try to handle both None return and KeyError, though that would involve duplicating code. (Regardless, I should add a regression test for the bug I'm fixing here.)
There was a problem hiding this comment.
I see. I lean toward whatever will make error messages less cryptic to users, which may mean duplicating code in this case.
The non-IngestTask argument was needed to work with DecamIngestTask prior to DM-14237. Now that DecamIngestTask is an IngestTask in the Liskov sense, we can use the generic interface.
daf.persistence.Policy returns `None` instead of raising `KeyError` for invalid lookups. However, this is not documented.
This code should have been removed when Datasets no longer set up their dataset packages (DM-12853).
kfindeisen
force-pushed
the
tickets/DM-13849
branch
from
4f54753
to
6de95ad
Compare
This PR modifies
test_ingestionandtest_datasetto depend onobs_testrather thanobs_decam, and fixes some more minor issues I came across while working on these tests.This PR must be merged after lsst/ap_verify_testdata#1.