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

test: parallelizing cypress tests to speed up pipeline #2155

Merged
merged 19 commits into from
Oct 20, 2023

Conversation

AtharvChandratre
Copy link
Contributor

Description

This PR enables the Cypress Tests to run in parallel in Github Actions, bringing down the time it took for the Cypress Tests part of the pipeline to run from ~8 min to ~2 min.

Related issue:
#2104

Test Results:

Time taken by non-parallel cypress runs:

Screenshot 2023-09-17 at 10 18 49 PM

Time taken by parallel cypress runs (with this PR's changes):

Screenshot 2023-09-17 at 10 14 49 PM

@netlify
Copy link

netlify bot commented Sep 18, 2023

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit f586d71
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/6532770db6bf4b000803dbb4
😎 Deploy Preview https://deploy-preview-2155--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

test: parallelizing cypress tests to speed up pipeline
@akshatnema
Copy link
Member

@reachaadrika Kindly review this PR

@github-actions
Copy link

github-actions bot commented Sep 22, 2023

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 29
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🔴 PWA 30

Lighthouse ran on https://deploy-preview-2155--asyncapi-website.netlify.app/

Copy link
Member

@akshatnema akshatnema left a comment

Choose a reason for hiding this comment

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

Kindly share the workflow test links you did in fork repository.

package.json Outdated
Comment on lines 99 to 100
"fs": "^0.0.1-security",
"path": "^0.12.7"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"fs": "^0.0.1-security",
"path": "^0.12.7"

These packages are included by default in node js. Kindly don't add it in package.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kindly share the workflow test links you did in fork repository.

@akshatnema - https://github.com/AtharvChandratre/website/actions/runs/6218086013

Copy link
Member

Choose a reason for hiding this comment

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

You haven't pushed your changes 😃

Copy link
Contributor Author

@AtharvChandratre AtharvChandratre Sep 30, 2023

Choose a reason for hiding this comment

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

Yep, just did! Was looking into why build-newsroom-videos.cy.js flakes sometimes. Seems to be because it's calling the YouTube API, which doesn't respond in time intermittently. I can create a separate issue to look into this later.

@akshatnema
Copy link
Member

akshatnema commented Sep 25, 2023

image

Also, I see that each container is taking a separate workflow/jobs inside the checks, can't we run all containers in single job only?

@reachaadrika
Copy link
Contributor

Hi @AtharvChandratre , you can instead of mentioning the entire command in the workflow yml , can also create a test-workflow script in package.json , and mention it , that shall give contributors an ease . Also since ${{ matrix.containers }},is a GitHub Actions-specific syntax for matrix variables we can not run this parallelizing command locally , I propose if we can have this for running tests locally too , what do you suggest @akshatnema

- name: Install dependencies
run: npm install

- name: Cypress Tests are running
run : node ./scripts/index.js && npm run test
uses: nick-fields/retry@v2
Copy link
Member

Choose a reason for hiding this comment

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

how often timeouts happen?
I'm a bit afraid that default rerun for tests, will take over GH infra too much.

also I'm pretty sure cypress have native support for retry and timeout

Copy link
Member

Choose a reason for hiding this comment

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

@AtharvChandratre Yeah, you can try cypress retry and timeout by adding it to the cypress config. Reference - https://docs.cypress.io/guides/guides/test-retries#Configure-Test-Retries

Copy link
Contributor Author

@AtharvChandratre AtharvChandratre Sep 30, 2023

Choose a reason for hiding this comment

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

@derberg thank you for the review. The suggestion makes complete sense. The timeout is in place because, as you pointed out, Cypress might keep running taking over a GH runner for too long. But I looked into the default wait times for various cypress commands (https://docs.cypress.io/guides/core-concepts/introduction-to-cypress#Default-Values) and they seem to be sufficiently large for our use case, while not taking too much GH Runner compute time, so I'm getting rid of that part.

@akshatnema
Copy link
Member

I propose if we can have this for running tests locally too , what do you suggest @akshatnema

Yeah, we should parallelize the tests locally as well, but the problem is, we can't use the same parallel tests implementation for GH actions (as you tried before). Our main concern here is to optimise the time taken by Cypress tests in GH Actions.

@AtharvChandratre
Copy link
Contributor Author

AtharvChandratre commented Sep 30, 2023

Hi @AtharvChandratre , you can instead of mentioning the entire command in the workflow yml , can also create a test-workflow script in package.json , and mention it , that shall give contributors an ease .

@reachaadrika - Thanks for the suggestion, this makes sense. Will make the change.

Edit - I tried to make this change, but in the cypress-tests.yml file it seems that we are unable to call the cypress-parallel.js file to extract all of the cypress spec files we have. This is because we can't run node cypress-parallel.js within a script defined in package.json. To alleviate this, I've kept the npm run test as is and added the spec flag after it within the cypress-tests.yml file

@akshatnema
Copy link
Member

@AtharvChandratre Kindly recheck the workflow file. All the tests are running in single container only.

@AtharvChandratre
Copy link
Contributor Author

AtharvChandratre commented Sep 30, 2023

@AtharvChandratre Kindly recheck the workflow file. All the tests are running in single container only.

@akshatnema - Yep, @VidhiRambhia is doing some research to see if there is a way to make all of the tests to run in a single job. Will let you know if it is possible soon. Thanks!

@VidhiRambhia
Copy link

image

Also, I see that each container is taking a separate workflow/jobs inside the checks, can't we run all containers in single job only?

Hi @akshatnema,

It's a great suggestion and I have been researching this. The matrix strategy approach used for parallelization (https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs) runs the same job but with runs spread across different containers. Because of the logic we have defined in cypress-parallel.js, every run takes care of a different group of tests so that all tests are run in parallel. If we try to run all tests on the same container, we would get a sequential run which is what we are trying to optimize

There is an approach to achieve this more optimally - https://github.com/cypress-io/github-action#parallel. But, unfortunately, this requires a Cypress Cloud account which is paid.

I understand that it might be confusing to see multiple runs of the same job as shown in your screenshot. Please let us know if you have any suggestions to change how workflow jobs are displayed in the check summary

@akshatnema
Copy link
Member

Please let us know if you have any suggestions to change how workflow jobs are displayed in the check summary

I and @derberg discussed this and got that we can have matrix containers and different jobs running in the same workflow. It works for us. For now, I can see that all the tests are running in each job, despite having multiple containers. You can revert to previous changes and they were good and efficient.

@AtharvChandratre
Copy link
Contributor Author

AtharvChandratre commented Oct 9, 2023

You can revert to previous changes and they were good and efficient.

Thank you for your feedback, the necessary changes have been made. The tests are now running in parallel like before.

@akshatnema
Copy link
Member

akshatnema commented Oct 10, 2023

@derberg Do you know why the workflow is still waiting to get executed? It's not getting finished although the changes were pushed last night.

Is it because we changed a workflow which is under Required parameters in repo?

@akshatnema
Copy link
Member

@derberg @AtharvChandratre I'm not linking the issue with this PR because test coverage is still not completed. Apart from that, it's approved from my side ✅

@akshatnema
Copy link
Member

@derberg final review from your side?

@akshatnema
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit 4aa0b0d into asyncapi:master Oct 20, 2023
18 checks passed
@akshatnema akshatnema added the hacktoberfest-accepted PRs which are accepted under Hacktoberfest label Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted PRs which are accepted under Hacktoberfest ready-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants