Add option to give warning instead of failing the build if coverage is below threshold#1670
Add option to give warning instead of failing the build if coverage is below threshold#1670roostaamir wants to merge 4 commits intocoverlet-coverage:masterfrom
Conversation
roostaamir
commented
Jul 10, 2024
- This will enable the option to choose if the build should fail when the coverage is below the threshold or just give a warning about it
- Based on the initial idea that started here: Add possibility to give warning when coverage is below the threshold instead of just failing the build #701
- This will enable the option to choose if the build should fail when the coverage is below the threshold or just give a warning about it
…staamir/coverlet into add-threshold-warning-option
|
To the reviewer: I need a bit of help to decide where to write tests for the new functionality. Could you please give me a suggestion about where would be the best test project to write test for this newly introduced parameter? Thanks in advance |
|
@dotnet-policy-service agree |
|
Hi again Are there any updates about comments or reviews on my PR? |
|
Thank you for the contribution. Maybe an additional test in DotnetTool.cs should verify the new command line option. By the way, I would use Unfortunately, I do not have the PR approve permission. @MarcoRossignoli, @daveMueller Could you please review this PR. |
I agree could be good add some tests for it |
|
Thank you very much @Bertk and @MarcoRossignoli I will make the requested changes and update the PR :) |
There was a problem hiding this comment.
Pull request overview
Adds a configurable “action” for coverage threshold enforcement so consumers can choose whether being below threshold fails the build/process or only emits a warning.
Changes:
- Introduces
ThresholdActMSBuild property (defaultfail) and wires it through targets/props into the MSBuild task. - Adds
ThresholdActionenum and updates MSBuild task + console to honor fail vs warning behavior when coverage is below threshold. - Documents the new MSBuild option in
Documentation/MSBuildIntegration.md.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/coverlet.msbuild.tasks.tests/CoverageResultTaskTests.cs | Updates task instantiation to include the new required ThresholdAct parameter. |
| src/coverlet.msbuild.tasks/coverlet.msbuild.targets | Passes ThresholdAct into CoverageResultTask. |
| src/coverlet.msbuild.tasks/coverlet.msbuild.props | Defines default ThresholdAct (fail). |
| src/coverlet.msbuild.tasks/CoverageResultTask.cs | Implements warning-vs-fail behavior for threshold enforcement in the MSBuild task. |
| src/coverlet.core/Enums/ThresholdAction.cs | Adds new enum representing the threshold action. |
| src/coverlet.console/Program.cs | Adds --threshold-act and changes threshold enforcement behavior based on action. |
| Documentation/MSBuildIntegration.md | Documents /p:ThresholdAct usage and accepted values. |
Comments suppressed due to low confidence (4)
src/coverlet.msbuild.tasks/CoverageResultTask.cs:204
ThresholdActparsing doesn’t trim/validate input: values like "warning " or any unexpected string will silently fall back toFail. Consider trimming and explicitly erroring (or at least logging a warning) when the value isn’t one of the supported options (fail/warning) so users don’t get surprising behavior.
ThresholdAction thresholdAct = ThresholdAction.Fail;
if (ThresholdAct.Equals("fail", StringComparison.OrdinalIgnoreCase))
{
thresholdAct = ThresholdAction.Fail;
}
else if (ThresholdAct.Equals("warning", StringComparison.OrdinalIgnoreCase))
{
thresholdAct = ThresholdAction.Warning;
}
test/coverlet.msbuild.tasks.tests/CoverageResultTaskTests.cs:103
- New
ThresholdActbehavior isn’t covered by tests: please add a test case where coverage is below the threshold andThresholdAct = "warning", asserting the task succeeds and logs a warning (but no error).
var coverageResultTask = new CoverageResultTask
{
OutputFormat = "cobertura",
Output = "coverageDir",
Threshold = "50",
ThresholdType = "total",
ThresholdStat = "total",
ThresholdAct = "fail",
InstrumenterState = InstrumenterState
};
src/coverlet.console/Program.cs:39
- Please add/update an integration test for
--threshold-act warningto verify the tool returns a success exit code while emitting the expected warning message (similar to existing--thresholdtests). This change alters CLI behavior and is easy to regress without coverage.
var threshold = new Option<string>("--threshold", "Exits with error if the coverage % is below value.") { Arity = ArgumentArity.ZeroOrOne };
var thresholdTypes = new Option<List<string>>("--threshold-type", () => new List<string>(new string[] { "line", "branch", "method" }), "Coverage type to apply the threshold to.").FromAmong("line", "branch", "method");
var thresholdStat = new Option<ThresholdStatistic>("--threshold-stat", () => ThresholdStatistic.Minimum, "Coverage statistic used to enforce the threshold value.") { Arity = ArgumentArity.ZeroOrOne };
var thresholdAct = new Option<ThresholdAction>("--threshold-act", () => ThresholdAction.Fail, "The action to take when coverage is below the threshold value. Defaults to failing the build.") { Arity = ArgumentArity.ZeroOrOne };
src/coverlet.core/Enums/ThresholdAction.cs:10
- Indentation in this new enum file doesn’t match the existing style in
src/coverlet.core/Enums(most enums use two-space indentation). Please align formatting to the surrounding conventions for consistency.
namespace Coverlet.Core.Enums
{
internal enum ThresholdAction
{
Fail,
Warning
}
| switch (thresholdAct) | ||
| { | ||
| case ThresholdAction.Warning: | ||
| logger.LogWarning(exceptionMessageBuilder.ToString()); | ||
| break; |
There was a problem hiding this comment.
In Warning mode this block only logs a warning, but exitCode has already been set to CoverageBelowThreshold earlier in the method, so --threshold-act warning will still exit non-zero. To match the intended behavior, avoid setting the coverage-below-threshold exit code when thresholdAct == Warning (set it only in the Fail path).
| case ThresholdAction.Fail: | ||
| exitCode += (int)CommandExitCodes.CoverageBelowThreshold; | ||
| throw new Exception(exceptionMessageBuilder.ToString()); |
There was a problem hiding this comment.
exitCode is incremented for CoverageBelowThreshold before the switch and then incremented again in the Fail case, which can yield an incorrect exit code (e.g., 4 instead of 2). Remove the redundant increment and set the flag/code only once.