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 Tests in CI (+ Refactor and Improvements) #21109

Merged
merged 24 commits into from Sep 16, 2020

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Jul 25, 2020

Description of the Change

This:

  • On Windows, runs core renderer tests using two Azure agents. This in addition to double the overall test speed, will allow rerunning the failed ones in case some fail due to timeouts.
  • allows requesting any tests on any operating system
  • parallelizes core renderer tests by using separate child processes for each file.
  • allows rerunning only the failed tests rather than restarting the whole CI.
  • reduces Windows timeouts by running the tests on windows-2019.

Verification

The CI passes

Release Notes

Same as the description

Closes

This supersedes these old pull requests:
#18270

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Aug 5, 2020

We could re-write this PR to not include the heavy use of Azure DevOps templates. Which would make for smaller and more targeted changes, and be easier to review.

Let us know; We'd be very glad to see this improvement land at this repo, so CI can run faster here.

@aminya
Copy link
Contributor Author

aminya commented Aug 12, 2020

We could re-write this PR to not include the heavy use of Azure DevOps templates.

I don't think rewriting this is a good idea. They should merge #21081 before this. Not using templates just makes a soup of code that it is hard to maintain.

@sadick254
Copy link
Contributor

sadick254 commented Sep 14, 2020

@aminya I think this needs rebasing.

@aminya aminya force-pushed the windows_tests_4upstream branch 2 times, most recently from e3a9b64 to e9e6d81 Compare Sep 14, 2020
@aminya aminya force-pushed the windows_tests_4upstream branch from e9e6d81 to 82abb4d Compare Sep 14, 2020
@aminya aminya changed the title Parallelize Tests Parallelize Tests in CI (+ Refactor and Improvements) Sep 14, 2020
@aminya
Copy link
Contributor Author

aminya commented Sep 14, 2020

@sadick254 This is ready to go! 😉

Copy link
Contributor

@sadick254 sadick254 left a comment

@aminya Thank you for your contribution. Looking at this PR 2 questions come to mind.

  • Does this affect the overall speed of running script/test
  • What is the impact of spawning a child_process for every spec file?

Another thing i have noted is, this PR changes the behaviour of running script/test. This PR makes it mandatory to specify the suite one would like to run. Initially script/test would run all the core tests without extra args.

@aminya
Copy link
Contributor Author

aminya commented Sep 15, 2020

@aminya Thank you for your contribution. Looking at this PR 2 questions come to mind.

  • Does this affect the overall speed of running script/test
  • What is the impact of spawning a child_process for every spec file?

Thank you for asking. I haven't noticed any negative impact on performance. The speed bottleneck of running the tests is not in spawning the child_process (which might take less than 1 second), but in running the tests themselves.

Using separate child_process allows us to selectively choose what we want to run, and rerun them if they fail (#21295). Using this I have improved the overall performance and test pass rate.

With that being said there is still room for improvement. If you have noticed we use async.series which is not ideal. I have been mostly concerned with increasing the overall test experience. To maximize the speed of running these, we should apply some other changes (to make the tests thread-safe) and then change this to async.parallel. I can do that after we merged #21295.

Without having these separately, running them in parallel is just impossible.

Another thing i have noted is, this PR changes the behaviour of running script/test. This PR makes it mandatory to specify the suite one would like to run. Initially script/test would run all the core tests without extra args.

Yes, and I think this is a better approach. However, if you don't like this, it has an easy fix. Here instead of throwing an error, we can run all the tests:
https://github.com/atom/atom/pull/21109/files#diff-0e2bc0f03fe54e5e0fff129205e6ee92R293

@aminya aminya force-pushed the windows_tests_4upstream branch from 1d59c60 to a9e690d Compare Sep 16, 2020
This increases the test-pass rate and reduced the timeouts

#109
@aminya aminya force-pushed the windows_tests_4upstream branch from 6af0d12 to 3dbe763 Compare Sep 16, 2020
@sadick254
Copy link
Contributor

sadick254 commented Sep 16, 2020

@aminya Thanks for the clarification.

Yes, and I think this is a better approach. However, if you don't like this, it has an easy fix. Here instead of throwing an error, we can run all the tests

No need. I think this is ok. 👍

@sadick254 sadick254 merged commit b94532e into atom:master Sep 16, 2020
1 check passed
@DeeDeeG DeeDeeG mentioned this pull request Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants