Skip to content

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

Merged
merged 26 commits into from
Nov 23, 2018

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Nov 19, 2018

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.

@@ -0,0 +1,840 @@
Index: /Users/bvaughn/Documents/git/react/build/node_modules/create-subscription/cjs/create-subscription.development.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should use relative paths

Copy link
Contributor Author

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. 😉

@bvaughn bvaughn force-pushed the automated-release-scripts-part-5 branch from 80448d4 to 0bbdbdd Compare November 20, 2018 19:34
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>
Copy link
Contributor

Choose a reason for hiding this comment

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

super minor issue 0 this looks odd with ligature fonts (but I can't think of a solution, you can't add a space there iiuc)
screenshot 2018-11-23 at 16 55 55

Copy link
Contributor

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]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. Ok.

Copy link
Contributor

@threepointone threepointone left a 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>
Copy link
Contributor

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);
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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/`
Copy link
Contributor

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?

Copy link
Contributor

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)

Copy link
Contributor Author

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

Copy link
Contributor Author

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.
Copy link
Contributor

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.

Copy link
Contributor Author

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 😄

Copy link
Contributor Author

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}
Copy link
Contributor

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

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 23, 2018

I did a fresh build before running these scripts, but they were shelved? I think. leading to the errors (inline in review)

What does this mean? You're not meant to run yarn build with these scripts. You're meant to either create a canary locally or (preferably) to check one out– as described here.

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.

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.}',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'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 .}
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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})
Copy link
Contributor

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

Copy link
Contributor Author

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 :)

Copy link
Contributor

@threepointone threepointone left a 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.

@bvaughn bvaughn merged commit ed4c4a5 into facebook:master Nov 23, 2018
@bvaughn bvaughn deleted the automated-release-scripts-part-5 branch November 23, 2018 20:53
@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 23, 2018

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

jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
Added regression test for release scripts
n8schloss pushed a commit to n8schloss/react that referenced this pull request Jan 31, 2019
Added regression test for release scripts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants