Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abort sl root call if output resembles a steam locomotive #15053

Merged
merged 5 commits into from
May 12, 2024

Conversation

rmartine-ias
Copy link
Contributor

Summary

Jest detects whether a repository is a sapling repo by calling the sl
binary, and getting the output. If sl (steam locomotive) is installed,
the output of sl root 1) takes forever to get and 2) is not the root,
but a moving image of a steam locomotive. This change monitors the
stdout stream, and aborts the sl call if the first character is an
escape character, which indicates that the terminal is being cleared to
make way for a train to come through.

See also: #14046

Test plan

This was tested manually. I don't think I have the time to figure out
automated testing for this, as it requires installing a binary, which
means mucking around in the CI code.

With the existing main branch, with sl on $PATH:

$ node ~/dev/jest/packages/jest/bin/jest.js --watch
Determining test suites to run...

  ● Test suite failed to run

thrown: [Error]

With the existing main branch, in a repo with sapling roots, with sapling on $PATH:

$ node ~/dev/jest/packages/jest/bin/jest.js --watch

<a bunch of output>

With the proposed changes, with sl on $PATH:

$ node ~/dev/jest/packages/jest/bin/jest.js --watch
No tests found related to files changed since last commit.

With the proposed changes, in a repo with sapling roots, with sapling on $PATH:

$ node ~/dev/jest/packages/jest/bin/jest.js --watch

<a bunch of output>

Jest detects whether a repository is a sapling repo by calling the `sl`
binary, and getting the output. If `sl` (steam locomotive) is installed,
the output of `sl root` 1) takes forever to get and 2) is not the root,
but a moving image of a steam locomotive. This change monitors the
stdout stream, and aborts the `sl` call if the first character is an
escape character, which indicates that the terminal is being cleared to
make way for a train to come through.

See also: jestjs#14046
Copy link

netlify bot commented May 3, 2024

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b06121a
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/663bf4b2dd6caa0008fafa3b
😎 Deploy Preview https://deploy-preview-15053--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@vegerot vegerot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choo-choo-choose to celebrate! 🎊 This bug has been a runaway train 🚆, derailing devs for years since I laid down the tracks for this feature. 🛤️ Let's conduct a thorough inspection 🕵️‍♂️ and write a test to ensure this patch stays on the right track 🛤️ and doesn't cause any more train wrecks. 💥🚈

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is awesome!

Should we set some module scoped variable so we can reuse in findChangedFiles (and also skip the spawn)?

(in addition, please add a changelog entry 🙂 )

@rmartine-ias
Copy link
Contributor Author

Added changelog entry.

Should we set some module scoped variable so we can reuse in findChangedFiles (and also skip the spawn)?

Do you mean like this @SimenB? Not sure what we'd be re-using in sl.findChangedFiles, since I don't think it gets called if we don't find any roots. This new change means we spawn at most 5 sl processes. I could move the logic into index.ts and delay calling sl.getRoot until we know what type of sl is installed, if that's what you mean?

@SimenB
Copy link
Member

SimenB commented May 8, 2024

Should we set some module scoped variable so we can reuse in findChangedFiles (and also skip the spawn)?

Do you mean like this @SimenB?

That, yes 🙂


Not sure what we'd be re-using in sl.findChangedFiles, since I don't think it gets called if we don't find any roots.

Right, that makes sense

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! This has long been a pain point, hopefully this is good enough 🙂

@SimenB SimenB merged commit 7bffeb5 into jestjs:main May 12, 2024
84 checks passed
@SimenB
Copy link
Member

SimenB commented May 12, 2024

Copy link

This pull request 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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants