Reinstate Test-Connection tests by rjmholt · Pull Request #13324 · PowerShell/PowerShell · GitHub
Skip to content

Reinstate Test-Connection tests#13324

Merged
adityapatwardhan merged 5 commits into
PowerShell:masterfrom
rjmholt:tests-macos-testconnection
Nov 14, 2020
Merged

Reinstate Test-Connection tests#13324
adityapatwardhan merged 5 commits into
PowerShell:masterfrom
rjmholt:tests-macos-testconnection

Conversation

@rjmholt

@rjmholt rjmholt commented Jul 31, 2020

Copy link
Copy Markdown
Collaborator

PR Summary

Fixes #13309.

Reverts some of the changes in #12943.

The Test-Connection tests first failed in CI due to a dependency on CI system DNS resolution. We tried to address that in #12943 but recently hit a failure where the new form of the tests using the gateway address also failed consistently across builds.

Given that the DNS issue was resolved, this changes the tests that have been failing back to using the hostname.

There's also a test that didn't work with DNS, was working with the gateway address, but is not working now. That test is now marked as pending.

PR Checklist

@rjmholt rjmholt added WG-Quality-Test issues in a test or in test infrastructure CL-Test Indicates that a PR should be marked as a test change in the Change Log labels Jul 31, 2020
@rjmholt

rjmholt commented Jul 31, 2020

Copy link
Copy Markdown
Collaborator Author

@rjmholt rjmholt mentioned this pull request Jul 31, 2020
Closed
6 tasks
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 3, 2020
PaulHigin
PaulHigin suggested changes Aug 3, 2020
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1 Outdated

@PaulHigin PaulHigin Aug 3, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't understand why this commented out script is here. Is this something we want to revert to, or avoid?

Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1 Outdated

@PaulHigin PaulHigin Aug 3, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add comment as to why it is marked 'pending'.

@vexx32 vexx32 Aug 3, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The pending was removed from this test?

vexx32
vexx32 approved these changes Aug 3, 2020
View reviewed changes

@vexx32 vexx32 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM apart from one of Paul's comments, we should either remove or comment as to why the extraneous code is left in. 🙂

Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1 Outdated

@vexx32 vexx32 Aug 3, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The pending was removed from this test?

@rjmholt

rjmholt commented Aug 3, 2020

Copy link
Copy Markdown
Collaborator Author

The macOS CI is still failing for the minute, so this doesn't seem to solve our problems with these tests. I'll need to find more time this week to work on this

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Aug 3, 2020
@rjmholt rjmholt force-pushed the tests-macos-testconnection branch from b099f35 to 4c843fa Compare August 12, 2020 17:05
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 12, 2020
@rjmholt rjmholt changed the title Move Test-Connection tests back to using hostname Reinstate Test-Connection tests Aug 12, 2020
@rjmholt rjmholt marked this pull request as ready for review August 12, 2020 21:03
@ghost ghost added the Review - Needed The PR is being reviewed label Aug 20, 2020
@ghost

ghost commented Aug 20, 2020

Copy link
Copy Markdown

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

@adityapatwardhan adityapatwardhan merged commit c7781d1 into PowerShell:master Nov 14, 2020
@ghost ghost removed the Review - Needed The PR is being reviewed label Nov 14, 2020
@adityapatwardhan adityapatwardhan added this to the 7.2.0-preview.2 milestone Nov 14, 2020
@ghost

ghost commented Dec 15, 2020

Copy link
Copy Markdown

@ghost ghost mentioned this pull request Dec 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes
@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw
@TylerLeonhardt TylerLeonhardt Awaiting requested review from TylerLeonhardt
+2 more reviewers
@PaulHigin PaulHigin PaulHigin requested changes
@vexx32 vexx32 vexx32 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-Test Indicates that a PR should be marked as a test change in the Change Log WG-Quality-Test issues in a test or in test infrastructure

Projects

None yet

Milestone

7.2.0-preview.2

Development

Successfully merging this pull request may close these issues.

The 'TraceRoute' test take 44 minutes to fail in macOS CI

4 participants

@rjmholt @adityapatwardhan @PaulHigin @vexx32