GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Reinstate Test-Connection tests by rjmholt · Pull Request #13324 · PowerShell/PowerShell

Conversation

@rjmholt

@rjmholt Rob Holt (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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
  • Breaking changes
    • None
    • OR
    • Experimental feature(s) needed
      • Experimental feature name(s):
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed
      • Issue filed:
  • Testing - New and feature
    • N/A or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
  • Tooling
    • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
    • OR
    • I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
      • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
      • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ghost ghost assigned Aditya Patwardhan (adityapatwardhan) Jul 31, 2020
@rjmholt Rob Holt (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
Rob Holt (rjmholt) requested review from TylerLeonhardt, Dongbo Wang (daxian-dbw) and Rain Sallow (/u/ta11ow) (vexx32) July 31, 2020 00:08
@rjmholt

Rob Holt (rjmholt) commented Jul 31, 2020

Copy link
Copy Markdown
Collaborator Author

Paul Higinbotham (@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'.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rjmholt Rob Holt (rjmholt) mentioned this pull request Jul 31, 2020
macOS DNS not resolving own hostname actions/runner-images#1335
Closed
1 of 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
Paul Higinbotham (PaulHigin) suggested changes Aug 3, 2020
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1 Outdated

@PaulHigin Paul Higinbotham (PaulHigin) Aug 3, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1 Outdated

@PaulHigin Paul Higinbotham (PaulHigin) Aug 3, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 3, 2020

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vexx32
Rain Sallow (/u/ta11ow) (vexx32) approved these changes Aug 3, 2020
View reviewed changes

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this 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. 🙂

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1 Outdated

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 3, 2020

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@rjmholt

Rob Holt (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
PaulHigin and vexx32 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
Rob Holt (rjmholt) added 4 commits August 12, 2020 10:04
@rjmholt
Move tests back to using hostname
68e3780
@rjmholt
$realAddress -> $hostAddress
a050341
@rjmholt
Go back to $testAddress
a7c6c40
@rjmholt
Move MTU tests to targetAddress
4c843fa
@rjmholt
Rob Holt (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
Remove commented out code
cc758ef
@rjmholt Rob Holt (rjmholt) changed the title Move Test-Connection tests back to using hostname Reinstate Test-Connection tests Aug 12, 2020
@rjmholt
Rob Holt (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

Deleted user (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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Nov 14, 2020
View reviewed changes
@adityapatwardhan
Aditya Patwardhan (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 Aditya Patwardhan (adityapatwardhan) added this to the 7.2.0-preview.2 milestone Nov 14, 2020
@ghost

Deleted user (ghost) commented Dec 15, 2020

Copy link
Copy Markdown

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

Handy links:

  • Release Notes
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost ghost mentioned this pull request Dec 15, 2020
The 'TraceRoute' test take 44 minutes to fail in macOS CI #13309
Closed
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@rjmholt
Reinstate Test-Connection tests (PowerShell#13324)
528b517
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw
@TylerLeonhardt TylerLeonhardt Awaiting requested review from TylerLeonhardt
+2 more reviewers
@PaulHigin Paul Higinbotham (PaulHigin) PaulHigin requested changes
@vexx32 Rain Sallow (/u/ta11ow) (vexx32) vexx32 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@adityapatwardhan Aditya Patwardhan (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

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@rjmholt @adityapatwardhan @PaulHigin @vexx32
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.