DM-17004: JointcalRunner.__call__ not receiving "butler" in kwargs #130
Conversation
tests/test_jointcalRunner.py
Outdated
| """Test that JointcalRunner calls JointcalTask with appropriate arguments.""" | ||
| @classmethod | ||
| def setUpClass(cls): | ||
| cls.data_dir = os.path.join(lsst.utils.getPackageDir('jointcal'), 'tests/data') |
There was a problem hiding this comment.
I strongly prefer that directories like this are calculated relative to __file__ rather than requiring getPackageDir to work. Also, I think this can be calculated once at the top of the file and doesn't need to be calculated in setUpClass.
There was a problem hiding this comment.
Why do you prefer to put it at the top of the file instead of in setUpClass? setUpClass is where "do this once" things go.
There was a problem hiding this comment.
Both places work, but I tend to prefer putting calculations at file scope that are fixed for the file and I would use setUpClass or setUp to calculate values derived from that fixed global.
There was a problem hiding this comment.
Why do you prefer using __file__ to getPackageDir? The former requires three nested os.path.blah things.
There was a problem hiding this comment.
Because I don't believe that a third party package should be used to find some test data that is in the directory below the test script itself. I also think that if I run this test script in isolation then it should find the data related to this version of the test script and not the data relative to some EUPS package setup that might be not what I intend.
tests/test_jointcalRunner.py
Outdated
|
|
||
|
|
||
| class TestJointcalRunner(lsst.utils.tests.TestCase): | ||
| """Test that JointcalRunner calls JointcalTask with appropriate arguments.""" |
There was a problem hiding this comment.
This test class is sufficiently different to most others we have that it would be nice if there were a couple of sentences here that gave an overview of what was being done.
tests/test_jointcalRunner.py
Outdated
| tracts : `list` [`int`] | ||
| List of tracts to build DataRefs for. | ||
| """ | ||
| configfile = os.path.join(lsst.utils.getPackageDir('jointcal'), 'tests/config/config.py') |
There was a problem hiding this comment.
Change to be relative from __file__ directory if data_dir is changing.
tests/test_jointcalRunner.py
Outdated
| input_dir = os.path.join(self.data_dir, 'cfht_minimal') | ||
| # the calling method is one step back on the stack: use it to specify the output repo. | ||
| caller = inspect.stack()[1].function | ||
| output_dir = os.path.join('.test', self.__class__.__name__, caller) |
There was a problem hiding this comment.
Are you doing these shenanigans to aid debugging the test? Why .test/? Why not a temp directory? Is a directory actually created or is that mocked too? Can you be more explicit in your description that you want to name this directory after the test method that is being executed rather than saying "the calling method"?
There was a problem hiding this comment.
Good point: I copied this from the other jointcal tests where not having them auto-deleted (i.e. tempfile) has been very useful. In this case, tempfile is definitely appropriate.
parejkoj
force-pushed
the
tickets/DM-17004
branch
from
14159ba
to
4a37b44
Compare
|
I've pushed some comments: do they help clarify what is going on? |
Add test that mocks Butler and JointcalTask to allow testing calling JointcalRunner with different numbers of tracts specified.
parejkoj
force-pushed
the
tickets/DM-17004
branch
from
4a37b44
to
4cd7ef5
Compare
No description provided.