Add spec violation rules #141
Conversation
Signed-off-by: Tom <[email protected]>
Signed-off-by: Tom <[email protected]>
Signed-off-by: Pierrick <[email protected]>
Signed-off-by: Pierrick <[email protected]>
Signed-off-by: Pierrick <[email protected]>
Signed-off-by: Tom <[email protected]>
Signed-off-by: Tom <[email protected]>
Signed-off-by: Tom <[email protected]>
✅ Deploy Preview for gbfs-validator ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
I think a few non-trivial issues remain unresolved:
Taxonomy of validation result
Do we need to differentiate between schema and non-schema errors?
Multiple patterns in use for validation
The are several patterns in use:
- original schema
- patched schemas ("partials")
- imperative rules (in JavaScript),
- and some hybrid of the latter two it seems (in validation of additionalProperties).
I think this is problematic, and it should be clarified what we prefer, and the codebase needs to be clearer.
I think one benefit of using schemas and patched schemas only is that the validation itself is done by ajv and all the results is an output of ajv. It has been argued that these are harder to read than the imperative style, but after reading this PR I'm not 100% convinced.
Another problem with the imperative style is the unclear connection to the version(s) that the rule applies to. In the schema patching pattern it is explicit, whereas here you have to read the code (at least if it's not documented) - and mistakes are harder to spot.
Misc
The rest of the review is mostly technical and cosmetic.
One issue that stood out is inappropriate usage of Array methods, especially map where forEach would be better, and some others.
Some files are getting quite hard to read, and are in dire need of refactoring.
Some confusing folder structure introduced in test folder.
| @@ -1,5 +1,7 @@ | |||
| const fastify = require('fastify') | |||
|
|
|||
| const last_updated_fresh = Math.floor(Date.now() / 1000) - 30 | |||
There was a problem hiding this comment.
It is good practice to not use real clocks in tests. It makes tests more reliable and readable. Jest has as of v27 the ability to do this neatly: https://jestjs.io/docs/jest-object#jestsetsystemtimenow-number--date
There was a problem hiding this comment.
This is a great idea, but this is beyond my knowledge unfortunately (even after reading the docs). Anyone, feel free to suggest an implementation. Thanks!
| }) | ||
| }) | ||
|
|
||
| return errors | ||
| return { errors, nonSchemaErrors, warnings } |
There was a problem hiding this comment.
I still think we need to review the taxonomy here. Why do we differentiate between errors and nonSchemaErrors? Would we not simply just have errors and warnings (where warnings are SHOULD violations)?
There was a problem hiding this comment.
I tried to combine errors and nonSchemaErrors but was limited by my technical skills. Anyone, feel free to suggest an implementation.
| beforeAll(async () => { | ||
| const { MockRequests } = require('./fixtures/v3.0-RC/exaustive') | ||
|
|
||
| class InvalidMockRequests extends MockRequests { |
There was a problem hiding this comment.
I don't really understand what is going on here? Why not just create a new fixture for this test?
There was a problem hiding this comment.
This is beyond my technical skills. Anyone, feel free to suggest an implementation.
| } | ||
|
|
||
| for (const lang of languages) { | ||
| const filtered = data.filter((a) => a.language === lang) |
There was a problem hiding this comment.
Array#some is more efficient here (and condition can be inlined)
There was a problem hiding this comment.
This is beyond my technical knowledge. Anyone, feel free to suggest an implementation. Thanks!
|
One thing I should add in my quest for schema-centric validation is of course that the SHOULD/warning validations are hard to implement that way... |
|
@testower I'm done applying the quick fixes from your suggestions. The other comments are beyond my technical knowledge. I will sync with @davidgamez about next steps. Thanks! |
|
Thank you @tdelmas for the initial PR (#106) and @testower for the thorough PR review. Based on the conversation following #153 (comment), we want to try doing the validation of these new rules by generating new schemas via a builder or a template instead of using imperative rules (eg: JavaScript). For this reason, I believe this PR can be closed in favor of the following issues:
Thank you! |
This PR follows #106.
Feed used for screenshots: https://acoruna.publicbikesystem.net/customer/gbfs/v2/gbfs.json