Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit output information for failed tests #1975

Merged
merged 23 commits into from Jun 14, 2021

Conversation

ArmaanMcleod
Copy link
Contributor

@ArmaanMcleod ArmaanMcleod commented Jun 6, 2021

PR Summary

Fix #917

I've added a Output.StackTraceVerbosity option with the following levels:

None - All Stack trace is hidden
FirstLine - First line of stack trace is shown
Filtered - Filtered lines that belong to Pester. This is default.
Full - All stack trace is shown

Sample test

# sample.Tests.ps1
BeforeAll {
    function Test-EvenNumber {
        [CmdletBinding()]
        param (
            [Parameter(Mandatory)]
            [int]
            $Number
        )

        $Number % 2 -eq 0
    }
}

Describe Test-EvenNumber {
    Context "Even Numbers" {
        It "<number> is even" -TestCases @(
            @{ Number = 1 },
            @{ Number = 3 },
            @{ Number = 5 }
        ) {
            param($Number)
            Test-EvenNumber -Number $Number | Should -BeTrue
        }
    }
}

Sample setup code

$pesterConfig = [PesterConfiguration]::Default
$pesterConfig.Run.Container = New-PesterContainer -Path C:\Users\Armaan\Documents\powershell-dev\sample.Tests.ps1
$pesterConfig.Output.Verbosity = "Detailed"
Invoke-Pester -Configuration $pesterConfig

None

$pesterConfig.Output.StackTraceVerbosity = "None"

None

FirstLine

$pesterConfig.Output.StackTraceVerbosity = "FirstLine"

FirstLine

Filtered(default)

Filtered

Full

$pesterConfig.Output.StackTraceVerbosity = "Full"

Full

I've also include a simple deprecation notice to Debug.ShowFullErrors, which will override Output.StackTraceVerbosity to Full.

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 ArmaanMcleod changed the title Limit output information Limit output information for failed tests Jun 6, 2021
@ArmaanMcleod ArmaanMcleod marked this pull request as ready for review June 7, 2021 15:42
@ArmaanMcleod
Copy link
Contributor Author

ArmaanMcleod commented Jun 7, 2021

Hi @nohwnd and @fflaten. Ready to have this reviewed, will need to have some feedback to see if I'm on the right track here 😃.

Also for some reason the PS6_2 build is failing with this error:

2021-06-07T15:32:39.3736728Z P tests failed!
2021-06-07T15:32:39.4269610Z 1 tests failed in 'C:\agent2\_work\1\s\tst\Pester.RSpec.Demo.ts.ps1'

I am not sure what the issue is here from the error log. I'm thinking of running a docker container with that PS version to see what is wrong. Seemed to start happening when I added my own tests to Output.Tests.ps1. Perhaps I'm using some incompatible pester syntax that doesn't behave with this version.

Copy link
Member

@nohwnd nohwnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, it is probably tested anyway, but it would be nice to have explicit test for the display tack trace property having value even if the ShowStackTrace is set to false.

I would also like to see the first line of the stack trace, as we discussed in the issue. But I am sure others will prefer this bare output in some cases so we might need to make it an enum.

@nohwnd
Copy link
Member

nohwnd commented Jun 8, 2021

Also for some reason the PS6_2 build is failing with this error:

Yeah that test is flaky. If that is the only thing that fails I will re-run it or ignore it. I need to fix that test, or remove it.

@ArmaanMcleod
Copy link
Contributor Author

ArmaanMcleod commented Jun 8, 2021

Looks great, it is probably tested anyway, but it would be nice to have explicit test for the display tack trace property having value even if the ShowStackTrace is set to false.

I would also like to see the first line of the stack trace, as we discussed in the issue. But I am sure others will prefer this bare output in some cases so we might need to make it an enum.

Thanks for the review @nohwnd, I will add another test for that case.

I can also change the above to using an enum.

Would you be ok with the following verbosity settings:

  • None -> stack trace completely hidden
  • Minimal -> Keep first line of stacktrace
  • Full -> Keep all stack trace

I could create a StackTraceVerbosity enum and remove the ShowStackTrace flag.

@nohwnd
Copy link
Member

nohwnd commented Jun 8, 2021

I will have to think about the names, but knee-jerk they would be:

None
FirstLine
Filtered <- Default
Full

The default for us is to filter-out lines that belong to Pester.

@fflaten looks good?

@fflaten
Copy link
Collaborator

fflaten commented Jun 8, 2021

I agree. Thinking Simple/FirstLine for the short one.

Add a deprecation notice to Debug.ShowFullError as I guess Full might be a replacement?

@nohwnd
Copy link
Member

nohwnd commented Jun 8, 2021

Add a deprecation notice to Debug.ShowFullError as I guess Full might be a replacement?

Yeah we can just deprecate it, and make it override the new option to Full.

@ArmaanMcleod ArmaanMcleod marked this pull request as draft June 9, 2021 13:13
@ArmaanMcleod ArmaanMcleod marked this pull request as ready for review June 10, 2021 14:02
@ArmaanMcleod
Copy link
Contributor Author

ArmaanMcleod commented Jun 10, 2021

I may also be doing the Filtered option incorrectly. Will need to review the behavior from https://github.com/pester/Pester/blob/main/src/functions/Output.ps1#L423 to see If I need to change this, since currently I am just outputting the lines from the trace that are picked up as Pester internal functions.

I guess I might need some help defining pester specific lines that need to be filtered, since my current Filtered option seems to be leaving a lot out, and some lines from the Full output look like they should belong in Filtered.

@ArmaanMcleod
Copy link
Contributor Author

ArmaanMcleod commented Jun 11, 2021

@nohwnd I've updated this PR to use a new option for those various levels of output. Also added some images in the description to show new behavior + some review comments with things I'm not sure need to be changed.

Let me know what you think 🙂

@nohwnd
Copy link
Member

nohwnd commented Jun 11, 2021

I've read it yesterday, thanks for the poke, comments above.

@ArmaanMcleod
Copy link
Contributor Author

ArmaanMcleod commented Jun 12, 2021

I've read it yesterday, thanks for the poke, comments above.

Thanks for the feedback @nohwnd, really helpful.

This looks pretty much done from a code perspective, let me know if there is anything I'm missing or need to address.

@nohwnd nohwnd merged commit 989dac0 into pester:main Jun 14, 2021
@nohwnd
Copy link
Member

nohwnd commented Jun 14, 2021

Perfect. Merged.

@ArmaanMcleod ArmaanMcleod deleted the limit-output-information branch June 14, 2021 09:01
@StephenFerrero
Copy link

This is awesome, thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit information for failed tests
4 participants