Fail decreasing #1412
Fail decreasing #1412
Conversation
This option lets the test fail, like --fail-under, if the amount of coverage is decreasing.
* Fix the error string so it ends with "%" * Refacor the implementation and let report coverage return the delta, because where it was hook up before the new report had allready been written, and would there for always report a change of 0.0%, and never failt the test, and make the application return an error.
* Add an entry to the Changelog * Add the argument to the commandline listing in the readme
Now the feature is integrated with --fail-under, so that it works like
this:
* If --fail-under is used by it self, it should behave like before
* If --fail-decreasing is used by it self, it will return an error if
the coverage percentage is decreasing.
* If --fail-under and --fail-decreasing is used together is works as
follows, if the coverage is below the threshold, it error out if the
coverage is decreasing, if the coverage is above the threshold, the
coverage can increase and decrease freely.
Summary:
* updated README and CHANGELOG
* Updated the command line help text
* Reverted some canges to src/report/mod.rs and reimplemented them in
src/lib.rs instead, made get_previous_result public
* In src/lib.rs:
* implemented get_delta, to get the coverage percentage
from the previous run, with the help of get_previous_result
* Implemented check_coverage, which now does the heavy lifting in
this change
* Refactored report_coverage_with_check to use check_coverage
nikgul
force-pushed
the
fail-decreasing
branch
from
40fec98
to
9701c60
Compare
There was a problem hiding this comment.
So one comment to simplify a function down, otherwise it looks fine to me.
Another thing you can do is add an integration test. Just take one of the integration test projects where coverage can change (i.e. one with features and you can enable more features). And then set up the combinations of fail-under and fail-decreasing. I have a bunch of tests for fail-under in tests/failure_thresholds.rs where these tests would fit in.
| report_coverage(c, tracemap)?; | ||
| check_fail_threshold(tracemap, c) | ||
| fn check_coverage(delta: f64, config: &Config, trace: &TraceMap) -> Result<(), RunError> { | ||
| let threshold = check_fail_threshold(trace, config); |
There was a problem hiding this comment.
You can make this function a lot simpler by doing just:
check_fail_threshold(trace, config)?;
if config.fail_under.is_none() {
check_fail_decreasing(delta, config)?;
}
Ok(())I think this covers all the feature space, you'd just have to move the error log into check_fail_decreasing
There was a problem hiding this comment.
I'm not sure that would be correct with the current feature set ?
If I understand the change request correctly, then check_coverage would return the threshold error, regardles if the coverage is increasing, could you read the commit message on the last commit with the fail-under integration and see if those cases are still coveret, which I don't think they are.
Have forgotten to update the pull request descrition
There was a problem hiding this comment.
Well if you set --fail-under then I would expect it to always fail if below that coverage amount even if the coverage is increasing because you haven't met the threshold.
There was a problem hiding this comment.
My thought was, that when you start to use tarpaulin, that you would be able to use it at once, and you set your end target from the start and don't have to correct it later, by providing kind of a "soft start", so that you don't have to provide all test cases at once, but you will have to provide tests for all new code.
Once you reach the threshold, then --fail-decreasing is basically not used anymore with the code implemented, if used in combination with --fail-under, where it takes over.
The user is always welcome to use them individually if the combined functionality is not what they want.
Which I have tried to describe in the cli help message, maybe I have failed that part of it, will have a look tomorrow if I need to reword something.
There was a problem hiding this comment.
Hmm I think that just adds confusion to the flag. If you don't want to see the behaviour at the start of a project you can just omit it. Though come to think of it most users use tarpaulin in CI and this flag won't do anything then anyway as they typically won't cache the previous tarpaulin results.
There was a problem hiding this comment.
So one comment to simplify a function down, otherwise it looks fine to me.
Another thing you can do is add an integration test. Just take one of the integration test projects where coverage can change (i.e. one with features and you can enable more features). And then set up the combinations of fail-under and fail-decreasing. I have a bunch of tests for fail-under in tests/failure_thresholds.rs where these tests would fit in.
Found 2 bugs while implementing the test cases for this feature. Had some issues with concurency, where different test cases would step all over each other, which has also been taken care of.
|
I have now implemented test cases for the feature, and while doing so found 2 bugs that I fixed. During test runs I see random test failures in:
Should I take a look at them, and submit a seperate PR, i expect it is something similar to what I had to deal with when I was writing the test cases in this PR, there may be more like it ? |
|
Just at work so will look properly later, but I use |
|
I tried that, but couldn't get it to work, ended up using config.set_target_dir() instead, which works nicely. |
This pull request implements the
--fail-decreasingcommand line option, which allows tarpaulin to return an error if the coverage percentage has decreased, similar to--fail-underwhich fails the run if the coverage is under a certain threshold.Current feature set implemented:
I have had a quick look at the tests, but are uncertain how and if I should make a test case for this, becaues it would require multi step tests, a first run, one increasing, one returning the same coverage, and one where it is decreasing.
This fixes: #1411
Note: I'm new to rust, so if you have any changes you want made, there could be some terminoligy, that i'm not fluent in or familier with yet :-)