DM-30148: PipelineTasks use wrong label as name #123
Conversation
kfindeisen
force-pushed
the
tickets/DM-30148
branch
from
5552e43
to
6d7b229
Compare
| return mock | ||
|
|
||
|
|
||
| class TaskFactoryTestCase(unittest.TestCase): |
There was a problem hiding this comment.
Please add a docstring: it's not obvious to me what a "TaskFactory" test would be testing. I think it's testing TaskFactory.makeTask() with a variety of combinations of optional arguments?
There was a problem hiding this comment.
It tests the functionality of TaskFactory (which happens to consist of one method)? I'm not sure what I could say that wouldn't be obvious.
There was a problem hiding this comment.
Hmmm, maybe the fact that TaskFactory itself is short on documentation is really my problem? I was looking at the test without knowing anything about TaskFactory: I didn't know it only had one method.
tests/test_taskFactory.py
Outdated
| overrides=None, butler=None) | ||
| self.tracker.assert_called_with(config=FakeConfig(), initInputs=None, name=None) | ||
|
|
||
| def testAllOptions(self): |
There was a problem hiding this comment.
This is a test of makeTask called with all kwargs? Maybe rename it or add a docstring.
There was a problem hiding this comment.
testOnlyMandatoryArg and testAllArgs?
Note that "kwargs" is kind of a pointless term here, since none of the parameters have defaults.
Policy is to only update for the current year, but I've added missing entries for 2019-2020 as well.
kfindeisen
force-pushed
the
tickets/DM-30148
branch
from
f2d04a5
to
a797b10
Compare
This PR fixes a name collision inadvertently introduced in #122, and adds unit tests to catch similar bugs in the future.