Skip to content

Fix translation of Ping error codes - #63237

Merged
wfurt merged 8 commits into
dotnet:mainfrom
deeprobin:issue-62592
Jan 8, 2022
Merged

Fix translation of Ping error codes#63237
wfurt merged 8 commits into
dotnet:mainfrom
deeprobin:issue-62592

Conversation

@deeprobin

@deeprobindeeprobin commented Dec 31, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Fix issue #62592 (closes#62592)

Current state of implementation

  • Apply fix
  • Modify tests (requires some infrastructure owner)

/cc @filipnavara

@ghostghost added area-System.Net community-contribution Indicates that the PR has been added by a community member labels Dec 31, 2021
@ghost

ghost commented Dec 31, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix issue #62592 (closes #62592)

Current state of implementation

  • Apply fix
  • Modify tests

/cc @filipnavara

Author:deeprobin
Assignees:-
Labels:

area-System.Net

Milestone:-

@filipnavara

filipnavara commented Dec 31, 2021

Copy link
Copy Markdown
Member

Implementation looks good 👍 Now just the (arguably annoying) test...

wfurt
wfurt reviewed Dec 31, 2021
View reviewed changes

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

generally looks good. I left some comments.

Comment threadsrc/libraries/System.Net.Ping/src/System/Net/NetworkInformation/IcmpV4MessageConstants.cs Outdated
Comment threadsrc/libraries/System.Net.Ping/src/System/Net/NetworkInformation/IcmpV6MessageConstants.cs Outdated
Comment threadsrc/libraries/System.Net.Ping/tests/FunctionalTests/PingTest.cs Outdated
@deeprobin

deeprobin commented Jan 6, 2022

Copy link
Copy Markdown
ContributorAuthor

I'll mark this PR as Ready-For-Review for once, since the actual change is ready.

The only problem is the tests for which we need an infrastructure owner to give us more insight about the tests.

@deeprobin
deeprobin marked this pull request as ready for review January 6, 2022 11:17
wfurt
wfurt approved these changes Jan 8, 2022
View reviewed changes

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@wfurt
wfurt merged commit 73a34ce into dotnet:mainJan 8, 2022
@karelzkarelz added this to the 7.0.0 milestone Jan 11, 2022
@ghostghost locked as resolved and limited conversation to collaborators Feb 10, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurtwfurtwfurt approved these changes

Assignees

No one assigned

Labels

area-System.Netcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Incorrect translation of Ping error codes

4 participants

@deeprobin@filipnavara@wfurt@karelz