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

tests: fix coverage generation #7475

Merged
merged 3 commits into from
Mar 12, 2019
Merged

tests: fix coverage generation #7475

merged 3 commits into from
Mar 12, 2019

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Mar 12, 2019

here's my take on fixing coverage :)

This seems to be the minimal fix for what appears to be two separate problems:

  1. missing a bunch of stuff on Travis. --hook-run-in-context appears to be the key here, so I assume something changed between how jest and/or yarn create subprocesses and/or how nyc hooks into them
  2. zero coverage when run on my machine. No one else on the LH team seems to be seeing this, but also seems to be an interaction between latest node/jest/yarn/nyc (see discussion in Coverage 0% istanbuljs/nyc#921) and process exit hook order. Fixed by changing the inner calls of yarn coverage and yarn unit:ci from yarn to npm run.

I also got rid of unit-cli:ci and unit-core:ci, since these were only called within npm scripts, and we can simplify by having yarn unit:ci pass in those extra arguments instead.

(my original approach was to stop using nyc to generate coverage and just use jest for that directly. We could still do this, but it turns out merging multiple jest coverage reports with existing lcov tools is a pain, so I vote leaving that for another day since we still need nyc for smoke coverage anyways)


(comment from below)

added another commit

  1. renamed unit:silentcoverage to unit:cicoverage (and smoke:cicoverage) as that's what it really means (making an lcov file for uploading to coveralls/codecov). Removed the --silent which was really just for silencing the "text" reporter...but we don't need to run the "text" reporter, so just removed that :) Replaced with the three line "text-summary" reporter, which seems fine in the CI output

  2. unit:coverage can now just call unit:cicoverage (same with coverage:smoke) so we don't need the nyc logic duplicated, and just add an html output for local dev

  3. added --all to nyc for a slightly more honest code coverage number by covering all files (excluding the obvious tests, etc) in lighthouse-core/, lighthouse-cli/, and lighthouse-viewer/. If a file had no tests, it wouldn't show up in the coverage and bring the number down, which seems like a bad policy :)

    This does bring our coverage just below 90%, but it's not too bad. Majority of the change is in connections (extension.js and raw.js have no coverage...it's possible we just want to ignore these) and lighthouse-viewer/ (which has basically no unit tests).

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.

LGTM

"unit": "yarn unit-core && yarn unit-cli && yarn unit-viewer",
"unit:ci": "yarn unit-core:ci && yarn unit-cli:ci && yarn unit-viewer",
"unit:ci": "npm run unit-core -- --runInBand --ci --coverage && npm run unit-cli -- --runInBand --ci --coverage && npm run unit-viewer -- --runInBand --ci --coverage",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a fun line to read :)

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a fun line to read :)

ha, happy for ideas on how to make it more readable :)

"unit-cli:ci": "jest --runInBand --coverage --ci \"lighthouse-cli/\"",
"unit-viewer": "jest \"lighthouse-viewer/\"",
"unit-cli": "jest \"lighthouse-cli/\"",
"unit-viewer": "jest lighthouse-viewer/**/*-test.js",
Copy link
Collaborator

Choose a reason for hiding this comment

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

was this bringing in too many things the old way?

Copy link
Member Author

Choose a reason for hiding this comment

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

was this bringing in too many things the old way?

yeah, when we switched the viewer puppeteer test from mocha to jest, that meant the looser jest lighthouse-viewer/ was running that test as a unit test which seems wrong and doesn't help coverage anyways because it runs against the built viewer in dist/, not the original source files.

Maybe the puppeteer tests should live somewhere else?

@brendankenny
Copy link
Member Author

added another commit

  1. renamed unit:silentcoverage to unit:cicoverage (and smoke:cicoverage) as that's what it really means (making an lcov file for uploading to coveralls/codecov). Removed the --silent which was really just for silencing the "text" reporter...but we don't need to run the "text" reporter, so just removed that :) Replaced with the three line "text-summary" reporter, which seems fine in the CI output

  2. unit:coverage can now just call unit:cicoverage (same with coverage:smoke) so we don't need the nyc logic duplicated, and just add an html output for local dev

  3. added --all to nyc for a slightly more honest code coverage number by covering all files (excluding the obvious tests, etc) in lighthouse-core/, lighthouse-cli/, and lighthouse-viewer/. If a file had no tests, it wouldn't show up in the coverage and bring the number down, which seems like a bad policy :)

    This does bring our coverage just below 90%, but it's not too bad. Majority of the change is in connections (extension.js and raw.js have no coverage...it's possible we just want to ignore these) and lighthouse-viewer/ (which has basically no unit tests).

@brendankenny brendankenny merged commit 89b689f into master Mar 12, 2019
@brendankenny brendankenny deleted the coverage branch March 12, 2019 19: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