DM-12117 check for old butler parents that are not nested #84
Conversation
n8pease
force-pushed
the
tickets/DM-12117
branch
from
79d183e
to
a03b0bf
Compare
parejkoj
changed the title
n8pease
force-pushed
the
tickets/DM-12117
branch
from
5bc39bf
to
617ff0e
Compare
| # whole RepositoryCfg, that described the Old Butler repository (including the mapperArgs that | ||
| # were used with it), was recorded as a "nested" repository cfg. That checkin did not account | ||
| # for the fact that there were repositoryCfg.yaml files in the world with only the path to | ||
| # Old Butler repositories in the parents list. |
There was a problem hiding this comment.
Thanks for adding a descriptive note about this block.
tests/test_DM-12117.py
Outdated
| """.format(mapperArgs) | ||
|
|
||
| def test(self): | ||
| """Test that an Old Butler parent repo that is can be loaded by a New Butler output repo and that the |
There was a problem hiding this comment.
"that is can be"?
Also, docstrings should be 79 char.
| cfg._parents[i] = cfg._normalizeParents(cfg.root, [parentCfg])[0] | ||
|
|
||
|
|
||
| def checkCfgs(cfgList): |
There was a problem hiding this comment.
It's not clear to me where this method gets used: it's not used in this commit anywhere. Also, it has no docstring.
tests/test_DM-12117.py
Outdated
| dirty: true | ||
| """.format(mapperArgs) | ||
|
|
||
| def test(self): |
There was a problem hiding this comment.
More descriptive method name than test: maybe test_oldButlerParents or something?
tests/test_DM-12117.py
Outdated
| 5. do the same, but specify a mapper arg, and verify that that mapper arg is passed to the parent as | ||
| well as the root repo. | ||
| """ | ||
| for mapperArgs in ({}, {'calib': 'foo'}): |
There was a problem hiding this comment.
Doing this as a for loop makes it hard to parse what's going on. I'd suggest writing a helper method _testSomething/checkSomething that takes mapperArgs and does what is inside the loop, and then unrolling the loop like this (if I'm interpreting it correctly):
def testSomething(self):
_testSomething({})
_testSomething({'calib':'foo'})This will make test failures much easier to debug, since you won't have to figure out which loop is failing.
Actually, since you're calling tearDown manually at the bottom of this, I'd recommend just making each of the above calls its own test method: testSomethingDefaults and testSomethingCalib. Then tearDown will behave as expected. And the method docstring here can go up to the class level.
tests/test_DM-12117.py
Outdated
| mapperArgs.get('calib')) | ||
| self.assertEqual(butler._repos.inputs()[1].repo._mapper.kwargs.get('calib'), | ||
| mapperArgs.get('calib')) | ||
| self.tearDown() |
There was a problem hiding this comment.
Don't call tearDown manually.
n8pease
force-pushed
the
tickets/DM-12117
branch
2 times, most recently
from
9971953
to
13c4198
Compare
we started nesting Old Butler parent RepositoryCfgs to preserve implicit arguments like what calib was used to load them. Some repositoryCfg.yaml files exist where the Old Butler parent repository is indicated only by path. This fix internally converts that path to a RepositoryCfg and the root repository's mapperArgs are also applied to the internal RepositoryCfg.
n8pease
force-pushed
the
tickets/DM-12117
branch
from
8f7657d
to
ffa867f
Compare
we started nesting Old Butler parent RepositoryCfgs to preserve
implicit arguments like what calib was used to load them. Some
repositoryCfg.yaml files exist where the Old Butler parent
repository is indicated only by path. This fix internally
converts that path to a RepositoryCfg and the root repository's
mapperArgs are also applied to the internal RepositoryCfg.