DM-30497: Bug: DiaSource table cannot find Ixx column #115
Conversation
|
Where can we put a consistency check for these values against those in |
|
Sure, it could work there. As far as I know there is supposed to eventually be a central place to test against for schemas derived from the DPDD. Colin would be the best person to ask about that I would guess. |
|
Why can't we test it here in this package? |
mrawls
changed the title
| @@ -295,6 +296,10 @@ def setUp(self): | |||
| self.apdb.storeDiaObjects(self.diaObjects, | |||
| self.dateTime) | |||
|
|
|||
| # These columns intentionally appear in DiaSource.yaml, | |||
| # but not in the APDB schema | |||
| self.ignoreColumns = ["filterName", "bboxSize", "isDipole"] | |||
There was a problem hiding this comment.
They are specified in the schema-extra file in ap_association.
There was a problem hiding this comment.
Thanks, I figured there was someplace these not-DPDD-yet-important columns lived.
There was a problem hiding this comment.
Actually, I see filterName and isDipole in there, but where does bboxSize come from?
| @@ -390,6 +395,25 @@ def testGetPixelRanges(self): | |||
| self.assertEqual(normPix, testPix) | |||
| self.assertEqual(flipPix, testPix) | |||
|
|
|||
| def test_apdbSchema(self): | |||
There was a problem hiding this comment.
This only tests if the default schema for adpb and the functors run by default match. Also, it is safe to have extra columns in the apdb schema not filled by the DiaSource functor file. The main testing I've been doing to make sure things work has been running ap_verify as an integration test. That only just failed, as we've discovered, as sqlite is not case sensitive in it's column names but Postgres is. We can leave these here for now, but I will point out that it does not protect against mismatches in the schema files.
There was a problem hiding this comment.
How does this not protect against mismatches in the schema files? This test ensures that everything is case-consistent as well.
There was a problem hiding this comment.
The schemas can be set at run time. This only tests for the default ones. It doesn't even pull in all columns that are currently being used and written to the apdb.
There was a problem hiding this comment.
I think testing default schema agreement is better than not testing at all, but that's a good point about the limitation. I agree with John that it would be better to test against an actually instantiated database, but that's more than I'm willing to do for this ticket.
tests/test_loadDiaCatalogs.py
Outdated
| @@ -295,6 +296,10 @@ def setUp(self): | |||
| self.apdb.storeDiaObjects(self.diaObjects, | |||
| self.dateTime) | |||
|
|
|||
| # These columns intentionally appear in DiaSource.yaml, | |||
| # but not in the APDB schema | |||
There was a problem hiding this comment.
Period at the end of sentence, please.
| table_list = list(yaml.safe_load_all(yaml_stream)) | ||
| for table in table_list: | ||
| if table['table'] == 'DiaSource': | ||
| apdbSchemaColumns = [column['name'] for column in table['columns']] |
There was a problem hiding this comment.
Wouldn't you then want a break after this, since there should only be one DiaSource table, so you no longer need to loop over the table_list?
tests/test_loadDiaCatalogs.py
Outdated
| diaSourceFunctor = yaml.safe_load_all(yaml_stream) | ||
| for functor in diaSourceFunctor: | ||
| diaSourceColumns = list(functor['funcs'].keys()) | ||
| for column in diaSourceColumns: |
There was a problem hiding this comment.
Instead of this loop, could you do self.assertEqual(set(diaSourceColumns), set(apdbSchemaColumns)), or maybe self.assertLess(set(diaSourceColumns), set(apdbSchemaColumns)) since I think the one is a proper subset of the other?
| @@ -390,6 +395,25 @@ def testGetPixelRanges(self): | |||
| self.assertEqual(normPix, testPix) | |||
| self.assertEqual(flipPix, testPix) | |||
|
|
|||
| def test_apdbSchema(self): | |||
There was a problem hiding this comment.
How does this not protect against mismatches in the schema files? This test ensures that everything is case-consistent as well.
tests/test_loadDiaCatalogs.py
Outdated
| for functor in diaSourceFunctor: | ||
| diaSourceColumns = [column for column in list(functor['funcs'].keys()) | ||
| if column not in self.ignoreColumns] | ||
| self.assertLess(set(diaSourceColumns), set(apdbSchemaColumns)) |
There was a problem hiding this comment.
This assert should go outside the for loop: you want to build up the list of column names from the DiaSource.yaml file, and then check against that whole list.
No description provided.