DM-23174: Consolidate daf_butler test code #229
Conversation
Developer guide says to use spans for consecutive years.
There was a problem hiding this comment.
Thanks for this clean up. I have a few questions before I do a final sign off. In particular I'd like @TallJimbo to comment on a couple of the test routines.
| @classmethod | ||
| def setUpClass(cls): | ||
| # Repository should be re-created for each test case, but | ||
| # this has a prohibitive run-time cost at present |
There was a problem hiding this comment.
We need to work this out. tests/test_butler.py creates huge numbers of temp directories with their own butlers and this test class should not be any different. For example:
python tests/test_butler.py
.........................................................x......
----------------------------------------------------------------------
Ran 64 tests in 15.860s
and most of those 64 tests are calling makeRepo in their own temp directory.
There was a problem hiding this comment.
This test on this branch is really fast for me:
$ time python tests/test_testRepo.py
........
----------------------------------------------------------------------
Ran 8 tests in 0.386s
OK
real 0m1.711s
user 0m1.070s
sys 0m0.604s
There was a problem hiding this comment.
Maybe we should use https://pypi.org/project/pytest-profiling/ and compare timings.
There was a problem hiding this comment.
I propose splitting that into another ticket, possibly a high-priority one for February. While I am worried about what's going on, the pipeline test framework is something we need ASAP, and this ticket has already dragged on for two weeks for various reasons (e.g., the Butler API changing due to concurrent development). There is no guarantee that we can track down the cause of the slowdown (which, as far as we know, affects only my computer) in any particular amount of time.
There was a problem hiding this comment.
Ok. Create a new ticket. It is concerning that you see such a problem. I guess it's a good thing that you have demonstrated that you can run all tests with a shared butler and changing collections for each test.
| # outfile has the most obvious effects of any Butler.makeRepo keyword | ||
| with tempfile.TemporaryDirectory() as temp: | ||
| path = os.path.join(temp, 'oddConfig.py') | ||
| makeTestRepo(self.root, {}, outfile=path) |
There was a problem hiding this comment.
I need to think why this works fine given that self.root won't include a butler.yaml file so the Butler created inside makeTestRepo should fail because it won't know about the config written to path. I think at least that the Butler returned will have a different config to the one in path.
The code has been cleaned up, and tests added.
It's hard to explicitly provide correct keys without understanding how the Butler dimensions system works in detail. Moving key constraints to automated (if simple-minded) code greatly reduces the burden on callers.
Each test should have its own collection for isolation, but creating a completely new repository each time is impractical.
This change lets tests avoid using Numpy arrays, whose nonstandard __eq__ behavior makes them poor test objects.
kfindeisen
force-pushed
the
tickets/DM-23174
branch
from
f7ebf5a
to
48bfafd
Compare
This approach is not object-oriented, but making the Datastore interface support possibile child datastores is a nontrivial fix.
kfindeisen
force-pushed
the
tickets/DM-23174
branch
from
48bfafd
to
37c383a
Compare
This PR moves the test utilities previously located in the
tests/directory to a newlsst.daf.butler.testspackage (which is not included inlsst.daf.butler). It also adds some extra utilities designed for test code in other packages, such as functions for creating and configuring mock repositories (see lsst/pipe_base#114 for an example of where this would be useful).