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

Fix for issue 47: When Pester tests fail, it should fail the build. #48

Merged
merged 2 commits into from
Feb 13, 2020

Conversation

splatteredbits
Copy link
Contributor

No description provided.

Copy link
Contributor

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

Typically setting $host.SetShouldExit() to anything other than 0 should fail the build. Note that a Write-Error -ErrorAction Stop or a throw will only cause the external error code to be set to 1.

Have you tested and confirmed this change allows chef verify and others to properly register the failure?

It may be worth testing to see whether exit $result.FailedCount may work more effectively, as well.

@@ -129,6 +129,10 @@ def run_command_script
$result = Invoke-Pester -OutputFile $OutputFilePath -OutputFormat NUnitXml -Path $TestPath -Passthru
$result | Export-CliXml -Path (Join-Path -Path $TestPath -ChildPath 'result.xml')
$host.SetShouldExit($result.FailedCount)
if( $result.FailedCount )
{
Write-Error -Message ('Pester finished with {0} failing tests.' -f $result.FailedCount) -ErrorAction Stop
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be inclined to suggest using throw rather than Write-Error -ErrorAction Stop 🙂

Suggested change
Write-Error -Message ('Pester finished with {0} failing tests.' -f $result.FailedCount) -ErrorAction Stop
throw 'Pester finished with {0} failing tests.' -f $result.FailedCount

@vexx32
Copy link
Contributor

vexx32 commented Feb 12, 2020

Also, /cc @smurawski for the build failure, as it appears to be related to some changes he made (it's throwing up on the Kitchen::VERSION constant being missing; perhaps there's a place we should be setting that, but not sure exactly where that should be being set / pulled from).

@splatteredbits
Copy link
Contributor Author

Typically setting $host.SetShouldExit() to anything other than 0 should fail the build. Note that a Write-Error -ErrorAction Stop or a throw will only cause the external error code to be set to 1.

It typically would, but Chef/Test Kitchen/something wraps the Pester verifiers PowerShell script and adds its own $host.SetShouldExit(). The script actually being run does this:

          Import-Module Pester -Force
          $TestPath = "#{config[:root_path]}"
          $OutputFilePath = $TestPath | Join-Path -ChildPath 'PesterTestResults.xml'
          $result = Invoke-Pester -OutputFile $OutputFilePath -OutputFormat NUnitXml -Path $TestPath -Passthru
          $result | Export-CliXml -Path (Join-Path -Path $TestPath -ChildPath 'result.xml')
          $host.SetShouldExit($result.FailedCount)
          if( $result.FailedCount )
          {
            Write-Error -Message ('Pester finished with {0} failing tests.' -f $result.FailedCount) -ErrorAction Stop
          }


$host.SetShouldExit($LASTEXITCODE)

Which doesn't work since Pester isn't a console app.

So, we can:

  1. Write-Error -ErrorAction Stop, which prevents the forced $host.SetShouldExit($LASTEXITCODE) from being reached.
  2. Explicitly set $LASTEXITCODE: $LASTEXITCODE = $result.FailedCount (I would need to test but pretty sure this would work).
  3. exit $result.FailedCount (again, I'd have to test)

I think I prefer #2, but I'll let you make that call.

Have you tested and confirmed this change allows chef verify and others to properly register the failure?

Yes. I'd like to add an automated way of testing, but don't know how to tell Test Kitchen "I expect these tests to fail".

@smurawski
Copy link
Contributor

@vexx32 I think pester.rb is missing a require "kitchen/version"

As to throw vs write-error -erroraction stop, I'm 🤷‍♂

@splatteredbits
Copy link
Contributor Author

I confirmed that exit $result.FailedCount and $LASTEXITCODE = $result.FailedCount both cause the build to fail if there are any failed tests. I'm going to defensively use both, just in case future versions of Test Kitchen change how it wraps PowerShell code.

Kitchen. It wraps this verifier's PowerShell script so that the last
line is `$host.SetShouldExit($LASTEXITCODE)` comes last. So, I set
$LASTEXITCODE to the number of test failures, then do my own `exit` with
that exit code. I want to make sure that no matter how the wrapper
around our code changes, it should still work.

Also, trying to fix the build.
Copy link
Contributor

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

I don't think setting $LASTEXITCODE does anything in particular, but there's nothing wrong with it other than that it's an automatic variable (which can sometimes be a bit iffy to try to overwrite).

Other than that minor point, this looks good. Thanks for fixing the build too! 💖 😊

@smurawski smurawski merged commit 0c082ac into test-kitchen:master Feb 13, 2020
@splatteredbits
Copy link
Contributor Author

@smurawski @vexx32 Any idea when this change will get released?

@smurawski
Copy link
Contributor

@splatteredbits Sorry for the delay - I was mostly out last week. I'll get it released today.

@smurawski
Copy link
Contributor

Released!

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.

3 participants