MicrosoftPremier / VstsExtensions

Documentation and issue tracking for Microsoft Premier Services Visual Studio Team Services Extensions
MIT License
56 stars 14 forks source link

coverageFailOption: 'build' to compare against previous run even if failed coverage check #110

Closed Raag007 closed 3 years ago

Raag007 commented 3 years ago

We have configured code-coverage-pipelines and these pipelines are optional in PR's

We need help with a scenario: Default line coverage is 80% and 0.5% variance.

PR 1 - makes some changes, and passes the code coverage pipeline check. PR 2 - reduces code coverage. Prior to the changes, we had 80% coverage. After, we had 75%. Since the coverage decrease relative to the previous commit on master is > 0.5%, the coverage pipeline fails the check on this set of changes. (This code checks-in to master). PR 3 - is a simple build change in the repo. It doesn't modify code coverage at all. The net coverage is still 75%, and coverage relative to the previous commit on master did not change, so the pipeline should succeed. (This PR pipeline compares with last succeeded master 80% but the current code coverage value for this branch is 75%, this pipeline always compares with 80% pipeline.

Therefore, consistent pipeline failures for PR's after code check-in with PR 2.

Is there any way, we can compare against the previous code-coverage-pipeline which has code coverage results value for master (Here it is 75%) even the pipeline is in failed state.

Let me know if you have any questions, Raag!

ReneSchumacher commented 3 years ago

Hi Raag,

I think I didn't fully understand the question. Could you post the actual configuration of the task? You mention a coverage threshold (coverageThreshold), which leads me to assume that you configured the policy with a fixed threshold. However, you also mention variance (allowCoverageVariance) and checking the previous build. Both options are not available with fixed thresholds.

If you configure a fixed threshold, the policy always compares current coverage with that threshold no matter what. It does not look at previous builds and it does not look at the configured variance. I assume that you've defined your pipeline in yaml; otherwise, the UI wouldn't let you set a variance value when you configured the policy with a fixed threshold.

From what I understand you would like to evaluate coverage changes and ensure that it's not decreasing more than 0.5%. To do that, you need to configure the policy to fail on previous build (coverageFailOption: build, coverageFailOption), allow variance (allowCoverageVariance: true, allowCoverageVariance), and set the variance value (coverageVariance: '0.5', coverageVariance). Note that this will fail the build if coverage drops more than the configured variance meaning that the highest coverage value you reached will always be the bar for every new build (we never compare with failed builds). If you only want the policy as some kind of reminder, you can set continueOnError: true so the build doesn't fail but is only marked as partially succeeded, and then set includePartiallySucceeded: true (includePartiallySucceeded) so that the policy will even take decreased values into account.

Does that help? If I misunderstood anything, just let me know.

René

Raag007 commented 3 years ago

Thanks for the quick response.
This is the task.

Default line coverage is 80% - I mean this is with previous build master result value.

Now, can you see my PRs example and let me know if you have any questions.

-Raag!

ReneSchumacher commented 3 years ago

Thanks for the update. The configuration already matches my recommendation except for the missing continueOnError: true so it's a good start.

There's one piece of information missing, though: does this pipeline run for both master and your PRs? And how is the baseBranchRef actually set?

You said that you use it as a PR validation that is optional and PR2 is completed with lower code coverage meaning that the pipeline that ran for the PR should have failed but was ignored/overridden. If that is the case, your scenario after PR2 looks somewhat like this:

Your pipeline on master won't succeed again unless you

  1. increase code coverage to at least 80% again, or
  2. set continueOnError: true in the pipeline, demoting the policy to "just a warning", or
  3. set the variable BQC.ForceNewBaseline to true (see here) for the next pipeline run.

Since I guess options one and two are not what you want, I'd recommend using the third option. We deliberately added this special variable in case you need to lower the bar for the policies.

Does that help?

Raag007 commented 3 years ago

Thanks for the response. We have set baseBranchRef default to $(System.PullRequest.TargetBranch) and this is master always.

I have a question regarding suggested option 3. In our case.

  1. We should add BQC.ForceNewBaseline: true variable to the configuration.
  2. Run the code-coverage pipeline manually with master. (Master will have 75% after code check-in with PR2)
  3. That way, this manually triggered pipeline won't fail and will ignore previously build pipeline code coverage results because we are using BQC.ForceNewBaseline: true variable.
  4. Once succeeded, this manually triggered pipeline code coverage value 75% will be used for PR 3?

-Raag!

ReneSchumacher commented 3 years ago

Basically yes. I probably wouldn't put that in the yaml pipeline. Instead, you could set the variable when queuing the manual build.

Raag007 commented 3 years ago

Got it, do we need to make any changes to this task? Or adding the suggested variable while manually running will take care of it?

ReneSchumacher commented 3 years ago

No need to change anything on the task configuration. Just add BQC.ForceNewBaseline as a variable when you queue the pipeline manually and set it to true. You'll see a "warning" that all policies are going to pass and your pipeline run will succeed. After that, the next runs will compare with the previous values again.

Let me know if there are any issue with this.

Raag007 commented 3 years ago

Quick question - why can't we use BQC.ForceNewBaseline: true in the template and set it as default?

Josmithr commented 3 years ago

Hi, I'm on the same team as Raag.

It sounds like setting BQC.ForceNewBaseline to true will result in all of the checks succeeding even if the results are outside of the expected range. Is that correct?

We're trying to avoid any manual intervention if possible in these scenarios. Basically we have 2 invariants we are trying to enforce:

  1. coverage never dips below some minimal threshold ever.
  2. Any given PR does not reduce coverage more than some very small percent-wise amount.

In the case where invariant 1 is broken, we want our coverage pipelines to continue failing indefinitely until coverage is lifted over the absolute minimum threshold (which is the behavior we currently see). But in the case where a PR breaks invariant 2, we only wish for the pipeline to fail once, and notify the developer(s) of the problem. We do not wish for subsequent PRs / CI runs to continually fail because a previous PR reduced coverage a bit too much.

Since our PRs are gated on both of the invariants, we don't expect this problem to occur often, but PR merge races occur, and there are cases where we make the conscious decision to allow a PR to go through even if it reduces coverage by more than the typical amount. In these cases it would be ideal if we didn't have to manually intervene to ensure subsequent runs succeeded.

Is there an option we can use that would instruct the tool to fail, when invariant 2 is not met, but to still publish the results to be compared against by future runs? That would be the most ideal for us.

Thanks so much for your help with this!!!

ReneSchumacher commented 3 years ago

Hi @Josmithr,

I'm afraid we don't have a good solution for that. In any case you would need two instances of the Build Quality Checks task: you configure the first one with a fixed threshold so it fails whenever coverage values drop below that boundary. That covers the first case you mentioned.

To cover the second case, you'd add a second instance of Build Quality Checks with the configuration you already are using so it compares coverage values with the previous build. This policy then starts failing as soon as coverage is lower than the previous build's coverage taking any variance you have configured into account. As BQC is meant to be a quality gate, we only ever compare with succeeded builds. Otherwise your coverage could drop lower and lower over time. As I mentioned, there are two ways to handle this:

  1. Consciously accept the drop as an exception. In other words, someone decides that this drop in coverage should be allowed and manually accepts the new lower values by forcing a new baseline with the BQC.ForceNewBaseline variable. We made this a manual step on purpose because people should be aware of what they are doing.
  2. Accept those drops by default but alert people. While we cannot make the build fail only once, you may decide to set continueOnError: true for the second instance of the BQC task. This will mark the first build that decreases coverage as partially succeeded and then start succeeding again for all subsequent builds as long as coverage values don't drop even further.

Based on your description I'd recommend using the second option. In addition I would combine it with an optional pull request status policy (see https://github.com/MicrosoftPremier/VstsExtensions/blob/master/BuildQualityChecks/en-US/PullRequests.md#status-policy) and make the build in your PR a mandatory policy. This ensures the following:

Does that help?

Josmithr commented 3 years ago

Yes, thank you for all of the detail! I think we now have a plan in place to move forward.

Thanks again!!

ReneSchumacher commented 3 years ago

Glad this works for you. I'm closing this issue then. If you still have problems getting your scenario implemented, please feel free to post additional comments or open a new issue.