-
Notifications
You must be signed in to change notification settings - Fork 48.8k
Add basic release script snapshot test #14280
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
Add basic release script snapshot test #14280
Conversation
Also updated Circle CI build script to generate and add a build-info.json file to artifacts
This commit also temporarily changes an error message so I can verify the updated error codes JSON
scripts/release/test.snapshot
Outdated
@@ -0,0 +1,840 @@ | |||
Index: /Users/bvaughn/Documents/git/react/build/node_modules/create-subscription/cjs/create-subscription.development.js |
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.
Should use relative paths
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.
Shit. That was my way of making sure no one but me changed the scripts. 😉
80448d4
to
0bbdbdd
Compare
To make this token available to the release script, add it to your {path .bash_profile} like so: | ||
|
||
{dimmed # React release script} | ||
export CIRCLE_CI_API_TOKEN=<your-token-here> |
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.
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.
well maybe (your-token-here)
or [your-token-here]
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.
Hm. Ok.
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 did a fresh build before running these scripts, but they were shelved? I think. leading to the errors (inline in review)
- I also noticed was that the docs mention
./scripts/release/<script>.js
, but the cli messages assume you're already in/scripts/release
dir. might want to be consistent there.
To make this token available to the release script, add it to your {path .bash_profile} like so: | ||
|
||
{dimmed # React release script} | ||
export CIRCLE_CI_API_TOKEN=<your-token-here> |
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.
well maybe (your-token-here)
or [your-token-here]
}; | ||
|
||
module.exports = async params => { | ||
return logPromise(run(params), 'Building artifacts', true); |
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.
this takes a looong time. thank you for the spinners (really, at least I'm certain it hasn't 'hung'), but is there a reason the output is hidden? it's been about 5 mins for me now.
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.
is there a reason the output is hidden?
Makes the overall flow of the build script easier to follow.
I guess if you think it's important to show the build script output, the scripts could be tweaked to do this. I don't feel very strongly here.
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 don't feel strongly about this either. I wish I could add priorities to my comments. this is definitely lowest pri.
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 have a "nice to have" entry on the umbrella issue that says:
Store local rolling average for how long the "build" step takes for long steps (e.g. create-canary) and show a progress bar with estimated time remaining.
That seems like maybe the ideal way to do this.
await exec('mkdir build'); | ||
await exec('mkdir build/node_modules'); | ||
await exec( | ||
`cp -r ${tempDirectory}/build/node_modules/*.tgz ${cwd}/build/node_modules/` |
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.
this assumes /build/node_modules exists, ie - you've done a build previously on this machine. I just failed that (working on a fresh checkout). mkdir -p
?
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.
(there's another instance of this, cmd-f cp -r)
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'm not sure what this comment means. Literally the line right above this, I mkdir build && mkdir build/node_modules
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.
Oh! Ha! I forgot to specify cwd
with this command.
|
||
{header Before publishing, please smoke test the packages!} | ||
|
||
1. Open {path ./fixtures/packaging/babel-standalone/dev.html} in the browser. |
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.
this doesn't work. the html assumes the files are in build/dist, but the script generates node_modules.
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.
Hm. At least some of the fixtures pull from build/node_modules
(e.g. the Webpack fixture). I assumed they all did to be honest. Let me wipe out build
and do a fresh pass at this to see what's up myself 😄
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 see the build/dist
references. I'll update them to point into build/node_modules
UMD folders!
|
||
1. Open {path ./fixtures/packaging/babel-standalone/dev.html} in the browser. | ||
2. It should say {quote "Hello world!"} | ||
3. Next go to {path ./fixtures/packaging} and run {command node build-all.js} |
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.
consequently, this fails because dist hasn't been generated
What does this mean? You're not meant to run
Gotcha. I can tweak the output messages. It actually shouldn't make a difference where you run them from since the scripts lookup the repo root relative to themselves, but... fair enough. |
const usage = commandLineUsage([ | ||
{ | ||
content: | ||
'Publishes the current contents of "build/node_modules" to NPM.}', |
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.
'Publishes the current contents of "build/node_modules" to NPM.}', | |
'Publishes the current contents of "build/node_modules" to NPM.', |
5. Go to the repo root and {bold serve -s .} | ||
6. Open {blue.bold http://localhost:5000/fixtures/packaging} | ||
4. Install the "pushstate-server" module ({bold npm install -g pushstate-server}) | ||
5. Go to the repo root and {bold pushstate-server -s .} |
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.
-s
isn't a pushstate-server option though? also, you need a port, so this should probably be pushstate-server . 9000
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.
9000 is the default port.
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.
ah I see, fair. it throws an error with -s
tho.
4. Install the "serve" module ({bold npm install -g serve}) | ||
5. Go to the repo root and {bold serve -s .} | ||
6. Open {blue.bold http://localhost:5000/fixtures/packaging} | ||
4. Install the "pushstate-server" module ({bold npm install -g pushstate-server}) |
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.
have you considered using npx for one off things like this? you could merge steps 4 and 5 with
npx pushstate-server . 9000
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.
Yeah maybe that's a good improvement :)
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'm sorry I dropped comments for the previous PR on this one :(
verified this one too. it, er, ran.
Just to circle back here and close the loop– I created our first canary from the merge commit that landed this branch in master. This was the Circle CI job: https://circleci.com/gh/facebook/react/12774 |
Added regression test for release scripts
Added regression test for release scripts
Issue #14201. Builds on top of #14260 (view the delta).
Adds a simple snapshot test of the release script's "prepare stable" functionality. This test checks out a specific CI artifact and prepares it for stable release. It then compares the diff that results from the version number substitutions to a known good diff and fails if it's changed.
(Note that if we add new packages or change e.g.
packages/shared/ReactVersion
we will want to update the test's CI version and re-generate the snapshot.)This test isn't that helpful but would give us a sanity check after modifying the
prepare-stable
script. I won't be upset if we don't think the test is worth it. 😄Learn more about these new release scripts here.