use action-setup-atom

[UziTech]

Description of the Change

use UziTech/action-setup-atom@v1 for installing Atom in GitHub Actions

Screenshot/Gif

N/A

Alternate Designs

N/A

Benefits

Cleaner workflow files and works on any os and channel easily

Possible Drawbacks

none

Applicable Issues

#2298 (comment)

Metrics

N/A

Tests

Ran tests on every platform

Documentation

N/A

Release Notes

N/A

User Experience Research (Optional)

[codecov]

Codecov Report

Merging #2459 (b7eb37e) into master (e732a48) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2459      +/-   ##
==========================================
- Coverage   93.46%   93.45%   -0.01%     
==========================================
  Files         237      237              
  Lines       13234    13234              
  Branches     1906     1906              
==========================================
- Hits        12369    12368       -1     
- Misses        865      866       +1     

Impacted Files	Coverage Δ
lib/atom/gutter.js	90.47% <0.00%> (-2.39%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e732a48...b7eb37e. Read the comment docs.

[UziTech]

@smashwilson I'm sure this will need some changes but it is a good start for using the GitHub Action for installing Atom.

When I was testing it out it looks like the windows tests were just failing because paths were getting converted to 8.3 file paths

  48) WorkdirCache
       clears the cache when the maximum size is exceeded:

      assert.strictEqual(actualDir, expectedDir)
      + expected - actual

      -C:\Users\runneradmin\AppData\Local\Temp\git-fixture-202046-3340-4cwu10.vgyo3
      +C:\Users\RUNNER~1\AppData\Local\Temp\git-fixture-202046-3340-4cwu10.vgyo3
      
      at Context.<anonymous> (D:\a\github\github/workdir-cache.test.js:143:12)

I can't seem to find the best way to handle that in node without a simple .replace in the tests.

If you want me to close this PR and you want to take this and work on it in your own PR I would be fine with that.

[aminya]

@smashwilson Could you take a look at this? Having better CI helps to debug the issues like this. setup-atom allows for installing stable, beta, and nightly.

[aminya]

Suggested change

	# os: [ubuntu-18.04, macos-10.14, windows-2016]
	os: [ubuntu-18.04, macos-10.14]
	os: [ubuntu-18.04, macos-10.14, windows-latest]

[UziTech]

The tests on windows are failing because of #2459 (comment)

[aminya]

Still, we should run them to observe if a new test starts to fail. We should continue to on error to not block the CI. This is much better than not running them.

      - name: Run the tests
        if: ${{ !contains(matrix.os, 'windows') }}
        run: atom --test spec

      - name: Run the tests on Windows 
        if: ${{ contains(matrix.os, 'windows') }}
        continue-on-error: true # due to https://github.com/atom/github/pull/2459#issuecomment-624725972
        run: atom --test spec

[UziTech]

I think a better solution would be to figure out how to fix the 8.3 path names in the tests and run them normally on windows. But as I said above, unless @smashwilson actually wants to merge this PR, it is just a POC.

[UziTech]

It should be pretty easy to create a function that can compare regular paths with FAT 8.3 paths in tests.

[aminya]

Definitely, in the long run, we can fix the test. The error, although fails the test, is just a path comparison. So for now, it is better to focus on the other failing tests in MacOS and Ubuntu which are blocking the Electron upgrade.

[smashwilson]

unless @smashwilson actually wants to merge this PR, it is just a POC.

I'm 👍 on merging this if one of you has time to get it green!

And I'm 👍🏻 on including the Windows builds until we can fix that crash.

[aminya]

I made a PR making this PR ready to go: UziTech#9. @UziTech Please merge it.

Using this PR, we can find the reason that the tests fail in with atom/atom#21782 which is blocking the Electron upgrade in atom/atom#21777

[aminya]

The failing tests are genuine failures. See here:
atom/atom#21782 (comment)

Maybe, we should increase the timeout to see if the errors go away

[UziTech]

@aminya I updated this PR with the code from your PR on my repo.

Looks like the tests are failing because of a timeout error. I'm not sure if that is something to do with this PR or not.

[aminya]

I don't think it is because of this PR (as mentioned in here)

One of the failing tests:

github/test/integration/file-patch.test.js

Line 241 in 5a8faf5

it('may be completed staged', async function() {

It fails here exactly:

github/test/integration/file-patch.test.js

Line 67 in 5a8faf5

await until(() => {

I wonder if it can be written without using test-until

[aminya]

@UziTech Could you update this branch?

[DeeDeeG]

About the CI failures here:

@smashwilson has indicated multiple times that these tests (integration: file-system patching) cause frequent headaches, out of proportion to their use for catching actual problems.

#2617 (comment)

They are notorious for flaking out and he has strongly considered deleting them.

You can also observe that these tests have been patched in multiple ways to make their timeouts more lenient. They rely on actual filesystem access, which is very unreliable, particularly in CI where you are testing in some VM somewhere on a shared datacenter host machine.

Take a look at these two separate workarounds that already exist for these flaky tests:

https://github.com/atom/github/blob/v0.36.6/test/integration/file-patch.test.js#L18-L23

That said, I believe there is a solution.

The following line is clearly meant to increase timeout length to 9 seconds: https://github.com/atom/github/blob/v0.36.6/test/integration/file-patch.test.js#L22

However, upon closer inspection, I believe this line decreases the timeout length. There is already a longer global timeout (30 seconds) set in the CI's env vars:

https://github.com/atom/github/blob/v0.36.6/.github/workflows/ci.yml#L22

Here is my patch to fix this unintentionally restrictive and short timeout:

DeeDeeG@e0bc5db

(Weird parseInt() stuff in the patch is copied to match similar stuff already in the repo, specifically this: https://github.com/atom/github/blob/v0.36.6/test/runner.js#L14)

I believe with my patch (see link just above), this PR would pass CI.

[DeeDeeG]

I'm considering the possibility that this is failing simply because of how many jobs there are. 6 parallel matrix jobs, starting in parallel with a lint and snapshot test job (8 jobs in the very beginning).

(Maybe the CI environment becomes super disk-bound -- on a tangent: note also that Atom's CI as of late has become massively parallel, and is at times very disk-heavy in each job).

I'd like to try it with max parallel jobs of 2 and work up from there if it passes.

https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstrategymax-parallel

[aminya]

I'm considering the possibility that this is failing simply because of how many jobs there are.

That is unrelated. The failures are genuine failures that should be fixed.

These matrix jobs run on different machines. Nothing runs in parallel in one machine.

[DeeDeeG]

This PR is passing if rebased right on top of v0.36.3 tag. So the regression in the integration: file-patching tests in certain circumstances is real.

[UziTech]

This PR only changes the way Atom and apm are downloaded and installed for the tests. It doesn't change the tests or code in any way. It basically takes the download scripts and puts them in an action. The only real difference is which folder Atom and apm are installed in (which I don't believe affects the tests).

[DeeDeeG]

One technically meaningful change that stood out to me was, this makes a symlink atom pointing to atom.sh and puts those on the PATH. And then runs that as atom [args]. Whereas before, it was the actual executable binary (for example: Atom*.app/Contents/MacOS/Atom*) being called directly on the macOS and Ubuntu CI runs.

So I wonder if changing the CI on master to run via atom.sh would cause the same timeouts? I intend to try that when I get a free moment.

I seem to have gotten this backward. CI here was already using atom.sh.

[smashwilson]

Ohhh you know what? I wonder if those integration tests now rely on having user.name and user.email populated in the system's git config, or the git identity pane is shown instead of the staging view. I bet the existing build configuration sets them, but this build does not.

.... I'm going to be upset if that's the entire build problem.

[DeeDeeG]

Upset + bugfix identified is an emotional rollercoaster, which I have been on many times as well, but it ends up fixing the project, so it's the way it's gotta be sometimes.

I definitely think the code is capable of working correctly but isn't for some very superficial cause that no-one would immediately think of. So that ^ sounds believable.

Edit: Ubuntu is green!! Edit Edit: macOS nightly is green!!!!!

[smashwilson]

Oh yeah, this is a feeling I'm well acquainted with 😂

The better solution would be to rework the integration tests somehow to not rely on global git configuration settings (if we do end up keeping them). But let's see if this makes the build green, first.

[DeeDeeG]

Not my fix to steal the thunder on, so thank you @aminya @UziTech and @smashwilson for sticking with this, but:

Linux/macOS are green!!!

(And Windows has been green the whole time, I think? So "knock on wood" but yeah I expect this will pass)

[aminya]

Ohhh you know what? I wonder if those integration tests now rely on having user.name and user.email populated in the system's git config, or the git identity pane is shown instead of the staging view. I bet the existing build configuration sets them, but this build does not.

.... I'm going to be upset if that's the entire build problem.

I knew that the issue is an assumption about the environment that does not hold true 😁. Happy to see that the assumption is finally identified now.

[UziTech]

Nice catch @smashwilson . Now to fix windows 🤔

[DeeDeeG]

I just meant to emphasize macOS/Linux, as Windows has been green all along. There must be a git config loaded into the runner or something on Windows.

[aminya]

I just meant to emphasize those as Windows has been green all along. There must be a git config loaded into the runner or something on Windows.

That's because of the continue-on-error: true. Fixing Windows seems too much work now. It should be done in other PRs.

[DeeDeeG]

Oh, right. Well that's a whole other conundrum then. But way beyond scope of this PR I think.

[aminya]

When I use continue-on-error, I write the number of test failures inside the CI file to later check if the failures are increased. If the changes add new test failures that means that there is an issue with that change.

The stats on the current Windows run:

[DeeDeeG]

I am seeing that there are some "expected tests" for the PR that (look like they) are never going to run. The runs for the latest commit are all green.

[smashwilson]

Excellent. Thanks everyone 🎉