-
Notifications
You must be signed in to change notification settings - Fork 109
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: exit if the coverage threshold is not met #399
Conversation
Hey @vladjerca, great work!! |
@erikbarke Glad to help! 🥳 |
@erikbarke Do you still need me to do anything on this bit? |
|
||
const isCoverageCheckFailed = results && config.hasCoverageThreshold && !threshold.check(browser, remappedCoverageMap); | ||
|
||
isBelowCoverageThreshold = isBelowCoverageThreshold || isCoverageCheckFailed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the variable isBelowCoverageThreshold
here, it's always false? Wouldn't we achieve the same result by just checking isCoverageCheckFailed
and exit if it's true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erikbarke isCoverageCheckFailed
is the local browser
instance check, where as isBelowCoverageThreshold
is the aggregate for entire run.
Now to expand a bit on why I aggregate rather than do a hard exit: I do this in order to allow all of the coverage reports to be logged before the reporter triggers a process exit 🤓
LE: also isBelowCoverageThreshold
isn't always false
it starts off as false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, got it!
@vladjerca, I totally forgot to submit the review, it's been sitting there for days unsubmitted 😞 |
I've investigated a bit what the differences were between
4.1.1
and5.x.y
, I noticed that the package was previously doingsync
operations.Since
5.0.0
the coverage calculations are done via theistanbul-lib-coverage
package to process the results.The issue here is that the processing is now async and mutation of the
results.exitCode
property is now part of amicroTask
.The only solution I could find was to aggregate all of the threshold checks and actually exit once all of the checks are performed.
fix #376