Strengthen the sanity checks, and fix some minor style or bugs uncovered #579
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A subtest in Python means that it will continue running the other examples (tests, cases, whatever) and show an error for each one, rather than simply stopping on the first invalid (test, case, whatever). This should make it easier for seeing what's wrong in CI runs without needing to repeatedly run each time. A further improvement would be grouping tests "horizontally" across versions rather than just within a version, as often authors are copy pasting tests across each version it applies to. With a subtest, they'll see N failures for N versions, whereas it'd be better to see 1 failure for e.g. test description which is too long across all N versions.
The reason we do this is both to force conciseness but also because runners of the suite likely turn one or more of the combination of case description and test description into an identifier within the implementer's test framework. If descriptions are really long, it makes things like copy pasting test names more annoying or difficult. 150 is quite long -- we use 70 for test descriptions, so in total this means potentially 220 characters for the combined length. I'd like to bring this down a bit but this is the number that passes all existing descriptions at least.
The reason we do this for tests is to allow implementers to use test descriptions as unique identifiers, and also because there's a small chance a duplicate description is a copy paste error, or some other test bug. The same reasoning applies to cases. 0da2aaa is another good reason (which in theory applies to both test cases and individual tests) where a case was accidentally duplicated a second time during backporting (where it was already present). See e.g. the first file fixed here, where descriptions should really indicate what validator is being tested for clarity. We don't however check directly for schema duplication in cases, as one could imagine distinct scenarios being tested with the same underlying schema. Fixing further files will be done in successive commits.
Fixes a minor inefficiency in draft3 where separate cases were used instead of just 2 tests.
These were fully duplicated accidentally (which the uniqueness check now finds).
It makes descriptions longer (which we independently check for) but also is stylistically a bit awkward. https://jml.io/pages/test-docstrings.html is a good post on the subject, with a sort of algorithm for removing this and related words. Fix a first file (anchor) where descriptions are shortened and clarified a bit. Further files will be fixed in successive commits.
There aren't any currently (though one was just removed in a previous 'should' commit).
handrews
approved these changes
Aug 5, 2022
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This looks a bit noisy, especially due to fixing across multiple drafts, but it does a few simple things to make the sanity checks a bit better. Only one is newish (mentioned below), the rest are really things we've always really "wanted" but was apparently missing from the sanity check. Specifically the changes here:
A bit more detail is in each commit message if you're even more curious, but hopefully it's clear that each description changed is at least as good as before.
Feel free to ignore reviewing the Python code if you (future reviewer) aren't comfortable reading it (or feel free to press through and review it).