-
Notifications
You must be signed in to change notification settings - Fork 79
chore: fix coverage reporting MONGOSH-1312 #1514
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
Conversation
To quote myself from the tech design:
Expand output with some lines highlighted to show the problem
|
I think the coverage looks correct now: https://parsley.mongodb.com/evergreen/mongosh_linux_coverage_check_coverage_patch_e92fb5aa609c050ee9e7ac966ff8af2592c853c2_64b1679b7742aea8427427da_23_07_14_15_19_57/1/task?bookmarks=0,138896 83%, but I'd kinda expect it to go down because e2e tests are now basically ignored coverage-wise. Plus it already said 83%-ish before this change, so I can believe that numbers just dropped over time. You can also download the html report from that link. I can't spot any duplicates now:
|
It looks like tests that were executing code straight in lib via child_process.spawn or similar (java-shell, some tests in cli-repl) were adding broken paths into the coverage. I've changed the .nycrc config to only instrument the .ts source files, not the lib files. This will probably drop our coverage because e2e tests won't count, but at least we should (hopefully) get sensible results for now.
I temporarily lowered the coverage threshold. We'll see once this runs what it should be.
Then I also split the coverage check from the reporting so that we can generate the report, then upload it, THEN possibly fail. That way we can download the report if coverage drops below the threshold.
Potential Future Work:
A better fix might be to try and generate coverage for the compiled and webpackaged mongosh properly and then have nyc use that or take that into consideration. Then we can get and add coverage for our e2e test too.