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

Set-ItResult: Return distinctive ErrorId depending on switch used #2401

Merged
merged 2 commits into from
Nov 9, 2023
Merged

Set-ItResult: Return distinctive ErrorId depending on switch used #2401

merged 2 commits into from
Nov 9, 2023

Conversation

csandfeld
Copy link
Contributor

@csandfeld csandfeld commented Nov 1, 2023

PR Summary

  • Set-ItResult updated to return distinctive ErrorId depending on switch used (previously always returned PesterTestSkipped).
    • -Inconclusive now returns PesterTestInconclusive
    • -Pending now returns PesterTestPending
    • Skipped still returns PesterTestSkipped
  • Invoke-TestItem in Pester.Runtime updated to handle additional ErrorId´s.
  • Existing Set-ItResult Pester tests re-enabled.

I am not sure if these changes require additional tests and/or documentation updates - let me know if they do.

Related to issue #2400.

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)

@@ -680,7 +680,7 @@ function Invoke-TestItem {

$Test.FrameworkData.Runtime.ExecutionStep = 'Finished'

if ($Result.ErrorRecord.FullyQualifiedErrorId -eq 'PesterTestSkipped') {
if (@('PesterTestSkipped', 'PesterTestInconclusive', 'PesterTestPending') -contains $Result.ErrorRecord.FullyQualifiedErrorId) {
Copy link
Member

Choose a reason for hiding this comment

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

@fflaten what you commented on in the Issue happens here. The error record id is translated to .Skipped = $true on the test object. So all the accounting that is done above this is correct, but you can still see the actual reason for marking the test as skipped on the ErrorRecord itself on the Result object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks 🙂 Didn't remember that Add-RSpecTestObjectProperties would set the correct result based on Skipped = $true during teardown.

@fflaten
Copy link
Collaborator

fflaten commented Nov 7, 2023

LGTM as a first step. 🙂

I'd prefer to change the output a little, but unfortunately it could be a minor breaking change since it was the only identifier for inconclusive/pending until now. E.g.

  # From
  [!] Test123 is skipped, because INCONCLUSIVE: test 245.6s (143.4s|102.2s)
  [!] Test456 is skipped, because PENDING: test 5ms (2ms|3ms)
  
  # To (until one or both Are proper result values again)
  [!] Test123 is skipped (inconclusive), because test 245.6s (143.4s|102.2s)
  [!] Test456 is skipped (pending), because test 5ms (2ms|3ms)

@nohwnd
Copy link
Member

nohwnd commented Nov 9, 2023

@fflaten yeah let's not change the output, so we don't break the existing workarounds, while there is no complete solution in place.

@nohwnd nohwnd merged commit 3fe6e60 into pester:main Nov 9, 2023
14 checks passed
@nohwnd
Copy link
Member

nohwnd commented Nov 9, 2023

Merged, thanks guys :)

@csandfeld
Copy link
Contributor Author

Merged, thanks guys :)

My pleasure :)

@billgothacked

This comment was marked as spam.

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.

4 participants