Create validation for document types with error handling#1846
Create validation for document types with error handling#1846freakboy3742 merged 17 commits intobeeware:mainfrom
Conversation
freakboy3742
left a comment
There was a problem hiding this comment.
This looks really good! I've left some comments inline, but they're mostly cosmetic wording changes for error messages, or suggestions about ways to simplify tests and/or code - the core of this is really solid.
| validate_document_icon("ext", invalid_document) | ||
|
|
||
|
|
||
| def test_validate_document_invalid_icon(): |
There was a problem hiding this comment.
It's generally a good idea to parameterize "invalid" tests so that you can try out a range of things that you know should be invalid. Even if there's only 2 initially, having a parameterised test makes it easier to add other examples later if the need arises.
The same is true of "valid" tests as well - there's less need for tests of valid examples, but it can be helpful sometimes to explicitly validate some cases (e.g., that strings with and without spaces are valid). That's not an issue here explicitly, but as protection against future possible validations, it's helpful to have the testing infrastructure in place.
| "extension": "ext", | ||
| } | ||
| # Failure raises an exception | ||
| with pytest.raises(BriefcaseConfigError): |
There was a problem hiding this comment.
As before - adding a match clause is helpful here (and in other raises tests in this file)
| "extension": "ext", | ||
| } | ||
| # Failure raises an exception | ||
| with pytest.raises(BriefcaseConfigError): |
There was a problem hiding this comment.
As before - adding a match clause is helpful here.
| ], | ||
| ) | ||
| def test_validations(validation): | ||
| valid_document = { |
There was a problem hiding this comment.
The "valid" document type declaration could be used as a template for all other uses. Rather than defining a 4 line dictionary in every test, you can define a single fixture for valid_document, and then each test can either modify or delete the one field that needs to be modified to generate an invalid document type for a specific test.
Co-authored-by: Russell Keith-Magee <[email protected]>
Co-authored-by: Russell Keith-Magee <[email protected]>
Co-authored-by: Russell Keith-Magee <[email protected]>
Co-authored-by: Russell Keith-Magee <[email protected]>
Co-authored-by: Russell Keith-Magee <[email protected]>
Co-authored-by: Russell Keith-Magee <[email protected]>
Co-authored-by: Russell Keith-Magee <[email protected]>
|
Thanks for all the feedback! Please take a look at the latest version, which incorporates your feedback. Thank you! |
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for those changes! I've made a couple of minor tweaks - merging the four validation methods into one (since there's never going to be a reason to independently validate each field), and some minor stylistic tweaks to the test cases (adding test descriptions, and some formatting changes to the test parameterization)- but otherwise, this looks great! Thanks for the contribution!
|
The CI failure is something we've seen before, but historically it's been a transient thing. However, in this case, it looks like it's repeatable. AFAICT, it's not something you've caused with these changes (other than producing a test case that can reproduce it reliably), so unless you're particularly enthused to go hunting an obscure Ubuntu bug, you don't need to dig into it. |
Fixes #1770
PR Checklist: