DM-16765: Changes when adding obs_lsst tests #125
Conversation
SimonKrughoff
force-pushed
the
tickets/DM-16765
branch
from
a00e725
to
48bd00f
Compare
| @@ -80,6 +82,7 @@ def setUp_tests(self, butler, mapper, dataIds): | |||
| self.butler = butler | |||
| self.mapper = mapper | |||
| self.dataIds = dataIds | |||
| self.log = Log.getLogger('ObsTests') | |||
There was a problem hiding this comment.
Do we use the logger anywhere in these tests?
python/lsst/obs/base/mapper_tests.py
Outdated
| if not isinstance(location, lsst.daf.persistence.butlerLocation.ButlerComposite): | ||
| self._test_map(location, dataId) | ||
| else: | ||
| self.log.info('ButlerComposite datasets are not tested for mapper functions') |
There was a problem hiding this comment.
Maybe log.warn? At least have some more information as to why they aren't tested.
There was a problem hiding this comment.
Warn does seem appropriate.
| self._test_map(location, dataId) | ||
| else: | ||
| self.log.info('ButlerComposite datasets are not tested for mapper functions') | ||
| location = self.mapper.map("raw", dataId) |
There was a problem hiding this comment.
Please add a comment about why this is different from the above: it took me a fair bit of staring to figure it out.
python/lsst/obs/base/butler_tests.py
Outdated
| exp = self.butler.get(name, self.dataIds[name]) | ||
|
|
||
| exp_md = self.butler.get(name+"_md", self.dataIds[name]) |
| exp = self.butler.get(name, self.dataIds[name]) | ||
|
|
||
| exp_md = self.butler.get(name+"_md", self.dataIds[name]) | ||
| self.assertEqual(type(exp_md), type(exp.getMetadata())) |
There was a problem hiding this comment.
Can you test that the objects themselves are equal (as they should be)? (I expect there's no PropertyList::operator==, so this is annoying, but it really needs to be done.)
There was a problem hiding this comment.
Unfortunately, no. As Jim explained in science pipelines some time ago, we are not completely strict about round tripping the metadata, so there are a few cards in one that aren't in the other.
His suggestion was that this is the best we can do at the moment (and is better than we were doing before).
python/lsst/obs/base/mapper_tests.py
Outdated
| @@ -71,8 +72,10 @@ def setUp_mapper(self, | |||
| dataIds and the results of calling them in queryMetadata | |||
| metadata_output_path : `str` | |||
| path to metadata output associated with dataIds['raw'] | |||
| map_python_type : `str` | |||
| map_python_type : class | |||
There was a problem hiding this comment.
Could be `type` instead of class?
python/lsst/obs/base/mapper_tests.py
Outdated
| if not isinstance(location, lsst.daf.persistence.butlerLocation.ButlerComposite): | ||
| self._test_map(location, dataId) | ||
| else: | ||
| self.log.info('ButlerComposite datasets are not tested for mapper functions') |
There was a problem hiding this comment.
Warn does seem appropriate.
| self._test_map(location, dataId) | ||
| else: | ||
| self.log.info('ButlerComposite datasets are not tested for mapper functions') | ||
| location = self.mapper.map("raw", dataId) |
The parameter that determins whether the configs and metadata will be tested defaults to True since that was what was implied before the change to make those tests optional.
SimonKrughoff
force-pushed
the
tickets/DM-16765
branch
from
e83988c
to
a0cda83
Compare
@parejkoj do you mind having a look at this?