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

test: exclude write-coverage from coverage report #15194

Closed
wants to merge 1 commit into from
Closed

test: exclude write-coverage from coverage report #15194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
!.gitignore
!.gitkeep
!.mailmap
!.nycrc
!.remarkrc

core
Expand Down
6 changes: 6 additions & 0 deletions .nycrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"exclude": [
"**/internal/process/write-coverage.js"
Copy link
Member

Choose a reason for hiding this comment

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

Is the wildcard necessary? It'll always be in lib/internal/process, unless the absolute path is used for comparison?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TimothyGu because the Makefile passes an absolute path for the test coverage's working directory:

--temp-directory "/Users/benjamincoe/bcoe/node/.cov_tmp"

nyc ends up working with the path:

/Users/benjamincoe/bcoe/node/lib_/internal/process/write-coverage.js when the file is processed by the exclude glob.

tldr; we definitely don't want to bake /Users/benjamincoe/bcoe into the exclude rule, and it seemed like prefixing with the ** was the easiest solution.

Copy link
Member

Choose a reason for hiding this comment

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

@bcoe Makes sense 👍

],
"reporter": ["html", "text"]
Copy link
Member

Choose a reason for hiding this comment

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

Does adding the "text" reporter here affect the default output ? Just wondering if results on https://coverage.nodejs.org/ will be affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhdawson I don't think it should; text will result in a nice summary being written to the terminal, for the user who just ran tests. html results in an index.html (etc) being written to the ./coverage folder, which I believe is what is in turn used on coverage.nodejs.org.

@addaleax I think I'm correct on this one?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that sounds right to me :)

}
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ coverage-test: coverage-build
$(NODE) ./node_modules/.bin/istanbul-merge --out \
.cov_tmp/libcov.json 'out/Release/.coverage/coverage-*.json'
(cd lib && .$(NODE) ../node_modules/.bin/nyc report \
--temp-directory "$(CURDIR)/.cov_tmp" -r html \
--temp-directory "$(CURDIR)/.cov_tmp" \
--report-dir "../coverage")
-(cd out && "../gcovr/scripts/gcovr" --gcov-exclude='.*deps' \
--gcov-exclude='.*usr' -v -r Release/obj.target/node \
Expand Down