-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
load tests fail obscurely when nix dependencies change #2308
Comments
* Write vegeta output to file explicitly. The previous version using `tee` captured the standard output of the `withPgrst` helper, too, causing weird errors when those had unexpected output. * Log and fail the postgrest build explicitly. In situations such as changing nix dependencies, the `cabal build` run could fail, but this wasn't visible in CI. With these changes, the load test now fails with a nice error message when the build fails.
* Write vegeta output to file explicitly. The previous version using `tee` captured the standard output of the `withPgrst` helper, too, causing weird errors when those had unexpected output. * Log and fail the postgrest build explicitly. In situations such as changing nix dependencies, the `cabal build` run could fail, but this wasn't visible in CI. With these changes, the load test now fails with a nice error message when the build fails.
I observed this earlier (didn't bother to look it up when and where) and came to the conclusion that this is unavoidable, basically. IIRC when the cabal file changes, this will be the consequence - and it's perfectly fine. The idea for the loadtest is to compare code the code we have written and the impact it has on performance. It is not meant to test performance of dependencies or performance in any setting close to reality. It's quite artificial in fact. I think what we should do is: Run the loadtest conditionally and only when the cabal file does not change - i.e. we don't have a dependency update. We just need to make sure that our PRs are either dependency updates or changes to actual code. Then, it would be absolutely fine to just not run the loadtest for dependency updates. |
@wolfgangwalther Thanks, yes that makes sense. I'll update the issue accordingly. Also thanks for review on #2309! |
This allows running the load test against main even if the dependencies change.
This allows running the load test against main even if the dependencies change.
The
Loadtest (Nix)
CI check compares Postgrest versions on themain
and PR branches. If the nix dependencies are changed in a PR, this will typically cause build failures of themain
branch, causing the check to fail.E.g., in PRs #2306 and #2292, we've seen the
Loadtest (Nix)
CI check failing with(e.g. here: https://github.com/PostgREST/postgrest/runs/6784583539?check_suite_focus=true)
The text was updated successfully, but these errors were encountered: