Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add debugging to MTUSize test. #21463

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JamesWTruher
Copy link
Member

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

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
Copy link
Member Author

adding @vexx32 and @iSazonov as reviewers as the reply will be returned more than previously. However, the test seems to be far more robust now

@JamesWTruher JamesWTruher changed the title WIP: Add debugging to MTUSize test. Add debugging to MTUSize test. Apr 16, 2024
@JamesWTruher
Copy link
Member Author

I'm explicitly ignoring the code factor issues

Copy link
Contributor

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@@ -585,6 +585,7 @@ private void ProcessMTUSize(string targetNameOrAddress)
if (reply.Status == IPStatus.PacketTooBig || reply.Status == IPStatus.TimedOut)
{
HighMTUSize = CurrentMTUSize;
replyResult = reply;
Copy link
Member

Choose a reason for hiding this comment

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

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 microsoft-github-policy-service bot added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Apr 24, 2024
@@ -614,6 +615,7 @@ private void ProcessMTUSize(string targetNameOrAddress)
else
{
retry++;
replyResult = reply;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 25, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label May 2, 2024
Copy link
Contributor

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review - Needed The PR is being reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants