[TEST] Breaking performance tests #1632
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
peaBerberian
force-pushed
the
misc/do-not-merge-break-perf
branch
from
e0ff5e8 to
b51193a
Compare
peaBerberian
force-pushed
the
misc/do-not-merge-break-perf
branch
3 times, most recently
from
a090fa5 to
c63f694
Compare
peaBerberian
force-pushed
the
misc/do-not-merge-break-perf
branch
from
c63f694 to
ee7a733
Compare
While checking why some test on #1927 didn't pass I realized that this was due to an actual real performance issue in multithread mode in rare scenarios. Basically, the `WorkerMain` module - whose role is to setup everything worker-side in multithreaded mode - gives a hint of what could be expected to be the first Period to load in the content. However, it always gave the first Period - I guess we put a placeholder when first writing the code and forgot to replace it until now. As this is only a hint, this doesn't really led to any important issue, the only visible problem - and why some integration tests failed sometimes - were that the actual module relying on that information took some time (the next playback observation so from 0 to 1 second) before correcting the value. In very specific scenarios, this could mean loading 1 second later than usual (this wasn't really visible on initial load here but it happened in tests for RELOADING cases).
This is an attempt at facilitating the writing of our performance tests, which measure the time taken by key actions and whether we had performance regressions for them (e.g. longer to load a content). The idea is just that I move client-side test management code to a `lib.js` file, and now original test files only contains steps and take the more declarative format we're used with other kind of tests. `lib.js` is then the one running them, sending results, handling errors etc. I also profited from this to add performance tests for multithread content loading, track switching and reloading.
Based on #1629 This PR proposes two things: 1. To run performance tests in CI for all PR 2. Run performance tests against the base branch (and not against the last RxPlayer release)
peaBerberian
force-pushed
the
misc/do-not-merge-break-perf
branch
from
ee7a733 to
1956545
Compare
peaBerberian
force-pushed
the
misc/do-not-merge-break-perf
branch
from
67b2904 to
11151c0
Compare
peaBerberian
added
the
skip-performance-checks
peaBerberian
added
bug
|
Automated performance checks have been performed on commit Tests results❌ Tests have failed. Performance tests 1st run outputWorse performance for tests:
No significative change in performance for tests:
If you want to skip performance checks for latter commits, add the |
.github/workflows/perfs.yml
Outdated
|
|
||
| jobs: | ||
| perf-tests: | ||
| if: ${{ github.event.label.name == 'Performance checks' }} | ||
| if: ${{ !contains(github.event.issue.labels.*.name, 'skip-performance-checks') }} |
There was a problem hiding this comment.
it's github.event.pull_request here
There was a problem hiding this comment.
:O I must have copy-pasted something at some point without realizing
There was a problem hiding this comment.
That was it!!!
|
Automated performance checks have been performed on commit Tests results❌ Tests have failed. Performance tests 1st run outputWorse performance for tests:
No significative change in performance for tests:
If you want to skip performance checks for latter commits, add the |
peaBerberian
removed
bug
|
Automated performance checks have been performed on commit Tests results❌ Tests have failed. Performance tests 1st run outputWorse performance for tests:
No significative change in performance for tests:
Performance tests 2nd run outputWorse performance at second attempt for tests:
No significative change in performance for tests:
If you want to skip performance checks for latter commits, add the |
peaBerberian
added
the
work-in-progress
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is just a dummy PR to showcase what #1630 will do in case of a performance regression, most notably posting a github comment on this PR reporting the issue