Add 5 'InvokeAsync' overloads and 'StopAsync' to the 'PowerShell' type#8056
Conversation
|
Note that I have not added unit tests yet for this PR. I looked to see where BeginInvoke and EndInvoke were tested, and they are included as part of a Pester test; however, PowerShell does not support async/await, so Pester does not appear to be an appropriate test framework for these changes. Please identify how these new methods should be automatically tested. I haven't added C# unit tests to this repo yet, so I'm not sure how I should proceed. Thank you. |
|
@KirkMunro Have you any gists with demo? It would help to document the enhancement. |
|
I don't have examples showing how complicated this can be using BeginInvoke/EndInvoke, because I just went straight to defining these locally in projects where I have used async PowerShell invocation since they are just so much simpler, so I don't have a before/after example to show for comparison. Also, the code is so short, I'll just share it here instead of in a gist. Here's a snippet showing what use of this looks like when you want async invocation of PowerShell from a .NET assembly with these additional methods in place: using (var ps = PowerShell.Create())
{
ps.AddCommand("Connect-AzureRmAccount")
.AddParameter("ServicePrincipal")
.AddParameter("TenantId", tenantName)
.AddParameter("Subscription", subscriptionId)
.AddParameter("Credential", credential);
var results = await ps.InvokeAsync();
// Process results
} |
|
@KirkMunro You can add C# tests here: https://github.com/PowerShell/PowerShell/tree/master/test/csharp |
|
@PowerShell/powershell-committee agree that we should expose a task model for async PowerShell, leaving implementation details to PR review |
|
@BrucePay Can you please review the signature of the proposed task-based async APIs? In committee's discussion on this, there are proposals that we might want to re-implement the async invocation in the task-based model instead of wrapping the |
|
Regarding c# unit tests, I didn't get to them before going on vacation this week, so I'll come back to this PR next week and catch up. |
|
@powershell-maintainers The |
|
@iSazonov I'll move the tests over. Thanks for letting me know the correct path for those tests. |
|
Wait. @iSazonov, that path doesn't exist in master, so this is a new process then? How are tests under that path executed locally for verification? |
|
@KirkMunro We haven't such tests at all. And my question is for MSFT team where we should place these tests. I point my new unmerged code which have engine API xUnit tests too. I look how CoreFX places API tests and do the same. |
|
@iSazonov Ok, thanks. I'll wait until the other maintainers discuss with you what should be done with these then, and follow the process accordingly. |
|
This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. |
|
@KirkMunro Looks like @iSazonov has created a place for the xUnit tests in #8356 😄 |
|
Yes, binary tests was moved to new folder. Now all tests run parallel (excluding macOS). |
src/System.Management.Automation/engine/hostifaces/PowerShell.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/hostifaces/PowerShell.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/hostifaces/PowerShell.cs
Outdated
Show resolved
Hide resolved
test/powershell/engine/Api/TaskBasedAsyncPowerShellAPI.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/engine/Api/TaskBasedAsyncPowerShellAPI.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/engine/Api/TaskBasedAsyncPowerShellAPI.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/engine/Api/TaskBasedAsyncPowerShellAPI.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/engine/Api/TaskBasedAsyncPowerShellAPI.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/engine/Api/TaskBasedAsyncPowerShellAPI.Tests.ps1
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
You xUnit tests were failing in CIs. Please fix them. I'm also fine that you remove the xUnit tests for this PR.
src/System.Management.Automation/engine/hostifaces/PowerShell.cs
Outdated
Show resolved
Hide resolved
I removed them since they were just duplicates of the Pester tests, and I'm not set up at the moment to easily test C# Unit tests (hence the issues with the CI not working). |
|
Pending any further feedback, I think this PR is good to go with enough tests to cover the new Async APIs well enough. |
|
@KirkMunro Thanks for your contribution! |
PR Summary
It is much easier to deal with asynchronous invocation of PowerShell using async and await, but the S.M.A.PowerShell class only exposes BeginInvoke and EndInvoke methods for async support. This PR adds 5 InvokeAsync overloads (one for each BeginInvoke method) to S.M.A.PowerShell to facilitate async invocation of PowerShell commands from .NET.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests