Skip to content

fix(publish): Change prepare to prepublishOnly #138

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 4 commits into from
Jul 3, 2018

Conversation

spencerwilson-optimizely
Copy link
Contributor

@spencerwilson-optimizely spencerwilson-optimizely commented Jun 30, 2018

Summary

  • Convert prepare hook to a (not-equivalent) prepublishOnly, reducing the number of times unit tests are run in prepare-literate builds by 1.

#129 introduced a regression where unit tests stopped being run for Node.js v4 and v6. Prior to that PR, unit tests were run

  • v4 and v6: once
  • v8, v9, v10, ... (versions whose npm understands the prepare hook): twice

After that PR was merged, the number of times unit tests were run in each build went down by 1, for every build. That means v4 and v6 stopped running unit tests altogether.

2faa9c4 fixed the regression by unconditionally running unit tests on all matrix-based builds (and explicitly adding a build to run xbrowser tests). With this, Node.js 8+ builds resumed running unit tests twice (example).

Regarding the prepare hook: npm@4 added the prepare hook with that fires on prepublish and postinstall. This is strange. Why run tests when someone installs the package? They see the mocha output in their shell, which is likely to surprise.

This PR stops the unconventional test runs on postinstall.

Test plan

CI.

@coveralls
Copy link

coveralls commented Jun 30, 2018

Coverage Status

Coverage decreased (-0.1%) to 97.11% when pulling 847726e on sw/postinstall into 2faa9c4 on master.

@tylerbrandt
Copy link
Contributor

tylerbrandt commented Jul 2, 2018

Hm I'm not sure what happened here; at the time that I added the "prepare" hook I didn't remove anything; I was just attempting to codify in code what we currently require from users to deploy. So the regression must have come later (or earlier).

@spencerwilson-optimizely
Copy link
Contributor Author

@tylerbrandt I think this was a regression due to #129. Before that PR, we unconditionally ran unit tests once, and we ran them twice in the case of npms which understand prepare.

@spencerwilson-optimizely spencerwilson-optimizely changed the title fix(ci): Run unit tests on every build fix(publish): Change prepare to prepublishOnly Jul 3, 2018
Copy link
Contributor

@tylerbrandt tylerbrandt left a comment

Choose a reason for hiding this comment

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

Ok, sounds good. Can you update the summary to remove the misleading stuff?

@spencerwilson-optimizely
Copy link
Contributor Author

Was in the process of doing that, you're just darn fast at reviewing, haha.

@spencerwilson-optimizely spencerwilson-optimizely merged commit 958c0b6 into master Jul 3, 2018
@spencerwilson-optimizely spencerwilson-optimizely deleted the sw/postinstall branch July 3, 2018 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants