Skip to content

Reinstate Test-Connection tests #13324

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.

Sign up for GitHub

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

Jump to bottom
Merged
adityapatwardhan merged 5 commits into from
Nov 14, 2020
Merged

Reinstate Test-Connection tests #13324

adityapatwardhan merged 5 commits into from
Nov 14, 2020

Conversation

@rjmholt
Copy link
Collaborator

@rjmholt rjmholt commented Jul 31, 2020

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

rjmholt commented Jul 31, 2020

@PaulHigin I'm seeing a remoting test failure here in the elevated Windows tests I think:

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at System.Management.Automation.Remoting.PrioritySendDataCollection.ReadOrRegisterCallback(OnDataAvailableCallback callback, DataPriorityType& priorityType) in D:\a\1\s\src\System.Management.Automation\engine\remoting\fanin\PriorityCollection.cs:line 240
   at System.Management.Automation.Remoting.Client.WSManClientCommandTransportManager.SendOneItem() in D:\a\1\s\src\System.Management.Automation\engine\remoting\fanin\WSManTransportManager.cs:line 3975
   at System.Management.Automation.Remoting.Client.WSManClientCommandTransportManager.OnRemoteCmdSendCompleted(IntPtr operationContext, Int32 flags, IntPtr error, IntPtr shellOperationHandle, IntPtr commandOperationHandle, IntPtr operationHandle, IntPtr data) in D:\a\1\s\src\System.Management.Automation\engine\remoting\fanin\WSManTransportManager.cs:line 3681
An error has occurred that was not properly handled. Additional information is shown below. The PowerShell process will exit.
Execution of {& $powershell $PSFlags -c $command} by build.psm1: line 1332 failed with exit code -1073741819
At D:\a\1\s\build.psm1:2192 char:17
+                 throw $errorMessage
+                 ~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : OperationStopped: (Execution of {& $po…it code -1073741819:String) [], RuntimeException
+ FullyQualifiedErrorId : Execution of {& $powershell $PSFlags -c $command} by build.psm1: line 1332 failed with exit code -1073741819
##[error]PowerShell exited with code '1'.

@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
test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1 Outdated
Copy link
Contributor

@PaulHigin PaulHigin Aug 3, 2020

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?

test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1 Outdated
Copy link
Contributor

@PaulHigin PaulHigin Aug 3, 2020

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

Copy link
Collaborator

@vexx32 vexx32 Aug 3, 2020

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
Copy link
Collaborator

@vexx32 vexx32 left a comment

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

test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1 Outdated
Copy link
Collaborator

@vexx32 vexx32 Aug 3, 2020

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

rjmholt commented Aug 3, 2020

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

ghost commented Aug 20, 2020

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

ghost commented Dec 15, 2020

🎉v7.2.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

@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