-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
test(integration/gatsby-cli): use sandboxed directory to "globally" install gatsby-cli #27056
Conversation
Looks good so far. Using a tmp dir is definitely prefered. |
… so it can't use any accidental node_modules
e197fee
to
9c48758
Compare
@@ -1,15 +1,22 @@ | |||
#!/bin/bash | |||
set -e # bail on errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dropped chaining commands with &&
in favour of this so the script fails when any command fails (with exception to if
statement conditions in this case)
check https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html for set
command (and -e
flag in particular) explanation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to showcase that this continue to fail tests properly - https://app.circleci.com/pipelines/github/gatsbyjs/gatsby/50132/workflows/a257b5b7-1e8c-4de1-9854-77c7e530725b is the intentionally not passing jest test ( from temporary draft PR just to test that - #27182 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and reason for dropping chaining is that becomes unreadable with conditional commands - like the chmod that is only done if the file actually exists that I added in this PR)
Gatsby Cloud Build Reportgatsby 🎉 Your build was successful! See the Deploy preview here. Build Details🕐 Build time: 4m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend @wardpeet as a second set of eyes on this. It seems reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
I've tested on windows and it all work |
Relies on #27055
This installs gatsby-cli in some
/tmp
directory and let tests use that installation to invoke global gatsby-cli commands.Those changes are looking to catch problems like #26919 which was not caught by any automatic testing (it worked on CI because executed gatsby-cli was in nested monorepo directory so it had access to all
node_modules
which did hide some problems)This is part of PR series:
feat(gatsby-dev-cli): install deps if there are no gatsby deps but --forceInstall was used
test(integration/gatsby-cli): use sandboxed directory to "globally" install gatsby-cli
(THIS PR)chore(gatsby-recipes): bundle dependencies
chore(gatsby-cli): bundle dependencies