Add debugging to MTUSize test. #21463
Conversation
JamesWTruher
requested review from
daxian-dbw,
TravisEz13,
adityapatwardhan and
anmenaga
as code owners
JamesWTruher
changed the title
|
I'm explicitly ignoring the code factor issues |
microsoft-github-policy-service
bot
added
the
Review - Needed
| @@ -585,6 +585,7 @@ private void ProcessMTUSize(string targetNameOrAddress) | |||
| if (reply.Status == IPStatus.PacketTooBig || reply.Status == IPStatus.TimedOut) | |||
| { | |||
| HighMTUSize = CurrentMTUSize; | |||
| replyResult = reply; | |||
There was a problem hiding this comment.
looks like you now set replyResult = reply in each condition, so why not just have it set before the if statement?
microsoft-github-policy-service
bot
added
Waiting on Author
| @@ -614,6 +615,7 @@ private void ProcessMTUSize(string targetNameOrAddress) | |||
| else | |||
| { | |||
| retry++; | |||
| replyResult = reply; | |||
There was a problem hiding this comment.
By looking at the code, it seems to me replyResult should only be assigned when reply.Status == IPStatus.Success. When not, the algorithm retries -- if it's still not successful after exceeding the maximum retries, it writes out error and returns (see code here) -- so it will not call WriteObject when reply.Status is not Success.
There was a problem hiding this comment.
@daxian-dbw - this is the reason that the test is flaky. Without returning the reply we cannot determine if the request has timed out. That's why I'm adding it under all these circumstances.
There was a problem hiding this comment.
i suppose i could add the reply as the target object of the error, save the error in the test (via -ErrorVariable) and then check in that case
There was a problem hiding this comment.
Sorry for the delay of getting back to this. Yes, saving the object in the error and checking the error in the test would be a better approach, as that won't change the intention of the method.
microsoft-github-policy-service
bot
removed
the
Waiting on Author
microsoft-github-policy-service
bot
added
the
Review - Needed
Also be sure to include the reply in more conditions. Fix up os version for Mac in get-platforminfo.
Use the proper parameter when getting macOS product version.
JamesWTruher
force-pushed
the
MtuTestFix01
branch
from
29ce422 to
eb99667
Compare
|
📣 Hey @JamesWTruher, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
daxian-dbw
added
the
CL-Test
Also be sure to include the reply in more conditions.
Fix up os version for Mac in get-platforminfo.
PR Summary
PR Context
The MTUSize test is somewhat flakey.
This hopes to improve that.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).