Skip to content
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

Parallelize cypress testing #3460

Merged
merged 20 commits into from Oct 18, 2021
Merged

Conversation

revanth0212
Copy link
Contributor

@revanth0212 revanth0212 commented Oct 1, 2021

Description

NOTE: https://github.com/magento-commerce/pwa-studio-cicd/pull/93 should be merged along with this PR

This PR introduces docker parallelization for cypress testing. Before this PR all tests were running synchronously taking around 30 mins for the whole suite. With parallelization, if we use 4 (threads) asynchronous processes internally the time can be bought down to 13-15 mins cutting the time taken by 50%.

3 tests with a single process as before
image
image

3 tests with 2 parallel processes
image
image

That's a 35% increment just by running 3 tests. When I tested all the tests with 4 processes, the process was optimized by 50%.

Related Issue

Closes PWA-2154

Acceptance

Should make cypress testing faster.

Verification Stakeholders

@sharkySharks

Verification Steps

  1. Checkout the branch
  2. cd venia-integration-tests
  3. Run yarn test:headless --url <PR_INSTANCE_URL> -t <NUMBER_OF_CONCURRENT_THREADS>

Screenshots / Screen Captures (if appropriate)

image

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have added translations for new strings, if necessary.
  • I have updated the documentation accordingly, if necessary.

@revanth0212 revanth0212 added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Oct 1, 2021
@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Oct 1, 2021
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Oct 1, 2021

Messages
📖

Associated JIRA tickets: PWA-2154.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against d481cbf

@revanth0212 revanth0212 marked this pull request as ready for review October 1, 2021 03:23
@@ -7,6 +7,28 @@
3. Run `yarn test`
4. Now select test to Run from Cypress UI.

## Run tests in headless mode
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcalcaben can you please verify the docs?

Copy link
Contributor

@jcalcaben jcalcaben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need some minor changes to the docs

venia-integration-tests/README.md Outdated Show resolved Hide resolved
venia-integration-tests/README.md Outdated Show resolved Hide resolved
venia-integration-tests/README.md Outdated Show resolved Hide resolved
venia-integration-tests/README.md Outdated Show resolved Hide resolved
venia-integration-tests/README.md Outdated Show resolved Hide resolved
venia-integration-tests/run-tests.js Outdated Show resolved Hide resolved
venia-integration-tests/run-tests.js Outdated Show resolved Hide resolved
venia-integration-tests/run-tests.js Show resolved Hide resolved
@m2-community-project m2-community-project bot moved this from Changes Requested to Review in Progress in Pull Request Progress Oct 5, 2021
jcalcaben
jcalcaben previously approved these changes Oct 6, 2021
@m2-community-project m2-community-project bot moved this from Review in Progress to Reviewer Approved in Pull Request Progress Oct 6, 2021
@tjwiebell
Copy link
Contributor

@revanth0212 - I'm not sure we should go any further with re-creating the Cypress CLI API in this script, we're not intending to shield the API, we just want to make it easier to run in the container. Tested forwarding with a simple ls wrapper and it seems to behave as expected. Were there some limitations you found where you weren't able to do argument forwarding?

#!/usr/bin/env node
const argv = process.argv.slice(2);
const ls = require('child_process').exec(`ls ${argv}`);
ls.stdout.pipe(process.stdout);

@revanth0212
Copy link
Contributor Author

@revanth0212 - I'm not sure we should go any further with re-creating the Cypress CLI API in this script, we're not intending to shield the API, we just want to make it easier to run in the container. Tested forwarding with a simple ls wrapper and it seems to behave as expected. Were there some limitations you found where you weren't able to do argument forwarding?

#!/usr/bin/env node
const argv = process.argv.slice(2);
const ls = require('child_process').exec(`ls ${argv}`);
ls.stdout.pipe(process.stdout);

@tjwiebell you are right but the reason I had to switch to using command args instead is to have more control over arguments. There isn't a nice way to give optional parallel args which this PR adds. You are right about piping the arguments directly from process.argv but not all options are for the docker command, some go to the runner, for instance -t argv is for the runner file, not the docker command. Instead of reading and manipulating argvs values manually, its easy to use a good renowned package. And we are not bloating the package since this is just internally only package.

What do you think?

@tjwiebell
Copy link
Contributor

What do you think?

Since there is no alternative to run headless outside of docker, my belief is the same. We're removing Cypress API with no way to extend the behavior outside of directly modifying the code (open/closed principle). The threads argument sounds like it could be an environment variable that is set in CI, and we don't need to have two scripts that are identical except for that one arg. Open to a tie break opinion, but I see little reason to continue redefining API when this scripts purpose is to just execute tests given user input.

@@ -5,19 +5,21 @@
"scripts": {
"clearCache:mac": "rm -rf ~/Library/Application\\ Support/Cypress/cy/*",
"test": "cypress open --browser chrome --config-file cypress.config.json",
"test:ci": "./run-tests",
"test:ci": "./run-tests.js -t 4",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels a little static to me, to always set it as 4. different machines will have different capacity based on the system cpu, so I think this should be calculated based on the machine. using os.cpus you can get information about the machine and then based on that run threads.

Also, it seems like maybe we always want to to run threads and maybe two commands isn't necessary?

@m2-community-project m2-community-project bot moved this from Reviewer Approved to Review in Progress in Pull Request Progress Oct 15, 2021
Copy link
Contributor

@sharkySharks sharkySharks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so another approach to parallelizing these tests is to instead of work with threads inside of the docker container you can create a load balancer orchestrator for the tests, the codebuild job that gets executed starts the orchestrator and then it kicks off codebuild jobs with specific specs (splitting each test or group of tests into a separate downstream codebuild job). the orchestrator can then wait for the jobs to complete and report back passing or failing results. There would need to be an extra step probably to aggregate the results of the parallel tests for debugging purposes, but that could be figured out. This would then split the load across multiple machines and really speed up the process.

Or there could be a combination of both methods and split tests into groups across machines and then within those groups run different threads of each test within the group. Here is an issue reference for splitting across machines with the --spec flag.

This can also be an iterative approach, my comments don't necessary negate the work you have done here 🙂

Copy link
Contributor

@sharkySharks sharkySharks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all change requests can be made iteratively if this start to parallelization needs more improvements.

@m2-community-project m2-community-project bot moved this from Review in Progress to Reviewer Approved in Pull Request Progress Oct 18, 2021
@sharkySharks sharkySharks merged commit f52c297 into develop Oct 18, 2021
@sharkySharks sharkySharks deleted the revanth/cypress-parallelization branch October 18, 2021 19:00
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Progress: done version: Minor This changeset includes functionality added in a backwards compatible manner.
Development

Successfully merging this pull request may close these issues.

None yet

5 participants