-
-
Notifications
You must be signed in to change notification settings - Fork 11
adding option to set max threshold #27
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
bin/jest-it-up
Outdated
| '--max-threshold <max>', | ||
| 'max value to update the threshold', |
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.
What do you think of renaming it target threshold? Should avoid the min vs max confusion. Also with a short -T flag:
| '--max-threshold <max>', | |
| 'max value to update the threshold', | |
| '-T, --target <threshold>', | |
| 'target threshold to reach', |
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.
done
README.md
Outdated
| -d, --dry-run process but do not change files | ||
| -v, --version output the version number | ||
| -p, --precision number of threshold decimal places to persist | ||
| ---max-threshold max value to update the threshold |
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.
Please run jest-it-up --help and paste the output here so that the docs match.
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.
done
lib/getNewThresholds.js
Outdated
| if (nextCoverage > threshold + margin) { | ||
| const next = Math.min(nextCoverage, maxThreshold ?? 100) |
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.
Can't we skip updating if nextCoverage is greater than maxThreshold instead?
| if (nextCoverage > threshold + margin) { | |
| const next = Math.min(nextCoverage, maxThreshold ?? 100) | |
| if (nextCoverage > threshold + margin && nextCoverage <= maxThreshold) { |
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.
not quite. if the nextCoverage is bigger than the targetThreshold, then we still want to update it to the targetThreshold.
But if the previous threshold was already the target threshold, then we don't need to update it. I added that condition.
5d32671 to
dad8e2f
Compare
Hi there,
In our org we would like to set a max value for coverage. Meaning if the coverage is above that value, we no longer bump the thresholds.
This ensures that we set realistic targets that we don't have to adjust downwards all the time.
This would also fix issue #21
This PR adds this ability to jest-it-up.
How it works:
current:
actual:
npx jest-it-up --max-threshold 60new:
please review it and consider merging it, if you think it adds value.