FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Stabilize the event handler leaking test and make `Start-PSBuild -SMAOnly` literally just rebuild `S.M.A.dll` by daxian-dbw · Pull Request #10790 · PowerShell/PowerShell · GitHub

Stabilize the event handler leaking test and make Start-PSBuild -SMAOnly literally just rebuild S.M.A.dll - #10790

Merged
Dongbo Wang (daxian-dbw) merged 1 commit into
PowerShell:masterfrom
daxian-dbw:fixes
Oct 16, 2019
Merged

Stabilize the event handler leaking test and make Start-PSBuild -SMAOnly literally just rebuild S.M.A.dll#10790
Dongbo Wang (daxian-dbw) merged 1 commit into
PowerShell:masterfrom
daxian-dbw:fixes

Conversation

Dongbo Wang (daxian-dbw) commented Oct 14, 2019
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

The event handler test is failing intermittently in our daily test run:
https://powershell.visualstudio.com/PowerShell/_build/results?buildId=35039

The cause of it is likely the SecuritySupportTests, where the registration/unregistration of the event handler are separately in two tests.
I merge those 3 tests into one and will see if that resolves the intermittent failure.

Also slightly update Start-PSBuild to make -SMAOnly literally only re-build and re-deploy S.M.A.dll, without doing extra post-build tasks.
The main purpose of this change is to avoid spinning up the pwsh for retrieving experimental features, so that we can add code in S.M.A.dll to wait for debugger to attach for debugging scenarios. /cc Rob Holt (@rjmholt)

PR Checklist

}
finally
{
AmsiUtils.Uninitialize();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Note: Uninitialize calls CloseSession internally.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please add this explanation and why we combine the tests to the file.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think the explanation is needed here. It's more confusing as you won't see the original test cases anymore.

Copy link
Copy Markdown
Collaborator

I believe xUnit runs tests in random order and it looks like the previous implementation depended on some ordering, so this might be the fix

Travis Plunk (TravisEz13) added the CL-Test Indicates that a PR should be marked as a test change in the Change Log label Oct 14, 2019
Travis Plunk (TravisEz13) changed the title Stabilize the event handler leaking test and make Start-PSBuild -SMAOnly literally just rebuild S.M.A.dll Stabilize the event handler leaking test and make Start-PSBuild -SMAOnly literally just rebuild S.M.A.dll Oct 14, 2019

Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

With one minor comment.

Copy link
Copy Markdown
Member Author

I will merge this PR to unblock CI failures.

Dongbo Wang (daxian-dbw) merged commit 48db3de into PowerShell:master Oct 16, 2019

Copy link
Copy Markdown

🎉v7.0.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Test Indicates that a PR should be marked as a test change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL