Pester icon indicating copy to clipboard operation
Pester copied to clipboard

Ignore PassThru when run fail while using Run.Exit

Open fflaten opened this issue 2 years ago • 7 comments

PR Summary

Result object was printed when run failed because it was sent to standard output, but never reached variable assignment before Run.Exit killed the process causing a text dump.

Fix #2300

PR Checklist

  • [x] PR has meaningful title
  • [x] 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)

fflaten avatar Feb 11 '23 21:02 fflaten

/azp run

fflaten avatar Feb 11 '23 22:02 fflaten

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Feb 11 '23 22:02 azure-pipelines[bot]

This will break $res = & ./test.ps1 (when the run fails) because exit will only exit the script, so in that case $res would contain the object prior to this PR. See https://github.com/pester/Pester/issues/2300#issuecomment-1426897882

But is this intended to work or just luck? I'd lean towards merging this, because Run.Exit says Exit with non-zero exit code when the test run fails. so I wouldn't expect to be able to post-process anything.

fflaten avatar Feb 11 '23 22:02 fflaten

But is this intended to work or just luck? I'd lean towards merging this, because Run.Exit says Exit with non-zero exit code when the test run fails. so I wouldn't expect to be able to post-process anything.

Exit in powershell is defined as exiting the script with the error code, not the engine. So $res = & ./test.ps1 should imho assign the value, rather than not.

Or am I misinterpreting what you are saying here?

nohwnd avatar Mar 31 '23 15:03 nohwnd

Or am I misinterpreting what you are saying here?

I think we agree. There's two sides here and I'm not sure if fixing one is worth breaking the other, which is why I left this without ready to merge. The question is if anyone's gonna be missing the part being broken. Current behavior:

Catching output from script Running $res = & ./test.ps1 you'd keep the pipeline and result-object even on failure. But would anyone process it for a failed run while using -CI/Run.Exit? Or would they immediately check $? -eq $false and break? Personally I'd use $res = Invoke-Pester inside the test.ps1 script (and that script would exit either way), not catch the script output itself. However I can't exclude that others might do something else.

Calling Pester directly Users invoking Pester directly would get their process terminated by exit and the result-object would be printed as string (noise) in the CI-logs etc. Unless they do called Pester through a child-process like $a = pwsh -noprofile -command { Invoke-Pester ..., which seems unlikely.

fflaten avatar Mar 31 '23 16:03 fflaten

Close or keep? I agree with your reasoning above that neither case is preferable, so maybe just keep the current one and close this?

nohwnd avatar Apr 12 '24 19:04 nohwnd

I'd say merge in v6. Easy rollback if necessary.

I'm struggling to see when someone would combine Run.Exit with processing script output. If so they could just drop Run.Exit and add a check in their script to get the previous behavior.

fflaten avatar Apr 28 '24 08:04 fflaten