DM-13524: Add unit tests for ingestion #24
Conversation
kfindeisen
force-pushed
the
tickets/DM-13524
branch
from
493f650
to
541e01b
Compare
tests/test_ingestion.py
Outdated
| Returns | ||
| ------- | ||
| butler : `lsst.daf.persistence.Butler` | ||
| A butler that should all be capable of finding ingested science data. |
There was a problem hiding this comment.
Here and in the next one, "A butler that should all be capable of finding" is oddly worded. Do you mean "should be capable of finding all ingested [...] data" ?
tests/test_ingestion.py
Outdated
| 'mapperArgs': {'calibRoot': self._calibRepo}}) | ||
|
|
||
| def tearDown(self): | ||
| shutil.rmtree(self._repo, ignore_errors=True) |
There was a problem hiding this comment.
This does make sense here but I'm just laughing to myself about ignore_errors as an argument 😆
tests/test_ingestion.py
Outdated
| calibFile) for calibFile in | ||
| [os.path.join('fci.fits'), | ||
| os.path.join('zci.fits')] | ||
| ] |
There was a problem hiding this comment.
This definition of files is difficult to make sense of over several lines. Consider defining testDataDir first and then grabbing the two files.
| @unittest.skip("Dataset enumeration requires specific data keys for date, filter, etc., see DM-12762") | ||
| def testNoFileIngest(self): | ||
| """Test that attempts to ingest nothing do nothing. | ||
| """ |
There was a problem hiding this comment.
I'm not convinced that loudly crashing is a bad thing if a user tries to ingest nothing, because I can't think of a use case where I would intentionally want to ingest nothing. Am I missing something?
There was a problem hiding this comment.
I can't think of a specific use case either, but failing over something like this is very unusual/surprising behavior. For example, if you try to iterate over an empty list, you expect the loop to simply do nothing instead of punishing you for not writing special-case code that would have amounted to the same thing.
Also, having a low-level function exit because of invalid inputs is extremely bad practice. If we want to ban empty file lists, the function should raise an exception so that the program can decide whether or not an exit is warranted (and have a chance to clean up properly either way).
kfindeisen
force-pushed
the
tickets/DM-13524
branch
from
541e01b
to
00d6537
Compare
The previous implementation required a highly specific yet undocumented directory structure. The new version is more flexible.
The tests test basic "normal operation" functionality. Testing no-ops is not possible until we fix a repository bug, and testing invalid files is impossible given Stack error-handling conventions.
kfindeisen
force-pushed
the
tickets/DM-13524
branch
from
00d6537
to
e767f85
Compare
This PR adds minimalist tests of ingestion. A couple of minor bugs are fixed, but the rest are deferred to future tickets.
There is no test for attempts to ingest nonexistent files, as the default
*IngestionTaskbehavior (log and ignore) is difficult to test.