[Bug]: Watch fails if SL (Steam Locomotive) is installed

[dvanoni]

Version

29.5.0

Steps to reproduce

1. Install SL (Steam Locomotive); e.g. brew install sl
2. Clone my repo https://github.com/dvanoni/jest-watch-sl-bug
3. npm install
4. npx jest --watch

Expected behavior

npx jest --watch runs and prints the following message:

No tests found related to files changed since last commit.
Press `a` to run all tests, or run Jest with `--watchAll`.

Watch Usage
 › Press a to run all tests.
 › Press f to run only failed tests.
 › Press p to filter by a filename regex pattern.
 › Press t to filter by a test name regex pattern.
 › Press q to quit watch mode.
 › Press Enter to trigger a test run.

Actual behavior

npx jest --watch fails with the following message:

Determining test suites to run...

  ● Test suite failed to run

thrown: [Error]

Additional context

#13941 introduced support for Sapling which provides a command named sl. This conflicts with the sl command provided by SL (Steam Locomotive). When you have SL installed on your machine rather than Sapling, jest-changed-files fails to run properly.

Environment

System:
  OS: macOS 13.2.1
  CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
Binaries:
  Node: 18.15.0 - ~/.asdf/installs/nodejs/18.15.0/bin/node
  npm: 9.5.0 - ~/.asdf/plugins/nodejs/shims/npm
npmPackages:
  jest: ^29.5.0 => 29.5.0

[mrazauskas]

Good catch. I was looking at Sapling documentation, might be there is one more related issue on PowerShell:

Note that the name of the Sapling CLI sl.exe conflicts with the sl shell built-in in PowerShell (sl is an alias for Set-Location, which is equivalent to cd).

Reference: https://sapling-scm.com/docs/introduction/installation#windows

Seems to be sensible to check that sl is really Sapling. Perhaps sl --version could be the command for this check? It prints: Sapling [version number].

[dvanoni]

I think that would work. If you run sl --version with SL installed, you'll get the steam locomotive yet again. 😄

[tvararu]

On my system, having sl installed didn't cause the tests to fail to run; rather, determining which tests to run takes a long time now with the --watch command, about 6 seconds. The tests execute instantly when running normally. Removing it via pacman -R sl makes --watch behave as before.

Environment:

$ npx envinfo --preset jest 
  System: 
    OS: Linux 6.2 Arch Linux 
    CPU: (8) x64 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz 
  Binaries: 
    Node: 18.1.0 - ~/.asdf/installs/nodejs/18.1.0/bin/node 
    Yarn: 1.22.19 - ~/.asdf/shims/yarn 
    npm: 9.6.2 - ~/.asdf/plugins/nodejs/shims/npm 
  npmPackages: 
    jest: ^29.5.0 => 29.5.0 

[Nokel81]

Copying from my own issue (which is now closed):

Having steam locomotive installed (on macOS via brew) caused watch runs to fail with a weird error:

  ● Test suite failed to run

thrown: [Error]

after digging around it turned out that the formatting is bad (and probably should also be fixed) but that opaque error is actually the following:

Error
    at ChildProcess.spawn (node:internal/child_process:413:11)
    at Object.spawn (node:child_process:700:9)
    at execa (/Users/nokel81/repos/lens/node_modules/jest-changed-files/node_modules/execa/index.js:83:26)
    at Object.findChangedFiles (/Users/nokel81/repos/lens/node_modules/jest-changed-files/build/sl.js:104:43)
    at /Users/nokel81/repos/lens/node_modules/jest-changed-files/build/index.js:51:17
    at Array.map (<anonymous>)
    at getChangedFilesForRoots (/Users/nokel81/repos/lens/node_modules/jest-changed-files/build/index.js:50:43)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  errno: -63,
  code: 'ENAMETOOLONG',
  syscall: 'spawn',
  originalMessage: 'spawn ENAMETOOLONG',
  shortMessage: 'Command failed with ENAMETOOLONG: sl status -amnu /Users/nokel81/repos/lens/packages/core\n' +
    'spawn ENAMETOOLONG',
  command: 'sl status -amnu /Users/nokel81/repos/lens/packages/core',
  escapedCommand: 'sl status -amnu "/Users/nokel81/repos/lens/packages/core"',
  exitCode: undefined,
  signal: undefined,
  signalDescription: undefined,
  stdout: '',
  stderr: '',
  all: '',
  failed: true,
  timedOut: false,
  isCanceled: false,
  killed: false
}

[rmartine-ias]

I would like to suggest that fixing this issue should include displaying a more descriptive error in this case than the one it does:

  ● Test suite failed to run

thrown: [Error]

[SimenB]

/cc @vegerot

[SimenB]

We might need to revert the sapling support and figure out a better approach. The timeout suggested in https://github.com/facebook/jest/pull/14061 seems icky to me. My own suggestion of grepping man pages or crawling the FS doesn't seem good either.

[vegerot]

We can take a similar approach to ripgrep and walk up the directory tree until we find a .git, .sl, or .hg directory which determines which program to use.

I can come up with ridiculous edge cases. What if someone install both 🚂 and Sapling, but renames Sapling from sl to sap? What do we do then? I guess it would be the same problem if someone renames the git exe 🤷‍♀️ .

I think the best solution would be to detect 🚂 and then skip running sapling.

Hacky proposal: check the size of sl. 🚂 is 34k and sapling is 49M. I doubt 🚂 will ever become more than 1Mb and Sapling will ever be less than 1Mb. Let's skip running sl if sl < 1Mb. We will have to resolve symlinks, etc.

[vegerot]

@SimenB I called in the pros

[mrazauskas]

I would like to suggest that fixing this issue should include displaying a more descriptive error

The conclusion in https://github.com/facebook/sapling/issues/597 is similar, but what should that error say? "Jest detected that sl binary is not Sapling. Please leave sl exclusively for Sapling"?

Many users will not know what Sapling is (I did not know few weeks ago). Also, why Jest needs sl to run the tests? Why a Git user should not be able to have Steam Locomotive installed? Tricky.

This brings an idea of some opt-out mechanism (leave my sl alone, please). Wait... Why not opt-in?! This is how I came to the idea of adding an scm configuration option (see https://github.com/facebook/jest/pull/14061#issuecomment-1500259969).

Just wanted to explain what was the thinking path. Not a fan of adding more and more options.

Obviously the problem should be solved on Jest side. Its main task is to run tests. Having an opt-in scm option there will be no need to spent time guessing which is SCM is used in a repo. That is performance gain for test running. Users like performance and users like Steam Locomotive too (;

PS By the way, if the default is set to 'git', only Mercurial and Sapling users will notice that something changed. Git users will get more performance for free.

[dvanoni]

I'm liking the suggestion here to ship a sapling command that could be used in this scenario instead of sl. It's certainly not the most immediate solution because it would require Sapling users to upgrade to a version that includes it (whenever that happens), but it does feel like the most robust suggestion I've seen.

I think it would also avoid the issue of having to add some sort of scm option to Jest, and I think it also avoids the issue with any error message weirdness.

Of course, this all assumes there isn't already some other tool that provides a sapling command. 😅

[rmartine-ias]

but what should that error say?

I think that it should pass through the existing error, something like 'Command failed with ENAMETOOLONG: sl status -amnu /Users/nokel81/repos/lens/packages/core\n' + 'spawn ENAMETOOLONG',

This case doesn't need a special error, because the underlying issue should be fixed by one of the several proposals (a sapling command, only running sl if the scm configuration option is set to sapling, etc).

Just having that error show up would unblock people, because you can see "something is up with sl".

I'm asking for this also because this is not the only Jest error I've seen that failed with thrown: [Error] (the last ended up being something with watchman) and I'm hoping to get some insight into the next one of those I see, as well.

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[Nokel81]

This is certainly still an issue

[hacker112]

@rmartine-ias I agree with you. If I would have seen that error I would have been able to find this bug report much faster. I only found it because I also started digging in node_modules for answers. Showing the error would help more people finding a faster solution.

I uninstalled sl as my solution, I had even forgotten that I installed it just for fun ages ago.

[vegerot]

I uninstalled sl as my solution

This is the obviously correct solution. The problem is that the error message is so bad people don't realize they need to do this. I think we can check the size of the sl binary to provide a better error message.

@sTRAGER also had another idea to improve the error reporting, but needs to elaborate it more

[rmartine-ias]

This is the obviously correct solution.

Unlinking/uninstalling sl is a correct temporary workaround for this issue. (You could also use direnv to selectively alias it to false when working on jest projects, or wrap it in something that detects if jest is calling it, maybe...)

Homebrew has > 20k people who have installed sl in the past year, debian popcon has > 1% of submitted reports with an install. I think that is more than enough usage to not declare incompatibility. It'd make me sad, too -- there is far too little whimsy on my computer and I do not like the idea that since one tool I must use has mistaken assumptions, I should have to reduce that further.

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[Nokel81]

Still an issue

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[Nokel81]

Not stale, I like the idea of a config that defaults to git

[digitalgopnik]

:+1:

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[Nokel81]

Not stale, would be great if this was a configuration option instead

[meadowsjared]

@rmartine-ias I fully agree that uninstalling is a good temporary workaround. What do you think a more permanent solution to the jest --watch incompatibility with everyone's favorite steam locomotive ❤️?

A config option? or something better possibly?

[rmartine-ias]

@rmartine-ias I fully agree that uninstalling is a good temporary workaround. What do you think a more permanent solution to the jest --watch incompatibility with everyone's favorite steam locomotive ❤️?

A config option? or something better possibly?

I am not familiar with the project's internals, or what tradeoffs the maintainers make, so I don't think I am the person to ask. I am probably missing some context or misreading some code.

That said, I would:

• Fix the error message system
• Only find roots once, when starting the watch process
• Store SCM source at that time on a new Root object
• Find sl roots through directory traversal, instead of binary calling (note: not sure if you'd need to look for sapling-specific files because of the git compatibility)
• AND/OR: Only find sl roots if any of the sapling configuration files exist

I think this would get:

• No errors, because sl isn't called unless a sapling root is found, and/or a sapling config file exists
• No need for additional configuration options, which would break existing workflows when mercurial/sapling users have to add the new option across all machines (including CI)

It would make it so that adding or removing SCM roots while running watch does not work as prior, though. I am sure somebody is doing this. The "only find roots once" thing can be dropped; that is a performance enhancement of questionable value.

[vintlucky777]

gosh I feel super frustrated when the worlds collide and you can't understand why that's happening.

the error message desperately need fixing. luckily, we do know the culprit in this example, but next time it could be some other piece of software causing interference and as-is there's not a single clue to tell the reason and debug.

[lfuelling]

~~Why not use $(npm bin)/sl to call sapling?~~ Ah, because it's installed using brew as well, sorry.

In that case: Why not use $(brew --prefix sapling)/bin/sl to call sapling?

[byronka]

A framework whose purpose is to support quality approaches, with an error message like this ("thrown: [Error]"), with no fix in the 10 months since the bug was reported - it is shameful. I just lost hours to this.

By merely providing a clearer error message this would be greatly remediated, at least as a stopgap measure until a more sophisticated improvement can be made.

[daviesgeek]

I just ran into the same issue; this needs to be fixed. I just spent a bunch of time debugging core jest code to properly have this error be thrown.