Codecov Report

Merging#2459 (b7eb37e) intomaster (e732a48) willdecrease coverage by0.00%.
The diff coverage isn/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

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatee732a48...b7eb37e. Read thecomment docs.

@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 to8.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.

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

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 withatom/atom#21782 which is blocking the Electron upgrade inatom/atom#21777

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

@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.

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

One of thefailing tests:

I wonder if it can be written without usingtest-until

@UziTech Could you update this branch?

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 toincrease 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 linedecreases 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

(WeirdparseInt() 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.

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

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 ondifferent machines. Nothing runs in parallel in one machine.

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

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).

One technically meaningful change that stood out to me was, this makes a symlinkatom pointing toatom.sh and puts those on thePATH. And then runs that asatom [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 onmaster to run viaatom.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 usingatom.sh.

Ohhh you know what? I wonder if those integration tests now rely on havinguser.name anduser.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 theentire build problem.

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 iscapable 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!!!!!

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.

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)

Ohhh you know what? I wonder if those integration tests now rely on havinguser.name anduser.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 theentire 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.

Nice catch@smashwilson . Now to fix windows 🤔

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.

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 thecontinue-on-error: true. Fixing Windows seems too much work now. It should be done in other PRs.

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

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:

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.

Excellent. Thanks everyone 🎉