-
Notifications
You must be signed in to change notification settings - Fork 1
add tests against generated code #217
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
Conversation
@@ -2858,56 +2858,6 @@ | |||
"ModelWithBackslashInDescription": { | |||
"type": "object", | |||
"description": "Description with special character: \\" | |||
}, |
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.
Here and in the next file, and in golden_record
, I've removed some schemas that no longer need to be in the golden-record-based tests. There are now better equivalent tests in test_unions.py
.
|
||
|
||
@_attrs_define | ||
class ADiscriminatedUnionType1: |
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.
Here's an example of one kind of testing I'm trying to get away from. The "golden record"-based tests work by taking a huge spec document, generating a client from it, and then verifying that the resulting code looks exactly like this reference version. Since the reference version was created by the exact same process, this doesn't really verify that the code is what we want— just that the code is the same as it was before, so this really only worked as a regression test.
@@ -13,50 +12,6 @@ | |||
MODULE_NAME = "openapi_python_client.parser.openapi" | |||
|
|||
|
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.
The stuff I've removed from this file, and all the other files below, is obsolete because I now have more straightforward tests covering the same conditions.
Failure cases like the ones here are now in end_to_end_tests/generator_errors_and_warnings
; success cases are in end_to_end_tests/generated_code_live_tests
.
self.monkeypatch.undo() | ||
for module_name in set(sys.modules.keys()) - self.old_modules: | ||
del sys.modules[module_name] | ||
shutil.rmtree(self.output_path, ignore_errors=True) |
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.
Btw, the proof that this "temporarily change the path & modules" stuff works is simply that running all the tests in generated_code_live_tests
in a single pytest run passes. Since many of the test classes reuse the same generated modules and class names for different things, if the sandboxing didn't work then the tests would inevitably pollute each other and fail.
) | ||
|
||
|
||
@with_generated_client_fixture( |
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.
The following test class covers all of the valid cases for discriminators. The error cases are in test_invalid_unions.py
.
@@ -11,7 +11,7 @@ jobs: | |||
test: | |||
strategy: | |||
matrix: | |||
python: [ "3.8", "3.9", "3.10", "3.11", "3.12", "3.13" ] | |||
python: [ "3.9", "3.10", "3.11", "3.12", "3.13" ] |
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'll be dropping 3.8 anyway, but I removed it from the CI test matrix here because some of the stuff in the new tests (like assertions about type annotations) is a lot harder to do in 3.8.
This is a tests-only change that I've also submitted to the main repo in openapi-generators#1156; see PR description there.
The goals are:
I don't think all of these changes need to be reviewed in detail; a lot of them are variations on the same types of things. I've called out the important points in PR comments.
One thing that may not be obvious is that the CI builds for this project include a 100% code coverage requirement. So, even though I've cut out a pretty large amount of old test code, the fact that the builds still pass is proof that my new stuff is adequately covering all the same code paths.