Run performance tests on all PR against base branch #1630
Conversation
38fc9eb to
4fef17b
Compare
1f99e84 to
b5f4e63
Compare
963e4cc to
fa4dc9e
Compare
4e13626 to
fdb9406
Compare
171f783 to
d330f9d
Compare
d3c562b to
773e28d
Compare
Maybe we can have it displayed as a table so it's more readable:
|
|
OK though to make it also readable as a text file (the intent is that the outputed report markdown file is both intended for humans with a text editor and to scripts outputing optionally richer text formats - like as a Github comment) is more complex when doing a table vs a list. But i'll try |
66d9814 to
23edde8
Compare
|
Automated performance checks have been performed on commit Tests results✅ Tests have passed. Performance tests 1st run outputNo significative change in performance for tests:
If you want to skip performance checks for latter commits, add the |
|
Automated performance checks have been performed on commit Tests results✅ Tests have passed. Performance tests 1st run outputNo significative change in performance for tests:
If you want to skip performance checks for latter commits, add the |
628dc3b to
7458f02
Compare
|
Automated performance checks have been performed on commit Tests results✅ Tests have passed. Performance tests 1st run outputNo significative change in performance for tests:
If you want to skip performance checks for latter commits, add the |
tests/performance/src/lib.js
Outdated
| } else { | ||
| testNumber = Number(location.hash.substring(1)); | ||
| } | ||
| if (testNumber < 100) { |
There was a problem hiding this comment.
I'm not sure what 100 is, is it the maximum number of tests that can be run?
There was a problem hiding this comment.
Just to give some context:
-
The nodejs script in
tests/perfomance.run.jsis the test runner executed server-side (similar to vitest'sglobalSetup): here it runs one of the test page multiple time on the local Chrome binary -
This script is the test library that will run in the browser alongside written tests (like
vitest's JS lib when we doimport { describe } from "vitest"), it does the client-side test management logic (including sending HTTP requests to the nodejs script to report results) so test files can just be about the test scenarios
This testNumber is the number of time we will reload the page in the current browser. What we actually do here is to switch between the current.html and previous.html pages each time the tests for the current page are all finished.
Because we're reloading the page, we cannot rely on variables declared in that script to track that count. Here we use the "fragment" part of an URL to store the current iteration (.e.g. /current.html#34).
Once we've reached #100, we tell the nodejs script that we're done. It then kills the browser and either re-run another one or finish the tests.
I don't know if that's clear, it's a little complex as we retry the same tests a LOT of time in different processes at different time just to be sure we've some performance issue and it's not just a false positive due to temporary high CPU usage, some browser optimizations when similar code is repeated, or random noise.
There was a problem hiding this comment.
How does it interact with TEST_ITERATIONS = 30 from performance/run.mjs ?
Does it means the browser test is started 30 times and each test does 100 reloads ? So it's actually 30x100 reloads. Isn't it too many?
There was a problem hiding this comment.
Yes that must be the right count (technically 1500 with the previous and 1500 with the new one).
Too many for who?
That's to give more confidence into the final result.
When I looked at other benchmarking projects, 1500 attempts is in the very low tier.
The real issue I can see is that tests may run for 1h but that's not dramatic to me.
There was a problem hiding this comment.
OK let's start with this value and adjust let's adjust it later if we are not satisfied.
Maybe the testNumber can be a parameter of declareTestGroup so it's explicit that the test will be repeated.
declareTestGroup("content loading monothread", () => { .. }, {timeout: 2000, repeat: 100})There was a problem hiding this comment.
In which case we wouldn't repeat some test groups once enough iterations have been done?
This would mean that the context in which other test groups are running may be completely different (e.g. the browser may have optimized some RxPlayer code paths when another test group had run) also, to take in account.
There was a problem hiding this comment.
Test groups should not rely on another test group anyway, the test should succeed if it was launched alone or if there was other test group that has run before
There was a problem hiding this comment.
Yes for the test passing part, but considering that the same page will run all test groups, the browser will e.g. execute TEST GROUP 1 then TEST GROUP 2, and other times just TEST GROUP 2, under that scenario.
The browser might have performed optimizations when JITing that JS and fed cache (even the RxPlayer has e.g. a codec checking cache) meaning that even if TEST GROUP 1 does not explicitly interfere with TEST GROUP 2, the former will have an effect in performance to the second.
In that aspect, running TEST GROUP 1 then TEST GROUP 2 and just running TEST GROUP 2 might not give the same performance results for TEST GROUP 2. Though I don't know what the right approach would be (only one test group per page could be a possibility).
Though most of those optimizations could also be kept as the page is reloaded as the loaded JS didn't change (which the browser can easily check for), so this is a complex subject.
tests/performance/src/lib.js
Outdated
| } else { | ||
| testNumber = Number(location.hash.substring(1)); | ||
| } | ||
| if (testNumber < 100) { |
There was a problem hiding this comment.
How does it interact with TEST_ITERATIONS = 30 from performance/run.mjs ?
Does it means the browser test is started 30 times and each test does 100 reloads ? So it's actually 30x100 reloads. Isn't it too many?
|
Automated performance checks have been performed on commit Tests results✅ Tests have passed. Performance tests 1st run outputNo significative change in performance for tests:
If you want to skip performance checks for latter commits, add the |
This PR does multiple updates with our performance tests (which test for performance regressions):
I simplified performance tests writing by adding a
lib.jsfile handling test management - and I added some multithread tests.I now choose to run performance tests in CI for all PR
Performance tests in CI are now run against the base branch (instead of against the last RxPlayer release )
A comment is left on the GitHub PR if performance issues are detected