Azure Devops & Github Actions error logging

[ArmaanMcleod]

PR Summary

Fix #1364.

I've added functionality to highlight CI errors in red for ADO and GithubActions.

This is configured from the Output.CIFormat, which covers the following options:

• None -> No CI autodetection
• Auto -> Autodetects CI
• AzureDevops
• GithubActions

For AzureDevops, we check the TF_BUILD environment variable is equal to True.
For GithubActions, we check the GITHUB_ACTIONS environment variable is equal to True.

For AzureDevops we can use ##vso[task.logissue type=error] to highlight the error header, and ##[error] to highlight the messages. This ensures only the error header is reported in the build log.

For GithubActions, you can only set an error, so I had to limit this to just the header, as shown below. The messages are also collapsed under the header using expandable groups.

Azure Devops Output

Build Log

Github Actions

Build Log

I did testing in my own repo by running a simple github action to test out this code: https://github.com/ArmaanMcleod/TestGithubActions/actions/runs/952195814

PR Checklist

• PR has meaningful title
• Summary describes changes
• PR is ready to be merged
  • If not, use the arrow next to Create Pull Request to mark it as a draft. PR can be marked Ready for review when it's ready.
• Tests are added/update (if required)
• Documentation is updated/added (if required)

[ArmaanMcleod]

I will need to add some tests for this as well.

[nohwnd]

It might be nicer if the option was more generic, azdo can do also warning and info (IIRC). Even if this PR would implement just the errors, having just a single option for enabling the azdo formatted output would be better.

We should also ensure that user can override this in config to be disabled even if the env variable is set. Or vice versa.

[nohwnd]

Changed this to be draft, because it is work in progress.

[ArmaanMcleod]

It might be nicer if the option was more generic, azdo can do also warning and info (IIRC). Even if this PR would implement just the errors, having just a single option for enabling the azdo formatted output would be better.

We should also ensure that user can override this in config to be disabled even if the env variable is set. Or vice versa.

Thanks for the feedback. Agree fully about making option more generic. I've renamed it to FormatAzureDevopsLogMessages if thats ok.

For overriding the option to $false even if TF_BUILD exists, should we include another option like IgnoreBuildEnvironmentVariable to ignore this environment variable? Thinking that might be easier, since the option is set to $false by default, and it would be difficult to determine how the env variable would get ignored without something else to override it. Perhaps I'm overcomplicating it, let me know your thoughts 👍

[nohwnd]

There is IsOriginalValue() on the options on config for this purpose.

PS> $c = New-PesterConfiguration
PS> $c.CodeCoverage.Enabled.Value -eq $false
True
PS> $c.CodeCoverage.Enabled.IsOriginalValue()
True
PS> $c.CodeCoverage.Enabled = $false
PS> $c.CodeCoverage.Enabled.IsOriginalValue()
False

I look at this during the merging, so this is probably automatic, I think the setting to pester preference happens before merging it to user provided config, and in that case user provided value will take precedence if they set it explicitly.

[nohwnd]

FormatAzureDevopsLogMessages

Sounds complicated, give me some time to come up with some better name. @fflaten do you have any opinion?

[ArmaanMcleod]

Ah that's perfect, IsOrignalValue() will definitely solve the issue 👍 .

Yeah I don't like FormatAzureDevopsLogMessages either now that I'm using it. Could shorten AzureDevops to something like ADO or AzDO. Maybe even replace Format with Write, although this seems more aligned with formatting.

Perhaps something like FormatADOLogOutput?

[fflaten]

Do we need a ADO-specific option? Today it's Azure, tomorrow it's GitHub Actions (if we can use https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions from Pester), Travis CI etc. I'm not familiar with all the different CI/CD systems. Maybe some will require a different Output-plugin in the future, but if Github Actions is possible, I'd expect it to be built-in like ADO. 🙂

Would a enum like Output.CIFormat be better? Since it modifies the output while supporting only one value should be supported at a time (None being default).

[fflaten]

Just tested, GitHub Actions format works:

So I'd personally expect that to be the next step as ADO (Pester's own CI) and GitHub Actions are equally popular.

[fflaten]

Can we limit the prefix to the first line only, typically the failing block/test ([-] mypath.to.failing.test)?
The error flags are used in the pipeline report as shortcuts to the log, so appending the special prefix to every message will fill the report with noise for the same error. Ex.

[ArmaanMcleod]

Would a enum like Output.CIFormat be better? Since it modifies the output while supporting only one value should be supported at a time (None being default).

I'm happy to create a Output.CIFormat option instead, which would be more generic for multiple CI providers. I'm thinking we can just support ADO and Github Actions like you said.

I'm also wondering what levels of output would we support?

ADO supports the following:

Write-Host "##[section] This is colored green!"
Write-Host "##[command] This is colored blue!"
Write-Host "##[debug] This is colored purple!"
Write-Host "##[warning] This is colored orange!"
Write-Host "##vso[task.logissue type=warning;]This is colored orange!" 
Write-Host "##[error] This is colored red!"
Write-Host "##vso[task.logissue type=error;]This is colored red!"

Which looks like this:

For now it seems errors and warnings should definitely be supported. Should we support the other format types as well? Like Debug for instance.

Can we limit the prefix to the first line only, typically the failing block/test ([-] mypath.to.failing.test)?
The error flags are used in the pipeline report as shortcuts to the log, so appending the special prefix to every message will fill the report with noise for the same error. Ex.

Also agree on this. The error logs get flooded with what I've currently done and limiting just the first line makes it less noisy. GitHub Actions will need to do the same.

We could also do TravisCI but I honestly have very little experience using it.

[nohwnd]

Option name CIColor ?

---

Coloring with just a single issue: Write-Host "##vso[task.logissue type=error;]This is colored red!" This for first line and then the color for the rest? Write-Host "##[error] This is colored red!"

---

Is original value, will probably by set after merging on the $PesterPreference, that we do few lines above. You need to move the code before & $SafeCommands['Get-Variable'] 'Configuration' -Scope Local | Remove-Variable and check the value that user provided. And also the $callerPreference from the context. If either of those are set we should not overwrite with automatic value.

[nohwnd]

Levels: I would support only error for now.

[fflaten]

Option name CIColor ?

Isn't this limiting? Color itself is better handled with ANSI as it's generic and supported in many CI-systems.

The added benefit with this type of support is the integration with CI-summary/reports.

[nohwnd]

AutodetectCI and specify what that means for each CI in the help?

[ArmaanMcleod]

AutodetectCI and specify what that means for each CI in the help?

So this autodetect option would be set to true by default? I guess that means disabling it might be simpler now since user can override to false and it won't autodetect. Even if its enabled we would still need to check if the env variable exists(unique to each CI provider), and if it doesn't exist we override it to false? This seems like a straightforward solution to me and simple to manage from a users perspective.

[fflaten]

Is original value, will probably by set after merging on the $PesterPreference, that we do few lines above.

If the merged configuration falsely reports IsOriginalValue=false for options even though it wasn't modified in neither Configuration nor callerPreference, then that sounds more like a bug in Merger imho. I haven't tested it, but I'd expect it to report correct status even after merging considering it only intends to change already user-modified settings. We include the merged one in the result-object so users should be able to detect modified/default options if they want.

AutodetectCI and specify what that means for each CI in the help?

Sounds like it might conflict with the -CI switch some people are used to. I'd like it to be a bit more explicit, but that's just my two cents. Not gonna use up my credits on this 😁

Personally I like

Output.CIFormat = 'Auto' (default), 'None', 'AzureDevOps', 'GithubActions'
# As StringOption or EnumOption (to get faster validation of false input)

# Auto-detect if not specified
if ($PesterPreference.Output.CIFormat.Value -eq 'Auto') {
    if ($env:thatAzDOdetectionThing) {
        $PesterPreference.Output.CIFormat = 'AzureDevOps'
    }
    elseif ($env:MagicGitHubActionsDetectionCondition {
        $PesterPreference.Output.CIFormat = 'GithubActions'
    }
}

[ArmaanMcleod]

TODO right before merging:
Disable in PesterPreference for RSpec tests that fail on purpose.

Thanks @fflaten, I added some commits to disable this for deliberate failing tests. If I have to test and/or add more more changes I will stash these commits and apply them later. The current build doesn't have any of these errors highlighted anymore.

@nohwnd This is probably ready for review again. I've updated the PR description with the new changes.

Let me know what you guys think 🙂 .

[fflaten]

Looks good 👍 Added some thoughts.

Not sure if we decided on the option-name and values, but that's up to you two. Starting to see the benefit of only None vs Auto like @nohwnd mentioned , and just mention AzDO and GHA as currently supported in the option string/description.

[nohwnd]

I will have another look on this later this week.

[ArmaanMcleod]

I made some changes to the GHA formatting, where the messages under the error header are collapsed into an expandable group. Screenshots are in the PR description.

Also added CI formatting in Main.ps1, since there is a section in there that catches an exception and writes to screen. Some reason I missed this the first time 😄 .

[nohwnd]

The output looks great, and I am reasonably happy about the naming (can't come up with better one though). Only a small nit pick about the variables in tests, and then it's imho ready to merge. Will wait for the changes, and @fflaten approve.

[fflaten]

Great work @ArmaanMcleod ! 🎉

I've been busy the last days, but I didn't have anything to add in a review. It's mostly the option-design/name I'm not sure about, but I think we've settled on this for now. Maybe we get some feedback from other users with the now released alpha. 👍