Add debugging to MTUSize test. by JamesWTruher · Pull Request #21463 · PowerShell/PowerShell · GitHub
Skip to content

Add debugging to MTUSize test.#21463

Merged
daxian-dbw merged 5 commits intoPowerShell:masterfrom
JamesWTruher:MtuTestFix01
May 21, 2024
Merged

Add debugging to MTUSize test.#21463
daxian-dbw merged 5 commits intoPowerShell:masterfrom
JamesWTruher:MtuTestFix01

Conversation

@JamesWTruher
Copy link
Copy Markdown
Collaborator

@JamesWTruher JamesWTruher commented Apr 12, 2024

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

@SteveL-MSFT SteveL-MSFT mentioned this pull request Apr 15, 2024
Closed
22 tasks
@JamesWTruher JamesWTruher requested review from iSazonov and vexx32 April 16, 2024 18:57
@JamesWTruher
Copy link
Copy Markdown
Collaborator Author

JamesWTruher commented Apr 16, 2024

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

JamesWTruher commented Apr 16, 2024

I'm explicitly ignoring the code factor issues

@jborean93 jborean93 mentioned this pull request Apr 17, 2024
Open
22 tasks
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Review - Needed The PR is being reviewed label Apr 24, 2024
SteveL-MSFT
SteveL-MSFT previously requested changes Apr 24, 2024
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated
Copy link
Copy Markdown
Member

@SteveL-MSFT SteveL-MSFT Apr 24, 2024

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
daxian-dbw
daxian-dbw requested changes Apr 25, 2024
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw Apr 25, 2024

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
Copy Markdown
Collaborator Author

@JamesWTruher JamesWTruher Apr 25, 2024

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
Copy Markdown
Collaborator Author

@JamesWTruher JamesWTruher Apr 25, 2024

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

Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw May 13, 2024

Choose a reason for hiding this comment

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

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 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
@daxian-dbw daxian-dbw removed the Review - Needed The PR is being reviewed label May 13, 2024
@JamesWTruher
Add debugging to MTUSize test.
b2a4902 
Also be sure to include the reply in more conditions.
Fix up os version for Mac in get-platforminfo.
@JamesWTruher
Handle time out in MTUSize test.
f4a9b16 
Use the proper parameter when getting macOS product version.
@JamesWTruher
Add analyzer rule suppression to quiet CodeFactor.
eb99667
daxian-dbw
daxian-dbw approved these changes May 21, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM

@daxian-dbw daxian-dbw dismissed SteveL-MSFT’s stale review May 21, 2024 20:44

New commits were pushed

@daxian-dbw daxian-dbw merged commit 0be73f1 into PowerShell:master May 21, 2024
@microsoft-github-policy-service
Copy link
Copy Markdown
Contributor

microsoft-github-policy-service Bot commented May 21, 2024 •
edited by unfurl-links Bot
Loading

@daxian-dbw daxian-dbw added the CL-Test Indicates that a PR should be marked as a test change in the Change Log label May 21, 2024
chrisdent-de pushed a commit to chrisdent-de/PowerShell that referenced this pull request Sep 12, 2024
@JamesWTruher @chrisdent-de
Add debugging to the MTU size test (PowerShell#21463)
8ebef67
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@SteveL-MSFT SteveL-MSFT SteveL-MSFT left review comments

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@iSazonov iSazonov Awaiting requested review from iSazonov

@vexx32 vexx32 Awaiting requested review from vexx32

Assignees

No one assigned

Labels

CL-Test Indicates that a PR should be marked as a test change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JamesWTruher @daxian-dbw @SteveL-MSFT