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

build: parallelize slow tests #3536

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

DevanceJ
Copy link
Contributor

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:
Fixes #3255
After referring to https://circleci.com/docs/parallelism-faster-jobs/ I increased parallel execution environments to 4 and also set timings type to classname

@DevanceJ DevanceJ requested a review from a team as a code owner March 16, 2024 20:03
@DevanceJ DevanceJ mentioned this pull request Mar 21, 2024
5 tasks
@yangannyx
Copy link
Member

yangannyx commented Apr 23, 2024

This is super cool! Awesome seeing the four buckets in the CI run here.

What exactly are classnames referring to in a typical forge test? I'm curious because I see that the time taken between the 4 buckets seems rather uneven, and I'm concerned Circle CI isn't optimizing from past runs properly

@DevanceJ
Copy link
Contributor Author

The documentation says that --split-by=timings should out of the box split the tests evenly across all the executors but when I tested it on my personal circleci setup for my forked repository I noticed the tests weren't split evenly, that's why I was trying to use --timings-type to maybe get it working appropriately. I'll go through the docs more thoroughly and do some digging around this and let you know what I find.

@yangannyx
Copy link
Member

Yeah, reading the docs I agree we should use --split-by=timings and --timings-type. I'm wondering if Circle isn't able to identify classnames on our tests, which is leading to the uneven distribution. Have you tried setting --timings-type to name or file and seeing if that makes things more even?

@DevanceJ
Copy link
Contributor Author

Hi sorry for the delay. Looking into this now. --timings-type=file is default behaviour which I think we should use for our tests.

@DevanceJ
Copy link
Contributor Author

Currently we only use parallelism for slow-tests which is fine but slow-tests-x64-node/windows take up the maximum time, should I try using 8 nodes.

@yangannyx
Copy link
Member

Hm, I don't think file is a great choice to split tests on either - we only have 6 slow test files to begin with, one which only runs on Linux. I also suspect that the tests that take the longest are located within the same files and not across files (see the electron-forge API tests in api_spec_slow.ts).

That being said, the goal here is to get an even distribution of tests by duration across each parallel bucket. Looking at the most recent CI run on this PR, I'm still seeing one bucket that takes the most time. I would maybe try measuring how long tests within the *_spec_slow.ts files are taking and seeing if --timings-type=name produces a more even distribution

@DevanceJ
Copy link
Contributor Author

Hi so I looked into the tests more deeply and learned that the Windows slow tests are taking most of the time to complete the installation of node modules and to configure wixtoolset, and for some reason CircleCi is not able to split the slow tests the way it should.

Yes you are right using --timings-type=name could be a possible solution to the optimisation of slow tests.

@DevanceJ
Copy link
Contributor Author

Looks like this documentation is not appropriate for our configuration because according to the current test failure and this.
Available values for --timings-type are filename, classname, testname, autodetect
I think using testname could be appropriate.

@erickzhao erickzhao marked this pull request as draft May 14, 2024 17:48
@erickzhao
Copy link
Member

Going to put this into draft mode until #3585 is resolved

@yangannyx
Copy link
Member

This PR should be unblocked by #3620 @DevanceJ! Apologies for the delay 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parallelize slow tests in CircleCI
4 participants