-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
tests: ci tests should include all files #13235
Conversation
@@ -51,7 +51,7 @@ | |||
"unit-viewer": "yarn jest \"lighthouse-viewer/.*-test.js\"", | |||
"unit-flow": "yarn jest \"flow-report/.*-test.[tj]s[x]?\"", | |||
"unit": "yarn jest", | |||
"unit:ci": "NODE_OPTIONS=--max-old-space-size=8192 npm run jest -- --ci", | |||
"unit:ci": "NODE_OPTIONS=--max-old-space-size=8192 npm run jest --ci .", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i SO wish this dot didn't have any material effect, but it appears to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, that's from #13148. Interesting that you need the extra --
for npm run [script]
but not for npm run [bin-thing]
well...it makes a certain amount of sense but it shouldn't have to :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, that's from #13148. Interesting that you need the extra
--
fornpm run [script]
but not fornpm run [bin-thing]
well...it makes a certain amount of sense but it shouldn't have to :)
to be completely honest i didn't verify the --
change is good. (or that --ci
still works as expected.) it seems possible i regressed this.
i know yarn XX
doesnt need extra --
, but don't know npm run as well.
@brendankenny do you think we should change this back? also.. to what? npm run jest -- --ci .
?
(probably just a note for future us: when i just added (flow-)report into c8.sh, i only got coverage for report-generator. the report renderer stuff remained uncovered.. that was only fixed with the So that -0.41% doesn't look significant, but it is. here's the "files" view of the two relevant commits on master: 359 files vs 400 files. so yeah.. verified that these changes add back in coverage for report and flow-report. :) |
over in #13125 i'm getting codecov failures and it appears our coverage isn't picking up tests in report, etc.
This appears to sort it out.