Cypress E2E Tests parallelization #3208
Conversation
…bility of running tests files individually
…roves test run time
vicenteqa
changed the title
There was a problem hiding this comment.
Great job @vicenteqa! I'm very happy we came up with a solution that does not require an invasive refactor on code. To be honest, I wasn't so optimistic on that.
I left some comments on things I think we can improve. Besides that, I think we can merge.
Maybe we want to stage this PR for a while, to verify if tests are consistent after some changes to the codebase? We can rebase a couple of time and see.
test/e2e/cypress/plugins/index.js
Outdated
| async function getPhotofinishBinaryAndGiveExecutablePermissions() { | ||
| const photofinishBinary = await runCommand( | ||
| 'whereis photofinish | cut -d" " -f2' | ||
| ); | ||
| await runCommand(`chmod +x ${photofinishBinary}`); | ||
| return photofinishBinary; | ||
| } | ||
|
|
||
| async function runPhotofinishMainScenario(photofinishBinary) { | ||
| return runCommand( | ||
| `cd ../.. && ${photofinishBinary} run --url "http://localhost:4000/api/collect" healthy-29-node-SAP-cluster` | ||
| ); | ||
| } |
There was a problem hiding this comment.
question: Why can't we use the loadScenario helper here?
Also, why the need to rewrite how we configure the photofinish command? See
web/test/e2e/cypress/support/commands.js
Line 98 in c9c0024
There was a problem hiding this comment.
because when this piece of code is executed, cy commands are still not available, cypress env parameters are not available either at that point of execution. Maybe we could use regular node env variables with process.env 🤔
There was a problem hiding this comment.
process.env
Maybe it's better, we keep the same structure.
Also, if we don't use the loadScenario command anymore, should we remove it?
There was a problem hiding this comment.
well, actually it is still used in some tests, specially to restore scenarios that previous test modified, and to load some other different scenarios.
test/e2e/cypress/plugins/index.js
Outdated
| on('before:run', async () => { | ||
| const photofinishBinary = | ||
| await getPhotofinishBinaryAndGiveExecutablePermissions(); | ||
| await runPhotofinishMainScenario(photofinishBinary); | ||
| }); |
There was a problem hiding this comment.
question: This means the scenario is loaded once per process, right?
So, given the target application is the same, the CI will be executed 8 times while on local execution just once.
There was a problem hiding this comment.
that's it, once per process, so if you execute the whole test suite locally (sequential) this will be executed just once at the very beginning.
| heartbeat_interval | ||
| ); | ||
| heartbeatsIntervals.push(interval); | ||
| sleep(500).then(() => { |
There was a problem hiding this comment.
issue: Do you think we can better understand why it is needed, to remove it?
Sleeps on arbitrary time spans are often cause of flakiness, as they may depend on the running machine
There was a problem hiding this comment.
I'm not a fan of arbitrary waits either and I know they should be avoided as a good practice, for the moment I don't have a conclusive answer tbh but I noticed that with a bit of delay the hosts_overview test flakiness is reduced a lot so I added it as a contingency measure, we can remove it if you want or even we can try the test flakiness tool designed by @gagandeepb then try with and without the arbitrary sleep and compare results.
There was a problem hiding this comment.
I don't have an alternative so I guess I must live with it 🤷♂️
I agree with this approach :) |
…d to be used when needed in each test file
Description
I am creating this PR to check in an isolated workflow changes that I may have to try to achieve Cypress parallelization.As an starting point I copied the development made by @CDimonaco in his PR and took only the E2E part. I want to try this after some changes that I made in the E2E tests.This PR implements Cypress Split plugin using 8 parallel threads.
To optimize test execution, I decoupled each test file, allowing it to run independently without relying on prior tests. This resolved issues where some tests required multiple executions of the "photofinish" main scenario, while others needed it to revert changes from previous tests. By executing "photofinish" once at the beginning of the test suite (either sequentially or in parallel in every CI thread) and removing unnecessary calls, overall test execution time has been reduced too.
PS:
This PR also adds ability to run each test file independently from other files to be run before, so now test files are decoupled.
I also added a small delay in start agent hearbeat function as I've noticed that it reduces flakiness in hosts_overview tests.
Fixes # (issue)
Did you add the right label?
Remember to add the right labels to this PR.
How was this tested?
Describe the tests that have been added/changed for this new behavior.
Did you update the documentation?
Remember to ask yourself if your PR requires changes to the following documentation:
Add a documentation PR or write that no changes are required for the documentation.