Skip to content
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

chore: Add tests for webpack.config.js module exports #2181

Merged
merged 12 commits into from
Feb 5, 2018

Conversation

acdvorak
Copy link
Contributor

@acdvorak acdvorak commented Feb 3, 2018

  • Adds an npm run test:buildconfig command, and calls it in npm test
  • Replaces ; with && in package.json (Windows cmd.exe does not support ;)

To test this PR, simply change a single character in a test/build/goldens/*.json file.
Then run npm run test:buildconfig in the console.

This PR is a prerequisite for #1994.

Adds `npm run test:buildconfig` command, and runs it during `npm test`.

Replaces `;` with `&&` in `package.json` (Windows `cmd.exe` does not support `;`).

Prerequisite for #1994
@codecov-io
Copy link

codecov-io commented Feb 3, 2018

Codecov Report

Merging #2181 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2181   +/-   ##
=======================================
  Coverage   99.14%   99.14%           
=======================================
  Files          90       90           
  Lines        3843     3843           
  Branches      497      497           
=======================================
  Hits         3810     3810           
  Misses         33       33

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4fdb5c...d2f3b2b. Read the comment docs.

@lynnmercier lynnmercier self-requested a review February 5, 2018 18:59
@lynnmercier lynnmercier self-assigned this Feb 5, 2018
Copy link
Contributor

@lynnmercier lynnmercier left a comment

Choose a reason for hiding this comment

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

You'll have to sync and update the golden...because we published two new node modules

package.json Outdated
@@ -19,12 +19,13 @@
"lint": "npm-run-all --parallel lint:*",
"postinstall": "lerna bootstrap",
"pretest": "npm run lint && node scripts/check-imports.js",
"test": "npm run test:unit && npm run test:closure && npm run test:dependency && npm run build; npm run clean",
"test": "npm run test:buildconfig && npm run test:unit && npm run test:closure && npm run test:dependency && npm run build && npm run clean",
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to the end of the tests. Most people just run Unit tests and then force quit because they don't want to wait (at least thats what happens to me)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test:buildconfig takes less than 1 second to run.

I originally had it at the end, but I got tired of waiting for test:unit to finish 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

People shouldn't be force-quitting after running unit tests though? At least, not if they want to make sure that everything actually works... (And if they really only want to run unit tests... npm run test:unit)

There might actually be solid argument for putting the fast-running things (e.g. test:buildconfig and build) first since if we manage to do something silly that fails fast, it'd be better for Travis CI to get it over with quick and move on to other jobs. We can do that separately from this PR though.

@acdvorak acdvorak merged commit 874e24f into master Feb 5, 2018
@acdvorak acdvorak deleted the chore/test/webpack-config-exports branch February 5, 2018 20:03
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.

4 participants