Skip to content
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

add tests that verify actual behavior of generated code #1156

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eli-bl
Copy link
Collaborator

@eli-bl eli-bl commented Oct 31, 2024

This creates a new category of end-to-end unit tests which run the generator and then import and execute pieces of the generated code. Unlike the "golden record"-based tests, these don't make assertions about the exact implementation of the generated code, but only its overall behavior.

Also, unlike the existing end-to-end tests, these are self-contained: the spec data (and, optionally, any custom config data) is contained in the unit test code, not in separate files. Each test class uses a small spec designed only for the assertions it will make.

I've added some initial tests of basic model class behavior and docstrings. There aren't currently any tests of endpoint behavior, but I think those could be pretty easily implemented using a mock HTTP client.

The intended benefits are:

  1. These tests prove that the generated code behaves as intended from the point of view of an application using the client. That's unlike the existing end-to-end tests (where we are verifying what the code looks like and only inferring what we think it will do), and the unit tests of the generator (where we're verifying the logic for the internal data structures that will be used to generate the code).
  2. They're more convenient for rapid development of a feature, as tests are lighter-weight and self-contained.
  3. In case of a regression in the generator, failures in these tests can be more specific and easier to interpret. The "golden record"-based tests would also fail, but the output from those is often quite verbose and it can be hard to see which part of the diff is relevant, especially if the error is duplicated in many model/endpoint files.

These aren't intended to completely replace the "golden record"-based tests; those can still be useful to catch regressions in the generator that affect the overall structure of the generated client package. However, I think probably quite a few of the current end-to-end tests could eventually be migrated to this system and might be more maintainable that way.

@@ -168,18 +145,17 @@ def test_literal_enums_end_to_end():
)
)
def test_meta(meta: str, generated_file: Optional[str], expected_file: Optional[str]):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rewritten this and other functions in this file to use the same generate_client context manager, even though the logic in the tests is fairly simple, for two reasons:

  1. It enforces consistent state cleanup. Previously, failures in some of these tests would leave generated files behind in the project directory.
  2. I think it makes the tests a bit easier to read by reducing boilerplate, so what's left is the logic specific to that test.

@eli-bl
Copy link
Collaborator Author

eli-bl commented Oct 31, 2024

I'm not sure why the coverage job is failing - it seems to be saying that there's no coverage for the --meta=none CLI usage, but there is still a test that does that (test_none_meta) and I don't think my changes made a difference in what it does.

@eli-bl eli-bl requested a review from dbanty October 31, 2024 22:44
@eli-bl
Copy link
Collaborator Author

eli-bl commented Oct 31, 2024

By the way, I think this mechanism would be especially useful for testing something like my discriminator PR, #1156 - since that is ultimately about validation behavior.

@eli-bl eli-bl marked this pull request as ready for review November 4, 2024 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant