feat: require additional fields for historical imagery (require linz extension) #199
Conversation
amfage
requested review from
l0b0,
MitchellPaff,
MDavidson17,
dwsilk and
paulfouquet
extensions/historical-imagery/tests/historical-imagery_collection.test.js
Outdated
Show resolved
Hide resolved
…:linz/stac into feat/historical-imagery-new-requirements
| @@ -30,6 +30,18 @@ o.spec('Aerial Photo Extension Collection', () => { | |||
| o(valid).equals(true)(JSON.stringify(validate.errors, null, 2)); | |||
| }); | |||
|
|
|||
| o('Missing summaries section should pass validation', async () => { | |||
There was a problem hiding this comment.
Not sure to understand why this test. Did you add it because you've removed the summaries requirement in the schema?
There was a problem hiding this comment.
Yes, and I thought it would be good to check that it doesn't creep back in at some point. We want to keep these extensions as "light" as possible to make them easier for other people to use.
There was a problem hiding this comment.
We want to keep these extensions as "light" as possible to make them easier for other people to use.
Totally agreed!
My question was focus on the unit test - as this tests that a non required field missing is not failing the validation and I did not understand why a test on this field when there are other non required fields that don't have a similar test. But I understand your point of view:
I thought it would be good to check that it doesn't creep back in at some point.
There was a problem hiding this comment.
Basically it's a regression test.
There was a problem hiding this comment.
What is you point of view on regression tests @l0b0 ? Should they stay there for good? Should we tag them for the code understanding later?
There was a problem hiding this comment.
Interesting question. I'll try to answer with some personal "testing logic":
- The example file should contain examples of all the properties defined by the extension. Otherwise, what's the point?
- The first test shows that the file with all the properties is valid, to make sure we have a sane baseline.
- Subsequent tests show how this baseline can and can't be changed. By removing a mandatory property and asserting that the result fails validation we've tested that it is in fact mandatory. By removing an optional property and asserting that the result passes validation we've tested that it is in fact optional.
- It follows that removing the tests for mandatory or optional properties allows bugs to sneak in unnoticed.
So even though this is a regression test, it's not "just" a regression test. It verifies a useful feature of the schema which is not verified by other tests, so it should probably stay there for the foreseeable future.
There was a problem hiding this comment.
Great answer, thanks @l0b0 !
There was a problem hiding this comment.
Thanks for the interesting discussion @paulfouquet and @l0b0 , very useful.
This is to require the linz extension in the historical-imagery extension. Also to require the processing:software field recommended by the linz extension, and to remove the summaries requirement from aerial-photo and film, as these are already required by the historical-imagery extension.