8000 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
[go: up one dir, main page]

Skip to content

Conversation

@daxian-dbw
Copy link
Member
@daxian-dbw daxian-dbw commented Oct 14, 2019

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 @rjmholt

PR Checklist

}
finally
{
AmsiUtils.Uninitialize();
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: Uninitialize calls CloseSession internally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@rjmholt
Copy link
Collaborator
rjmholt commented Oct 14, 2019

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

@TravisEz13 TravisEz13 added the CL-Test Indicates that a PR should be marked as a test change in the Change Log label Oct 14, 2019
@TravisEz13 TravisEz13 added this to the 7.0.0-preview.5 milestone Oct 14, 2019
@TravisEz13 TravisEz13 self-assigned this Oct 14, 2019
@TravisEz13 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
Copy link
Collaborator
@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

With one minor comment.

@daxian-dbw
Copy link
Member Author

I will merge this PR to unblock CI failures.

@daxian-dbw daxian-dbw merged commit 48db3de into PowerShell:master Oct 16, 2019
@daxian-dbw daxian-dbw deleted the fixes branch October 16, 2019 16:55
@ghost
Copy link
ghost commented Oct 23, 2019

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

Handy links:

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

0