Skip to content

Conversation

o-l-a-v
Copy link

@o-l-a-v o-l-a-v commented May 27, 2024

PR Summary

Wrote two tests for issue #1504. The issue is not fixed yet, but I had two scenarios that could easily be added to the relevant test file.

Repro to wrongfully trigger PSUseUsingScopeModifierInNewRunspaces, which I added to test:

# Microsoft.PowerShell.Core \ Start-Job
Start-Job -ScriptBlock {
    Param($Foo)
    $Foo
} -ArgumentList 'Bar' | Receive-Job -Wait -AutoRemoveJob

# Microsoft.PowerShell.ThreadJob
Start-ThreadJob -ScriptBlock {
    Param($Foo)
    $Foo
} -ArgumentList 'Bar' | Receive-Job -Wait -AutoRemoveJob

PR Checklist

Copy link
Collaborator

@bergmeister bergmeister left a comment

Choose a reason for hiding this comment

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

updated branch to run new CI, looks good otherwise, always happy to have more tests :-)

}
}
'@
ScriptBlock = '{
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for making it more readable :-)

@andyleejordan
Copy link
Member

Are these essentially testing that the extant bug exists, and should fail when we fix the bug (meaning we'll flip their assertion)?

@o-l-a-v
Copy link
Author

o-l-a-v commented Mar 17, 2025

Sorry for the late reply. I wrote the rules with the goal of them failing with current behavior.

@bergmeister
Copy link
Collaborator

Sorry for the late reply. I wrote the rules with the goal of them failing with current behavior.

Gotcha, makes sense now. Do you plan to fix the issue as part of this PR? Changing it to draft for now then

@bergmeister bergmeister marked this pull request as draft March 17, 2025 15:29
@o-l-a-v
Copy link
Author

o-l-a-v commented Mar 17, 2025

Gotcha, makes sense now. Do you plan to fix the issue as part of this PR? Changing it to draft for now then

No. I just had two very concrete examples that I wanted to add somewhere for when this maybe gets fixed in the future.

@andyleejordan
Copy link
Member

Got it, will leave open as a draft PR then. Thanks!

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