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.

Sign up for GitHub

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

Jump to bottom

🏗🐛 Fix gulp test --coverage #16417

Merged
merged 2 commits into from
Jun 27, 2018
Merged

🏗🐛 Fix gulp test --coverage #16417

merged 2 commits into from
Jun 27, 2018

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Jun 27, 2018 •
edited

Our code coverage tools have been broken for a while. This PR fixes them, such that gulp test [--unit|--local-changes|--files] --coverage prints a neatly formatted text code coverage report and pops up a rich HTML report in your default browser.

  • In the HTML report, the source code lines don't line up with the coverage annotations (fix coming up in a separate PR)
  • Now that this works, we should probably start running gulp test --local-changes with --coverage (coming up in a separate PR).

Fixes #16396

@rsimha rsimha self-assigned this Jun 27, 2018
@rsimha rsimha requested a review from jpettitt June 27, 2018 16:58
@rsimha
Copy link
Contributor Author

rsimha commented Jun 27, 2018 •
edited

/to @jpettitt @choumx

@rsimha rsimha requested a review from dreamofabear June 27, 2018 17:27
@jpettitt
Copy link
Contributor

Works but the uncovered line numbers it reports don't map to the actual src.

gulp test --coverage --files=extensions/amp-geo/0.1/test/*.js says line 342 in amp-geo.js is not covered but that line is a comment ...

jpettitt
jpettitt approved these changes Jun 27, 2018
View reviewed changes
Copy link
Contributor

@jpettitt jpettitt left a comment •
edited

Choose a reason for hiding this comment

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

LGTM, understanding that line numbers issues will be a later PR

@rsimha
Copy link
Contributor Author

rsimha commented Jun 27, 2018

@jpettitt The discrepancy between line numbers and the actual source appears to be due to gotwarlost/istanbul#59. The fix involves some trial and error, so I'll work on a fix in a separate PR.

@rsimha rsimha merged commit 5446179 into ampproject:master Jun 27, 2018
@rsimha rsimha deleted the 2018-06-27-Coverage branch June 27, 2018 22:16
This was referenced Jun 27, 2018
Closed
Merged
@rsimha
Copy link
Contributor Author

rsimha commented Jun 28, 2018 •
edited

Re: #16417 (comment), the line number discrepancy is addressed in #16432. With the fix in place, here's what you'll see for amp-geo.js:

image

@rsimha rsimha mentioned this pull request Jun 28, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpettitt jpettitt jpettitt approved these changes

@dreamofabear dreamofabear Awaiting requested review from dreamofabear

Assignees

@rsimha rsimha

Labels
cla: yes WG: infra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gulp test --coverage --files ... throws errors
3 participants
@rsimha @jpettitt @googlebot