-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
Adds test class for checking query durations #3380
base: main
Are you sure you want to change the base?
Conversation
This is a first pass and I wrote it specifically so that it would fail on the query in test_book.py
I tried to get fixtures working to include test data, but got stuck trying to generate them on models that inherit with |
@bookwyrm-social/code-review I think this is a helpful change, but I'd love to get some outside input both on the concept and the execution. Since the query runtime isn't always exactly the same on the same machine, it could cause unexpected test failures, and since the runtime can vary wildly on different machines, tests will likely fail or pass locally differently than they do in CI. The duration value that I picked is one that failed prior to the fix in #3378 and passed after it was merged in. I made a real effort to get fixtures with serialized test data and gave up trying to wrangle the multi-table inheritance into serializing in a usable way, which is why I just created 10k edition entries in the one test I really wanted to get to fail. |
This is a great initiative, thanks! 💯 I've been thinking about the challenges that you describe here. I have no code to offer (yet), but for now, regarding this problem:
For me, this hints at the following: instead of having the tests raise on "perceived slowness" a-la-unit testing, have them just emit timing information. Then, never deal in absolutes, and instead compare the emitted information before and after the change, on the same machine. I'm not sure what the developer UX would be for this, but a Github Action seems a good start: for every PR, run the test suite as normal. Then, also run, on the base branch, the tests that emit timing information. And compare the two. That said: this is just an idea. Although I'd like to make time to prepare a mockup, I'm okay if you want to move forward with |
Ah yeah I think that's a great idea! I'd be super happy for you to make a mock-up of that |
Tests
What type of Pull Request is this?
Does this PR change settings or dependencies, or break something?
Details of breaking or configuration changes (if any of above checked)
Description
This is a first pass and I wrote it specifically so that it would fail on the query in test_book.py
Documentation