Failing CI on Windows

[DanielRuf]

It seems some of the tests fail on Windows:
https://ci.appveyor.com/project/Daniel15/jest/builds/21990233

[thymikee]

We run our tests on Azure now

[DanielRuf]

Sure but this is still shown in the CI status (last commit) and the Azure Windows build was canceled.

[DanielRuf]

See https://github.com/facebook/jest/commits/master and the red CI status for all commits.

[SimenB]

There's some sort of issue with detectOpenHandles hanging. I haven't dug into it, but I'm suspecting some edge case due to an upgrade of node somewhere

[thymikee]

Yea, we need to figure out the failures of these 2 tests (interesting that they're different on AppVeyor vs Azure). Would you be able to help diagnose this?

[SimenB]

CI just passed: https://dev.azure.com/jestjs/jest/_build/results?buildId=452

There's been a couple of node releases, I wonder if one of them fixed whatever the issue was?

[SimenB]

Hmm, no. 10.15.1 has been used for both the hanging and the passing builds.

@kaylangan any idea about what might have changed? The previous 2 builds passed after the windows build always hung for some time: https://dev.azure.com/jestjs/jest/_build?definitionId=1&_a=summary

[kaylangan]

@SimenB I've been looking into this and haven't figured it out yet. I can't repro the failures locally and see that there are occasional passes. While we investigate, I suggest we set the timeout for the job to 35 minutes (maybe 30 if we wanted to be more aggressive) so that we don't unnecessarily tie up one of your 10 pipelines and results get sent back faster. I can submit a PR if that sounds ok with you.

[willsmythe]

I think this is caused by the upgrade of Yarn (1.9.4 to 1.13.0) on the hosted Windows image. If you look at the build history, the last successful build used 1.9.4, and every build after (which fails due to a timeout) has been on 1.13.0.

Without going into all the details right now (it's late), the until method in e2e/runJest.js which uses execa to spawn Jest, was previously spawning "node.exe" directly, but now spawns a shell (cmd.exe) that wraps node.cmd (yah, node.cmd). This changes the behavior when kill() is called on the jestPromise (the cmd.exe shell gets killed, but the node.cmd/node process hangs around).

Prior to Yarn 1.13.0, execa/cross-spawn would read the #! at the top of jest.js (the file being spawning) and use "which" to discover the .exe for node and then spawn it (see cross-spawn/../parse.js#L41). Yarn 1.13.0 injects what appears to be a temporary path at the start of $PATH that contains a "node.cmd". When cross-spawn calls "which", it now find this node.cmd, which cannot be spawned directly (it must must be wrapped in cmd.exe).

One option is to downgrade Yarn to 1.9.4 on Windows:

  - job: Windows
    pool:
      vmImage: vs2017-win2016
    steps:
      - script: |
          git config --global core.autocrlf false
          git config --global core.symlinks true
        displayName: 'Preserve LF endings and symbolic links on check out'

==>   - script: npm install -g yarn@1.9.4   
          
      - template: .azure-pipelines-steps.yml

This successfully builds the latest commit from master, see: https://dev.azure.com/willsmythe/jest/_build/results?buildId=486

It would be good to explore why Yarn is creating its own node.cmd and putting it at the front of $PATH.

[DanielRuf]

Was yarn installed with or without node? Did not check it yet.

[DanielRuf]

Related: yarnpkg/yarn#3255

[SimenB]

Oh wow, that must have been hard to track down. Thanks!

@Daniel15 @arcanis something you've encountered?

[arcanis]

It would be good to explore why Yarn is creating its own node.cmd and putting it at the front of $PATH.

This is so that scripts are guaranteed to be executed with the exact same Node as the one running Yarn itself. Previously we were using whatever was the global one, so there might be weird mismatches

This changes the behavior when kill() is called on the jestPromise (the cmd.exe shell gets killed, but the node.cmd/node process hangs around).

.This looks like a bug - I'm not familiar with Windows, any idea of what we should change to properly kill the wrapper when cmd gets killed?

[SimenB]

Gonna add the workaround of downgrading yarn so we can have green CI again. Will let this remain open though so we don't lose track of it.

(potentially put together a small reproduction using yarn + execa and opening an issue in either of their repos?)

[willsmythe]

@arcanis - instead of Yarn generating a node.cmd and injecting its path at the front of $PATH, could Yarn have just injected the path of the current node process (path.dirname(process.execPath)) instead?

Regarding your question about kill() "not working as expected" on Windows, there are lots of threads, blog posts, and libraries out there trying to solve this problem. The most relevant here (since Jest depends on execa which depends on cross-spawn) is kentcdodds/cross-spawn-with-kill. The creator of cross-spawn-with-kill tried to get cross-spawn to address the problems with kill() , but was rejected, and so created cross-spawn-with-kill. See moxystudio/node-cross-spawn#40.

If this change had been accepted into cross-spawn in 2016, we wouldn't be talking about this right now ;)

Here is a simpler test/example that demonstrates the problem in general: https://github.com/willsmythe/orphaned-node-process-test

Here are a few thoughts on how to solve the current problem in Jest ...

1. Update the call to execa() in e2/runJest.js to spawn the current Node process, passing JEST_PATH as the first argument (as opposed to spawning JEST_PATH directly; this avoids the lookup of Node in the path, etc).

2. Add the current Node process's path to the front of the PATH environment variable passed to execa() in runJest.js.

3. Implement a custom "kill" function (like what cross-spawn-with-kill does) in e2e/runJest.js

4. Fix Yarn to not generate a node.cmd or inject it at the start of the PATH environment variable (or to inject the path to the current Node process at the front of PATH)

5. Work with execa or cross-spawn to fix the kill() function

I am going to propose a PR to address this using option 1. I think it's the least risky and avoids the parsing logic that currently reads the shebang at the top of jest.js and avoids the which logic in cross-spawn to find the appropriate node to run.

[SimenB]

Fixed in #7838, I'd say 🙂 It'd be nice to do 5., but I don't think it's worth the effort from our side.

Thank you so much for the help @willsmythe!

[DanielRuf]

Awesome 👍

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.