Skip to content

Conversation

@brendankenny
Copy link
Contributor

another iteration of trying to streamline this thing :)

# Run the tests.
bash ./lighthouse-core/scripts/release/test.sh
# Change into the pristine folder.
# Change into the newly-created pristine folder.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is confusing unless you realize it was just created for you

```sh
# Run the tests again.
# Note: you can skip this if you didn't need to land any changes.
bash ./lighthouse-core/scripts/release/test.sh
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this doesn't really make sense here (if you've just had to land a PR you should be starting over)

yarn test

echo "Running the smoke tests...."
yarn smoke || yarn smoke || yarn smoke
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not really necessary.

  • in theory, master is green. yarn test is a nice sanity check, but running all the smoke tests is overkill because
  • they're about to be run again in the packaged pristine version and
  • it's incredibly annoying to have to run this all over again if master is still green but there's some hiccup further down in this script :)

npm init -y
npm install "$LH_PRISTINE_ROOT/lighthouse-$VERSION.tgz"
npm explore lighthouse -- npm run smoke
npm explore lighthouse -- npm run smoke || npm explore lighthouse -- npm run smoke
Copy link
Contributor Author

Choose a reason for hiding this comment

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

and it's nice to run this twice just in case there's some flake (like I just experienced)

Copy link
Collaborator

Choose a reason for hiding this comment

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

should we make it three like we just removed above? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

wanna lift the bash-ism i used in #9766 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW connor that actually required significant cognitive overhead for me to verify it's doing what we want it to do. when it's only a couple times, I'd greatly prefer || <repeat>

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

all seems good to me!

over to @connorjclark since I believe some of these were your specific additions based on your experiences?

npm init -y
npm install "$LH_PRISTINE_ROOT/lighthouse-$VERSION.tgz"
npm explore lighthouse -- npm run smoke
npm explore lighthouse -- npm run smoke || npm explore lighthouse -- npm run smoke
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we make it three like we just removed above? :)

@connorjclark
Copy link
Collaborator

connorjclark commented Oct 2, 2019

over to @connorjclark since I believe some of these were your specific additions based on your experiences?

lgtm

@connorjclark
Copy link
Collaborator

Can you add somewhere to mark the expected Stable chrome version in the release notes?

@paulirish
Copy link
Member

Can you add somewhere to mark the expected Stable chrome version in the release notes?

already there
image

@paulirish paulirish merged commit 6d56a70 into master Oct 11, 2019
@paulirish paulirish deleted the releasetweaks branch October 11, 2019 03:54
paulirish pushed a commit that referenced this pull request Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants