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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Strict validation in configuration file (v2 only) #4607
Strict validation in configuration file (v2 only) #4607
Changes from all commits
0bbbc6a
573c878
2fc02e7
28ccce3
06b40f8
1187efb
67d0420
c60f59a
a2c6c9d
7ce0202
d33854a
997b5e2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always pass the
version
key, but that's is already validated from outsideThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may worth to have some unit test for the specific methods that do the trick:
_get_extra_key
andvalidate_keys
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In particular the one that is recursive with different start/stop conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate_keys
is already called when runningvalidate
._get_extra_key
is validated here https://github.com/stsewd/readthedocs.org/blob/strict-validation/readthedocs/config/tests/test_config.py#L1718-L1743There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added one more test, but we are already covered anyway with the other tests (they don't throw an exception).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not throwing an exception doesn't test that
_get_extra_key
returns the correct keys. That's the test I wanted.The same for
validate_keys
to check that if there is extra keys, it fails.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually
test_strict_validation
test that, an error is thrown andkey
andcode
are the correct in the error. We test withvalidate
as entrypoint, as that's the way to use the config module (all tests were changed to test like that)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what you say. Although, I'd like to have a unit test for the methods I mentioned before for these cases:
_get_extra_key
validate_keys
The
test_strict_validation
is testing only one case and using the complete flow. I'd like to test the inner pieces, in particular the than that it's recursive (it's very easy to make a mistake on initial conditions)