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

fix test coverage reporting when running with parallelization #6590

Merged
merged 1 commit into from
Jun 1, 2022

Conversation

aldavidson
Copy link
Contributor

@aldavidson aldavidson commented Jun 1, 2022

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

When running tests with multiple parallel workers (as in C.I. and locally by default), SimpleCov was not correctly merging the coverage reports from each individual worker. As a result, our test coverage was being mis-reported as 63%.

The issue dates back to commit 979437 which removed the parallel_tests gem, which SimpleCov detects automatically and works well alongside, replacing it with Rails 6's native test parallelization, which SimpleCov has known issues with.

This PR adds a few lines of extra config on parallelization setup/teardown to fix SimpleCov's naming of the individual coverage reports, and to correctly surface the coverage output from each worker

Before:

$ govuk-docker run whitehall-lite rake test cucumber
(...) 12464 / 19675 LOC (63.35%) covered

After:

$ govuk-docker run whitehall-lite rake test cucumber
(...)
 16269 / 17333 LOC (93.86%) covered.

Copy link
Contributor

@leenagupte leenagupte left a comment

Choose a reason for hiding this comment

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

Al, this looks really good. Amazing bit of sleuthing!

Just a couple things to make the change history easier to follow for future developers: 1. Can you fix up the linting fixes to previous commit and 2. the PR description is great, could you add that to the commit message. Thanks!

@aldavidson
Copy link
Contributor Author

  1. Can you fix up the linting fixes to previous commit and 2. the PR description is great, could you add that to the commit message.

Done!

Copy link
Contributor

@ollietreend ollietreend left a comment

Choose a reason for hiding this comment

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

Great work @aldavidson

It's odd that your test run in the PR description seemed to produce 93.86% coverage, whereas Jenkins claims it to be 94.55%. I wonder if it's the difference between running MiniTest & Cucumber vs just Cucumber on its own, since Jenkins does both but I think you only ran Cucumber.

Either way, looks great. Thanks for this!

Copy link
Contributor

@leenagupte leenagupte left a comment

Choose a reason for hiding this comment

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

Thanks for updating the commit message. 🎉

You don't need the line about "rubocop fixes" though! 😄

@aldavidson
Copy link
Contributor Author

It's odd that your test run in the PR description seemed to produce 93.86% coverage, whereas Jenkins claims it to be 94.55%.

That may also be because I rebased from main after writing the description?

When running tests with multiple parallel workers (as in C.I. and locally by default), SimpleCov was not correctly merging the coverage reports from each individual worker. As a result, our test coverage was being mis-reported as 63%.

The issue dates back to commit 979437 which removed the parallel_tests gem, which SimpleCov detects automatically and works well alongside, replacing it with Rails 6's native test parallelization, which SimpleCov has known issues with.

This commit adds a few lines of extra config on parallelization setup/teardown to fix SimpleCov's naming of the individual coverage reports, and to correctly surface the coverage output from each worker
@aldavidson
Copy link
Contributor Author

You don't need the line about "rubocop fixes" though! smile

Eep! I'm rusty :) amended now, thanks

@aldavidson aldavidson merged commit f422a26 into main Jun 1, 2022
@aldavidson aldavidson deleted the fix-parallel-test-coverage branch June 1, 2022 12:02
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.

None yet

3 participants