Skip to content

Fix container name in ScriptBlock BeforeAll failures (fixes #2636). #2637

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

Conversation

davidmatson
Copy link
Contributor

@davidmatson davidmatson commented Apr 8, 2025

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)

Replace Result.Item with Result.Name to handle the ScriptBlock case. Previously, the container name was being reported only for files, not for ScriptBlocks (because files, unlike ScriptBlocks, have Result.Item).

@davidmatson
Copy link
Contributor Author

davidmatson commented Apr 8, 2025

OLD: This PR is still a draft and not yet ready to be merged. There's one test failure that I don't understand. If a maintainer knows the right change here, please feel free to edit.

| - Running in PSHost without UI 
[n] - Executes successfully without errors -> Expected not $null but got $null. 
  at Verify-NotNull, /Users/runner/work/1/s/tst/axiom/Verify-NotNull.ps1:8 
  at <ScriptBlock>, /Users/runner/work/1/s/tst/Pester.RSpec.InNewProcess.ts.ps1:387 
  at t, /Users/runner/work/1/s/tst/p.psm1:110 
  at <ScriptBlock>, /Users/runner/work/1/s/tst/Pester.RSpec.InNewProcess.ts.ps1:374 
  at b, /Users/runner/work/1/s/tst/p.psm1:80 
  at <ScriptBlock>, /Users/runner/work/1/s/tst/Pester.RSpec.InNewProcess.ts.ps1:372 
  at i, /Users/runner/work/1/s/tst/p.psm1:45 
  at <ScriptBlock>, /Users/runner/work/1/s/tst/Pester.RSpec.InNewProcess.ts.ps1:19 
  at <ScriptBlock>, /Users/runner/work/1/s/test.ps1:93 
  at <ScriptBlock>, /Users/runner/work/1/s/test.ps1:86 
  at <ScriptBlock>, /Users/runner/work/_temp/66f25778-d7a2-4923-949a-c5761f8e13bd.ps1:3 
  at <ScriptBlock>, <No file>:1 

EDIT: Found the answer; see below.

@davidmatson
Copy link
Contributor Author

I figured out why that one test was failed and made an edit to get it to pass. I think the PR is ready to now.

@davidmatson davidmatson marked this pull request as ready for review April 8, 2025 20:59
@nohwnd
Copy link
Member

nohwnd commented May 2, 2025

Thanks for the PR, can you add more information about the fix, and add a specific test for it? The edit in the current test looks little suspicious, can you explain why it needs to change like this?

also putting the Fixes #number directly into the PR description is more helpful, because it will link the PR to the issue, and will close automatically, unlike if you put it in the title 🙂

@davidmatson
Copy link
Contributor Author

also putting the Fixes #number directly into the PR description is more helpful, because it will link the PR to the issue, and will close automatically, unlike if you put it in the title 🙂

Oh, I thought it worked either way, based on the docs and previous experience:
https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue

For example, in this case, I think it still linked the issue automatically (see above). Let me know if you want me to change it anyway though.

@davidmatson davidmatson force-pushed the user/dmatson/fixScriptBlockBeforeAllContainerName branch from 400a6e8 to fd1b9f6 Compare May 2, 2025 23:11
@davidmatson
Copy link
Contributor Author

Thanks for the PR, can you add more information about the fix, and add a specific test for it?

sure - done

@davidmatson
Copy link
Contributor Author

The edit in the current test looks little suspicious, can you explain why it needs to change like this?

Sure - here's what this test output would look like for a file:


Starting discovery in 1 files.
Discovery found 1 tests in 865ms.
Running tests.
[+] path\to\some\Tests.ps1 4.22s (980ms|2.52s)
Tests completed in 4.33s
Tests Passed: 1, Failed: 0, Skipped: 0, Inconclusive: 0, NotRun: 0

but here's what it currently looks like for a ScriptBlock:


Starting discovery in 1 files.
Discovery found 1 tests in 83ms.
Running tests.
[+]  Describe 'd' { It 'i' { 1 | Should -Be 1 } }  145ms (6ms|58ms)
Tests completed in 150ms
Tests Passed: 1, Failed: 0, Skipped: 0, Inconclusive: 0, NotRun: 0

i.e. it includes the entire code for the ScriptBlock rather than the ScriptBlock's formatted "name". It probably gets really messy if the script block is long.

The updated code instead shows it more like a normal file:


Starting discovery in 1 files.
Discovery found 1 tests in 49ms.
Running tests.
[+] <ScriptBlock>  103ms (21ms|36ms)
Tests completed in 108ms Tests Passed: 1,  Failed: 0,  Skipped: 0,  Inconclusive: 0,  NotRun: 0

Let me know if that helps clarify the motivation for changing the existing test. I think that's net a good change, but let me know if you think otherwise.

@nohwnd
Copy link
Member

nohwnd commented May 5, 2025

Thanks for the test, is this solving a real issue for you? I don't normally follow the success output, and I don't expect people to run tests from scriptblocks that much. I mostly added that to be able to test pester easily. Do you run tests from scriptblocks?

Before:

image

After:

image

I don't see any information lost from failed tests because there we show the failure directly, which is great.

@davidmatson
Copy link
Contributor Author

Thanks for the test, is this solving a real issue for you?
I don't normally follow the success output, and I don't expect people to run tests from scriptblocks that much. I mostly added that to be able to test pester easily. Do you run tests from scriptblocks?

We give multiple other teams the ability to run Pester tests within another ES, and we support scriptblocks. In the past, when I've guessed there were Pester features other teams wouldn't be using, I've guessed wrong : )

I don't see any information lost from failed tests because there we show the failure directly, which is great.

For the failure case, the line with the failure is currently just "[-]", which doesn't identify the name of the file with the ScriptBlock that failed. Displaying that information consistently allows us to integrate it back into our ES's logs.

@davidmatson davidmatson force-pushed the user/dmatson/fixScriptBlockBeforeAllContainerName branch from fd1b9f6 to 7e763ba Compare May 6, 2025 20:23
@nohwnd nohwnd merged commit d08debe into pester:main May 9, 2025
11 checks passed
@nohwnd
Copy link
Member

nohwnd commented May 9, 2025

ok merged into pester 6, do you need those changes in pester 5?

@davidmatson davidmatson deleted the user/dmatson/fixScriptBlockBeforeAllContainerName branch May 9, 2025 16:53
@davidmatson
Copy link
Contributor Author

Thanks, @nohwnd!
Yes, if we could get these fixes into Pester 5 as well, that would be great - thanks!

@nohwnd
Copy link
Member

nohwnd commented May 10, 2025

/backport to rel/5.x.x

Copy link

Started backporting to rel/5.x.x: https://github.com/pester/Pester/actions/runs/14946881012

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.

2 participants