Fix Key Vault tests that are completing too soon #18546
Conversation
Several invocations of Assert.ThrowsAsync were incorrect which likely lead to the issues failing nightly live tests.
| @@ -60,7 +62,7 @@ public void StartCreateCertificateError() | |||
| }, | |||
| }; | |||
|
|
|||
| RequestFailedException ex = Assert.ThrowsAsync<RequestFailedException>(() => Client.StartCreateCertificateAsync(certName, certificatePolicy)); | |||
| RequestFailedException ex = Assert.ThrowsAsync<RequestFailedException>(async () => await Client.StartCreateCertificateAsync(certName, certificatePolicy)); | |||
There was a problem hiding this comment.
Hm, what does this change? It was a Task-returning lambda before as well.
There was a problem hiding this comment.
Both forms should do the same thing modulo the extra state machine:
https://sharplab.io/#v2:CYLg1APgAgTAjAWAFBQAwAIpwKwG5nJQDMmM6AwugN7Lp2YlQAs6AsgBRTYA8W2AdFABsAPnQAXAJTVa9ObLl0O7aQF4xAFRWT8SRfQ5QAHOhXp1mAJzotknQvQBfB/peLimITZU09+/1AA7J785AD2ALYADgA2AKbiccDCuv70zn50DsiOQA===
There was a problem hiding this comment.
What, I believe, is different is how exceptions are actually handled. The operations tests that keep failing during live runs appear to be firing too early, and I believe the lack of the state machine higher up the call stack may be to blame. I'm not 100% sure, though. I've walked through the code before and can't think of any other reasons that status is still "inProgress" when the method call returns.
All the other changes is just because I noticed all other code in our repo seems to be using async/await with Assert.ThrowsAsync. Previously, I figured any Task-returning method would be fine, and most of the tests I updated throw synchronously anyway (e.g. ArgumentNullException immediately after method invocation).
If you'd rather, I could revert all the other changes besides the ones I intended, but I feel this also makes the code consistent with other projects. I don't think the addition of the state machine will noticeably hinder test code.
There was a problem hiding this comment.
I want to understand if there is really a problem and see if we need to fix other projects the same way. I would think the exception handling is the same as well.
There was a problem hiding this comment.
There's definitely a problem (expected OperationCancelledException not thrown in CertificateClientLiveTests) but I'm not 100% sure this will fix it. But it seems the only plausible explanation because if not awaiting the call, exception handling I believe would be different. I'll ping you offline with a link to the issue.
|
Per offline discussion, I'll enable content logging first till we get the next hit. Perhaps it's possible that we're getting error details while the status is "inProgress". Won't know without content (and would seem like a service bug in that case). |
ghost
added
the
no-recent-activity
|
Hi @heaths. There hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by removing the |
|
Closing this for now, but will keep the branch alive. |
https://learn.microsoft.com/en-us/powershell/scripting/whats-new/what-s-new-in-powershell-74?view=powershell-7.4 - Add AllowInsecureRedirect switch to Web cmdlets (#18546) That new option cause a new exception type that exposed a bug where we assumed if InnerException property existed that it was not null. This fix verifies that the property is not null.
https://learn.microsoft.com/en-us/powershell/scripting/whats-new/what-s-new-in-powershell-74?view=powershell-7.4 - Add AllowInsecureRedirect switch to Web cmdlets (#18546) That new option cause a new exception type that exposed a bug where we assumed if InnerException property existed that it was not null. This fix verifies that the property is not null. Co-authored-by: Wes Haggard <[email protected]>
Several invocations of Assert.ThrowsAsync were incorrect which likely
lead to the issues failing nightly live tests.