Validate test files against the schema they declare #1813
Conversation
There was a problem hiding this comment.
The unified-tests.yml Github action runs both make and check_schema_version.sh, which seems redundant. What is the difference between those two scripts?
There was a problem hiding this comment.
Um, yeah. check_schema_version.sh indeed handles the exact same thing we're doing here - with that in mind this PR definitely isn't necessary, but we should remove the double checks. I'll follow up on this.
|
|
||
| unified-test-format: unified-test-format/invalid unified-test-format/valid-fail unified-test-format/valid-pass | ||
|
|
||
| unified-test-format/invalid: HAS_AJV | ||
| @# Redirect stdout to hide expected validation errors | ||
| @ajv test -s $(SCHEMA) -d "invalid/*.yml" --invalid > /dev/null && echo "invalid/*.yml passed test" | ||
| @./check-files.sh invalid invalid/*.yml > /dev/null && echo "invalid/*.yml passed test" |
There was a problem hiding this comment.
This is out of scope for this change, but maintaining an inclusive list of all test subdirectories seems dangerous because it can easily miss new specs or test directory changes. Can we switch to excluding the invalid test files and including all other test files in source/?
There was a problem hiding this comment.
When I last refactored the Makefile, I made sure to add individual targets for ease of use. We could add a separate target that checks all YAML files, paired with logic to skip any YAML test file that doesn't include a schemaVersion parameter on the top level. That would leave only the invalid files to be skipped.
Previously we used a hard-coded schema version to validate test files, which needed updating whenever we release a new schema version. This could be replaced by using schema-latest, but this doesn't solve another problem we currently face: since we validate test files with a newer version of the schema, it could be that they use functionality that was only introduced in a later version without us ever knowing.
The solution introduced here greps a given file for its
schemaVersion, then validates the test against that schema version. If the schema in question is not given, the file is validated againstlatestand a warning is printed. This is necessary because there is a valid-fail test that specifies schema version0.1, which the validator wouldn't be able to find. The logic in the file also accounts for changing thespecset in the call toajvaccording to whether the schema uses draft-2019-09 (1.24 and newer) or draft-7 (older schema versions).An unfortunate side effect of this change is that validation now takes significantly longer than before. While previously a
makerun took ~6 seconds, it now tales ~150 seconds. Parallelising this usingmake -j10on my machine improved performance to ~56 seconds, which is still considerably slower than before. However, given the improvements provided, the fact that this is usually only run in CI, and the fact that people can runmakeon just the spec they've worked on makes me think that this delay is acceptable for the time being.Please complete the following before merging:
[ ] Update changelog.[ ] Test changes in at least one language driver.[ ] Test these changes against all server versions and topologies (including standalone, replica set, and shardedclusters).