Parallelize Tests

[aminya]

Description of the Change

This:

• allows requesting any tests on any operating system
• parallelizes core renderer tests by using separate child processes for each file.
• runs core renderer tests and packages tests in parallel using async.parallel. So if one of the tests goes in timeout, the others will continue to run. This speeds up running the tests by a lot! 🚀
• allows rerunning only the failed tests rather than restarting the whole CI.
• 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.

Verification

The CI passes

Release Notes

N/A

[aminya]

The tests now run half the time (2X faster) and there are fewer timeout issues! It is surprising that no one has noticed this for such a long time!

It is interesting to know that calling commands through child-process is way slower than calling them directly!

[DeeDeeG]

Much faster! I suppose if we posted this to upstream they'd let us know if they still needed it the old way...

Seems good to me. Windows 64-bit runner runs 64-bit Node, Windows 32-bit runner runs 32-bit Node... What's not to like?

[DeeDeeG]

By the way Windows 64-bit tests have been failing (timing out) everywhere. Upstream, this fork, my personal fork.

This commit had CI re-run 11 times at upstream before the tests passed on Windows 64-bit.

I think this PR is fine, and not causing the CI failures.

[DeeDeeG]

We should manually run the "release branch build" pipeline on this and see if it produces working Atom installers on Windows. That would be a good way to validate this, IMO.

Edit: trying here: https://dev.azure.com/DeeDeeG/b/_build/results?buildId=83&view=logs&j=0d2f351d-5899-57e2-0cb5-b37eb91cc930&t=0d2f351d-5899-57e2-0cb5-b37eb91cc930

[aminya]

I saw that 11min is wasted in this for the timeouts caused by some of the tests. In my previous tests, it only took ~30 min to complete the tests.

Do you think we should try dividing the tests similar to MacOs?

[DeeDeeG]

Do you think we should try deciding dividing the tests similar to MacOs?

Sure. if there is a neat way to do it, I don;t see why not. Hopefully much of it can be "copy-pasted" (or reverse-engineered) from the macOS config.

[aminya]

@DeeDeeG Do the cache steps also cache the build atom step?

[DeeDeeG]

@aminya They cache only the Bootstrap.

Caching the build would be trickier, but upstream was interested in it way back in the day, so I shouldn't say it's impossible: atom#19437

In the future, we may want to investigate a couple of different options:
[...]

• Use caching for script/build too. That may still require using a fork of https://github.com/microsoft/azure-pipelines-artifact-caching-tasks, but we're spending a lot of time doing redundant work, such as generating the snapshot or transpiling files.

[aminya]

@aminya They cache only the Bootstrap.

Caching the build would be trickier, but upstream was interested in it way back in the day, so I shouldn't say it's impossible: atom#19437

Currently, the zip files are downloaded and reused. That is similar to caching.

[DeeDeeG]

A lot of the build happens in [repo_root]/out/, so if we can cache some stuff there, maybe that would speed things up even more. Probably have to rewrite some of the script/build code to not run a second time. (like the script/build --no-bootstrap flag, but for some things even later in the process... like the electron snapshot???)

Edit to add:

Currently, the zip files are downloaded and reused. That is similar to caching.

Only for mac though, right? And they wouldn't need to be uploaded/downloaded if the steps weren't split up for parallelization, so the uploading/downloading in and of itself is something slowing things down, and a bit of a compromise vs running straight on in a single container. (Outweighed by the benefit of parallel jobs though.)

In my mind the goal of "more caching" is to do with saving and restoring something that persists from an entirely separate run of CI. From a different commit, or a previous run of the same commit, either way. Not as much splitting up a run for parallelization, though that is another good strategy. And yeah okay that is caching. But it's same-run caching, as opposed to caching from one run to the following fresh one.

[aminya]

A lot of the build happens in [repo_root]/out/, so if we can cache some stuff there, maybe that would speed things up even more. Probably have to rewrite some of the script/build code to not run a second time. (like the script/build --no-bootstrap flag, but for some things even later in the process... like the electron snapshot???)

Speeding up the build is not our priority. There are many things to improve there:
https://github.com/atom/atom/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+build

That needs serious changes to the build script. I don't think caching helps here much. If we can find anything cacheable I would vote for it, but I didn't see anything in the script.

I created a project to track build related improvements: https://github.com/atom-ide-community/atom/projects

[aminya]

@DeeDeeG any idea about these downloading errors in MacOS renderer tests? I am not sure if I have seen them before:

Error Downloading Update: Could not get code signature for running application
Error Downloading Update: Could not get code signature for running application
2020-07-17 02:11:16.279 Atom Dev[43327:67273] Persistent UI failed to open file file:///Users/runner/Library/Saved%20Application%20State/com.github.atom.savedState/window_1.data: No such file or directory (2)
nvm is not compatible with the npm config "prefix" option: currently set to "/usr/local"
Run `npm config delete prefix` or `nvm use --delete-prefix v6.17.1 --silent` to unset it.

They do not affect the passing of the tests, but they appear as warnings.

[DeeDeeG]

Those messages are three separate things.

• Could not get code signature for running application electron/electron#7476
  • We can't auto-update, because we are not code-signed. I'm not sure either why we didn't see this before.
• "savedState"/"window_1.data" seems to be trying to restore open tabs from "last time",
  but it's weird to see that since there is no "last time" in CI. Maybe we are caching something we shouldn't be... But I don't think we are.
• https://stackoverflow.com/questions/34718528/nvm-is-not-compatible-with-the-npm-config-prefix-option the nvm prefix thing is some recurring weird nuisance but not-very-important message with the Node Version Manager for installing Node versions. I don't think it's important. Edit: Technically, I think this exits out nvm with an error code! So, yeah it theoretically could be important. But we should be on "some recent [email protected]+", so the specific version managed by nvm is hopefully not important beyond that.

Best explanation I can give at the moment, since I honestly don't know why we are seeing this just now, but I'll keep it in mind. Maybe it's because we changed the shell we run in, (Powershell), and perhaps that's why different warnings/errors are making their way in front of our eyeballs.

[DeeDeeG]

On another note: If this or any PR is "ready for review" please let me know. Apologies, I was asleep when the big templates PR was "ready to go," so I didn't see the final state of it before it got merged.

[aminya]

This is ready to go. I may change minor things if I see anything.

[DeeDeeG]

Thanks for letting me know!

It happens to be late where I am, so I'm going to have to go to sleep pretty soon...

I remember this being a promising concept, and hopefully not very hard after (and compared to) #46. I would need to look at it tomorrow to really comment on things thoroughly, but I can take a quick look now.

[DeeDeeG]

Suprised that this changes script/test, which is also useful to run outside of CI.

And src/main-process/start.js is also unexpected. Can't look closely right now at the changes, but if it's possible to do this limited to script/vsts/, then that's desirable IMO.

Edit: I see this changes script/test quite a lot. I can only say I hope upstream are interested. We test in a meaningfully different way than upstream by now, so the guarantees that our code works upstream are becoming fewer and fewer as we diverge. Keep that in mind I guess.

I'm not super great at JavaScript. If this is the direction you want to go, it'll take me a while to review it. I can do that tomorrow, and if this isn't merged by then (it would be nice to get a chance to really look this over, IMO) then I'll do my best.

[aminya]

And src/main-process/start.js is also unexpected.

That is because temp package cannot remove the created temp file when we use async.parallel. That is not anything important. It just means some log files will remain when Atom crashes. With that being said I have reported that already in this issue: bruce/node-temp#86

Suprised that this changes script/test, which is also useful to run outside of CI.
Can't look closely right now at the changes, but if it's possible to do this limited to script/vsts/, then that's desirable IMO.

The main test script is script/test. Things in CI are just a simple call to that script. To parallelize the tests I had to change script/test itself. Instead of looking at the Github diff, check the source code itself.

[DeeDeeG]

I was expecting more along the lines of how macOS is split into three test runners. Minimally updated to work for Windows.

I will review the code if you don't merge before I can take a look.

Honestly it is not difficult to run things one-off in a Ci environment closer to upstream's, so if I ever have doubts that tests results are the same with these changes vs upstream's setup I'll do a run and see for myself, just to address my own concerns.

Sadly I cannot review the JS "today" in my time zone, it would have to be in about 8 hours-ish at the earliest. (More likely 10 or 12 hours from now, or a bit later, depending on how complicated the code is.)

[aminya]

I was expecting more along the lines of how macOS is split into three test runners. Minimally updated to work for Windows.

No, they run different tests. MacOS runs packages' test, Windows runs Core Renderer tests.

Sadly I cannot review the JS "today" in my time zone, it would have to be in about 8 hours-ish at the earliest. (More likely 10 or 12 hours from now, or a bit later, depending on how complicated the code is.)

I will keep this until you review it. There is no rush 😄.

[DeeDeeG]

I'm not really qualified to do a deep-dive into the JavaScript. I'm still trying to read it, but what I've seen all looks reasonable. If I understand correctly, script/test wasn't prepared to split up the renderer tests before? So that was needed in script/test in order have the CI run it in a split up way.

I fully endorse that, assuming it was needed (appears to be the case).

If CI passes, I approve.

Comment below is something I think we could suggest at upstream.