DM-13567: Add tests to ap_pipe #40
Conversation
There was a problem hiding this comment.
A variety of comments. I think the most important structural one is to see if you can refactor testReuseExistingOutput so that it doesn't have nested loops. Those can make debugging test failures a pain.
You need to add setupOptional(ap_pipe_testdata) to this package's ups table.
tests/test_appipe.py
Outdated
| except pexExcept.NotFoundError: | ||
| raise unittest.SkipTest("ap_pipe_testdata not set up") | ||
|
|
||
| cls.config = cls._makeDefaultConfig() |
There was a problem hiding this comment.
I would have config be set in setUp, so that it is always reset for each test.
|
|
||
| def makeMockDataRef(datasetType, level=None, dataId={}, **rest): | ||
| mockDataRef = Mock(dafPersist.ButlerDataRef) | ||
| mockDataRef.dataId = dict(dataId, **rest) |
There was a problem hiding this comment.
I'm surprised that this worked! Guess I learned a new thing about dict initialization today.
tests/test_appipe.py
Outdated
| with patch.object(task, "differencer") as mockDifferencer: | ||
| with patch.object(task, "diaSourceDpddifier") as mockDpddifier: | ||
| with patch.object(task, "associator") as mockAssociator: | ||
| with patch.object(task, "diaForcedSource") as mockForcedSource: |
There was a problem hiding this comment.
Ouch. You don't need to nest these, you can put them all in one context: with patch1() as mock1, patch2() as mock2, ...: doStuff().
There was a problem hiding this comment.
Thank you, that works!
tests/test_appipe.py
Outdated
| def testGenericRun(self): | ||
| # test the normal workflow of each ap_pipe step | ||
| task = ApPipeTask(self.butler, config=self.config) | ||
| inputRef = self.butler.dataRef("raw", **self.dataId) |
There was a problem hiding this comment.
I see this inputRef repeated below: can you put it into setUp?
tests/test_appipe.py
Outdated
| forcedSource=mockForcedSource) | ||
|
|
||
| def testGenericRun(self): | ||
| # test the normal workflow of each ap_pipe step |
There was a problem hiding this comment.
Comment should be docstring?
tests/test_appipe.py
Outdated
| for skippable in [ | ||
| ['ccdProcessor'], | ||
| ['ccdProcessor', 'differencer'], | ||
| ]: |
There was a problem hiding this comment.
Loops in tests make the tests difficult to untangle. You're only doing two things here, so I think it would be clearer if you made the internals of this into a separate method (e.g. checkSkippedTask) and then call it:
self.checkSkippedTask(['ccdProcessor'])
self.checkSkippedTask(['ccdProcessor', 'differencer'])
Similarly for the loop below, although I'm not sure how to unravel that one.
There was a problem hiding this comment.
We fixed the first loop as suggested and added a message that identifies the subtask being tested for the second loop.
tests/test_appipe.py
Outdated
| with self.mockPatchSubtasks(task) as subtasks: | ||
| # Self-subtraction ok because we're not actually doing any processing | ||
| task.runDataRef(inputRef, templateIds=[self.dataId]) | ||
| subtasks.ccdProcessor.runDataRef.assert_called() # Should be twice |
There was a problem hiding this comment.
You can check assertEqual(runDataRef.call_count, 2) here.
tests/test_appipe.py
Outdated
| task = ApPipeTask(self.butler, config=calexpConfig) | ||
| inputRef = self.butler.dataRef("raw", **self.dataId) | ||
| with self.mockPatchSubtasks(task) as subtasks: | ||
| # Self-subtraction ok because we're not actually doing any processing |
There was a problem hiding this comment.
I don't understand what this comment is referring to?
There was a problem hiding this comment.
Clarified. It was referring to the fact that the differencing template has the same dataId as the science template.
|
|
||
| @contextlib.contextmanager | ||
| def mockPatchSubtasks(self, task): | ||
| """Make mocks for all the ap_pipe subtasks. |
There was a problem hiding this comment.
I think it's worth explaining in the docstring why this context manager is necessary: I was thinking that it wasn't because calls to mocks always return other mocks, so e.g. task.differencer.runDataRef is a mock. Except that task is not a mock, so you do need this. I thought you might be able to remove the context manager and put it into setup, patching ApPipeTask itself, but the subtasks don't exist until the Task is created, so you do need this.
So yes, some more explanation here would be good.
tests/test_appipe.py
Outdated
| Parameters | ||
| ---------- | ||
| task : `lsst.ap.pipe.ApPipeTask` | ||
| the task whose subtasks will be mocked |
mrawls
force-pushed
the
tickets/DM-13567
branch
3 times, most recently
from
37ffb77
to
7253a4c
Compare
tests/test_appipe.py
Outdated
| self.checkReuseExistingOutput(task, ['ccdProcessor', 'differencer']) | ||
|
|
||
| def checkReuseExistingOutput(self, task, skippable): | ||
| # Mock guarantees that all "has this been made" tests pass, |
There was a problem hiding this comment.
Docstring for this method?
Pair coded with @kfindeisen. The new tests require the ap_pipe_testdata package.