MicrosoftPremier / VstsExtensions

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

Coverage Policy Error Reported when createBuildIssues is false #113

Closed pinkfloydx33 closed 4 years ago

pinkfloydx33 commented 4 years ago

I have a coverage policy check where createBuildIssues is purposefully set to false. When the policy fails, an error is being emitted to the pipeline as an error causing my pipeline to go 'yellow'. I run two different checks in the same pipeline, both fail but only the first emits the error. It seems that createBuildIssues: false is not working properly for fixed coverage checks.

This is the task that cause the error to be emitted:

      - task: BuildQualityChecks@7
        displayName: 'Check Minimum Line Coverage'
        inputs:
          checkCoverage: true
          coverageFailOption: 'fixed'
          coverageType: lines
          treat0of0as100: true
          coverageThreshold: 70
          runTitle: 'Minimum Coverage'
          createBuildIssues: false
        continueOnError: true 

This check (also failing) does not emit an error onto the pipeline

      - task: BuildQualityChecks@7
        displayName: 'Check Coverage Improvement'
        inputs:
          checkCoverage: true
          coverageFailOption: 'build'
          coverageType: 'lines'
          treat0of0as100: true
          forceCoverageImprovement: true
          coverageUpperThreshold: 70
          includePartiallySucceeded: false
          baseDefinitionId: '$(System.DefinitionId)'
          ignoreDecreaseAboveUpperThreshold: true    
          baseBranchRef: '$(PolicyCheck.TargetBranch)' # from another task
          runTitle: 'Required Improvement'
          createBuildIssues: 'false'
        continueOnError: true

We are running version 7.4.7 of the extension. Here is the full output of the log for the task that is not behaving properly:

SystemVssConnection exists true
Using IdentifierJobResolver
Validating code coverage policy...
Successfully read code coverage data from build.
Evaluating coverage data from 1 filtered code coverage data sets...
Total lines: 335
Covered lines: 144
Code Coverage (%): 42.9851
##[error]At least one build quality policy was violated. See Build Quality Checks section for more details.
Finishing: Check Minimum Line Coverage

I've attached the full redacted log from running with System.Debug=true logs.txt

ReneSchumacher commented 4 years ago

Hi @pinkfloydx33,

thanks for reporting this and sorry for the inconvenience this might be causing. I'll look into it and fix the issue as quick as I can.

ReneSchumacher commented 4 years ago

Hm, I probably should have looked at the logs first before commenting :-)

I cannot see any issue here. According to the log you configured a minimum coverage threshold of 60% line coverage but your tests only covered 42.9851%. Thus, the policy is failing (i.e., emitting an error), which is converted to a partially failed pipeline run, because you set continueOnError to true. This is totally what I would expect.

You didn't attach a log for your other check. Yet, the configuration is different. Instead of comparing with a fixed minimum threshold you compare coverage with the previous build's coverage and force improvements with an upper threshold of 70%. This only fails if the coverage does not increase from build to build or falls below 70% (since you set ignoreDecreaseAboveUpperThreshold to true).

pinkfloydx33 commented 4 years ago

I can attach the log for the other one later this morning. But shouldn't the fact that createBuildIssues: 'false' prevent the error record? Shouldn't I only see an error record emitted if the setting is true?

That's what I would expect based on the documentation for the setting and it is how the other check behaves. Otherwise what is the purpose of this setting if not to suppress the error record? I understand why my pipeline goes yellow: an error record is emitted and continueOnError=true so I get a partially succeeded run; if continueOnError were false I'd see a full failure.

With createBuildIssues=false I'm expecting the policy to fail and for it to be noted on the policy extension tab and for it to report a failed policy to a PR but not to error the build itself (partially nor completely).

A little background: I actually do set this value to true for PR builds only (using logic from a Task that runs prior to it). We're working on a new product and migrating to yaml pipelines and a whole new work flow. These are all new concepts to everyone but myself. A lot of pipelines start off without appropriate coverage as this is something else the team is not used to. We expect by PR time they'll be good but in the interim a non-green pipeline confuses everyone and I get a lot of reports from other devs about "broken" builds.

ReneSchumacher commented 4 years ago

I see where your expectation comes from and perhaps we need to polish or docs a bit more. The createBuildIssues setting does not affect the task result in any way. It just affects how warnings and errors of the task are reported.

Initially, errors and warnings have only been shown in the task log and in the task summary section. If, e.g., a policy was violated, the task would fail with the generic error "A policy was violated" and users needed to switch to the Extension tab of the pipeline summary and look at the custom output. After a while, some users have complained that they want to see errors and warnings from the BQC task in the same way as other build issues are listed, i.e., right on the summary page. Thus, we introduced the setting createBuildIssues which - if set to true - publishes all warnings and errors as regular build issues so they show up on the summary page.

Independent of that setting, though, the task result is set according to the policy status; so if your build violates the policy, the task will always fail. After all, what good is it, if you violate the policy but we still show everything as healthy? And that is where the continueOnError setting comes into play, which is explicitly designed for these scenarios. The build still succeeds, but it is highlighted in a special way (partially succedded) to let people know that there is an issue that they chose to ignore.

pinkfloydx33 commented 4 years ago

Ok it seems like I was mistaken and that both failing checks are indeed issuing error records. I could've sworn the second one wasn't (or maybe it was legitimately passing and I overlooked it).

So then I guess my question is what is the purpose of createBuildIssues=false if not to suppress the error record?

pinkfloydx33 commented 4 years ago

Sorry I missed your reply.

If the purpose of the flag is to suppress/enable for the summary tab, then I will say that it's broken (or I still don't understand) because the records appear on the summary page when set to false. I'm on my phone so cannot attach a screenshot but will shortly.

pinkfloydx33 commented 4 years ago

Ok--I see the difference now as I re-ran my pipelines with setting forced to true.

I would say then that my original issue is not an actual issue. Should I close the issue or should we re-purpose it for clarification to the documentation?

In either event I appreciate your time.

ReneSchumacher commented 4 years ago

Let's close the issue and I put an item on our backlog to clarify the documentation. Again, thanks for reporting this. It's always good to receive feedback about things that aren't intuitive or documented in an ambiguous way.