bookwyrm-social / bookwyrm

Social reading and reviewing, decentralized with ActivityPub
http://joinbookwyrm.com/
Other
2.24k stars 264 forks source link

Adds test class for checking query durations #3380

Open mouse-reeve opened 3 months ago

mouse-reeve commented 3 months ago

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

mouse-reeve commented 2 months ago

I tried to get fixtures working to include test data, but got stuck trying to generate them on models that inherit with select_subclasses

mouse-reeve commented 1 month ago

@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 https://github.com/bookwyrm-social/bookwyrm/pull/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.

dato commented 1 month ago

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:

the runtime can vary wildly on different machines

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 raise_long_query_runtime as well. But if you think this idea makes sense, I can move forward with it as well.

mouse-reeve commented 1 month ago

Ah yeah I think that's a great idea! I'd be super happy for you to make a mock-up of that