Skip to content
This repository was archived by the owner on Mar 14, 2024. It is now read-only.

Task/add integration test#2502

Merged
robdodson merged 5 commits intomasterfrom
task/add-integration-test
Apr 3, 2020
Merged

Task/add integration test#2502
robdodson merged 5 commits intomasterfrom
task/add-integration-test

Conversation

@robdodson
Copy link
Contributor

Changes proposed in this pull request:

  • Begins to add integration tests. This first batch just asserts that key files show up in our build.

Kinda curious what folks think about the npm scripts.

@robdodson robdodson requested a review from samthor as a code owner April 2, 2020 22:40
@googlebot googlebot added the cla: yes Contributor has signed the CLA label Apr 2, 2020
@netlify
Copy link

netlify bot commented Apr 2, 2020

Deploy preview for web-dev-staging ready!

Built with commit 8699d3e

https://deploy-preview-2502--web-dev-staging.netlify.com

@robdodson
Copy link
Contributor Author

I realized that we can just do npm run build because eleventy is what actually generates the algolia.json file. No need to do a prod build since the only real change there is the image urls are different.

@robdodson
Copy link
Contributor Author

I guess the next question is to figure out where this fits in our github actions.

package.json Outdated
"watch:rollup": "chokidar \"src/lib/**/*.{js,scss}\" -c \"npm run rollup\"",
"watch:sass": "chokidar \"src/styles/**/*.scss\" -c \"npm run sass\"",
"watch:test": "TEST_MODE=dev npm-run-all clean build:rollup --parallel watch:rollup karma"
"watch:test": "TEST_MODE=dev npm-run-all clean rollup --parallel watch:rollup karma"
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate to nitpick but this is now "watch:test:unit" rather than integration tests, right?

Copy link
Contributor

@samthor samthor left a comment

Choose a reason for hiding this comment

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

So this is fine for now but I still think the npm commands are a bit confusing.

A semi-serious alternative proposal: we just have one test target, but that itself spawns Karma or (even better) Puppeteer. Internally that test runner might support e.g. "--type=unit" or "--type=integration". Just my 2c.

@robdodson
Copy link
Contributor Author

So this is fine for now but I still think the npm commands are a bit confusing.

yeah I agree. Because the integration tests will probably be a lot slower than the component specific unit tests I wanted to separate them out. But naming leaves a lot to be desired.

A semi-serious alternative proposal: we just have one test target, but that itself spawns Karma or (even better) Puppeteer. Internally that test runner might support e.g. "--type=unit" or "--type=integration". Just my 2c.

hm yeah maybe we could do something like npm run test -- unit which passes the arg unit to a test.js that does an exec based on what was passed in? However, we still have tasks like watch:test or whatever that need to set env vars and run rollup and karma. Not sure if we want to also put those into the script or keep them as npm tasks..?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes Contributor has signed the CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants