Skip to content
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

Fix Connect_DualMode_DnsConnect_RetrievedEndPoints_Success on Linux #54681

Merged
merged 4 commits into from
Jun 29, 2021
Merged

Fix Connect_DualMode_DnsConnect_RetrievedEndPoints_Success on Linux #54681

merged 4 commits into from
Jun 29, 2021

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Jun 24, 2021 •
edited
Loading

50 failures in ~20 days. See #54677 (comment).

/cc @tmds

Fixes #54677

@antonfirsov antonfirsov requested a review from a team June 24, 2021 12:26
@ghost
Copy link

ghost commented Jun 24, 2021

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

Issue Details

50 failures in ~20 days. See #54677 (comment).

/cc @tmds

Author: antonfirsov
Assignees: -
Labels:

area-System.Net.Sockets
Milestone: -

tmds
tmds reviewed Jun 24, 2021
View reviewed changes
src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs Outdated
@@ -109,6 +109,7 @@ public async Task Connect_DualMode_MultiAddressFamilyConnect_RetrievedEndPoints_
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/54677", TestPlatforms.Linux)]
Copy link
Member

@tmds tmds Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@antonfirsov can you update the test so both IPAddress.Loopback.MapToIPv6() and IPAddress.IPv6Loopback are allowed for localEndPoint.Address?

I expect for remoteEndPoint.Address we should stick to IPAddress.Loopback.MapToIPv6() only because the test server is bound to IPAddress.Loopback.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. I overlooked the exception message, and for some reason thought LocalEndPoint was IPv4, not ::1.
Makes sense to fix the test instead of disabling.

@antonfirsov
Copy link
Member Author

I updated this to fix #54677, instead of disabling, let's wait for CI before merging.

@antonfirsov antonfirsov changed the title Disable Connect_DualMode_DnsConnect_RetrievedEndPoints_Success on Linux Fix Connect_DualMode_DnsConnect_RetrievedEndPoints_Success on Linux Jun 24, 2021
@antonfirsov
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@danmoseley
Copy link
Member

@stephentoub this looks related to your change:

Assert.Throws() Failure
Expected: typeof(System.OperationCanceledException)
Actual:   (No exception was thrown)


Stack trace
   at System.Threading.Tests.PeriodicTimerTests.WaitForNextTickAsync_CanceledWaitThenWaitAgain_Succeeds() in /_/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs:line 186
--- End of stack trace from previous location ---

https://dev.azure.com/dnceng/public/_build/results?buildId=1204988&view=ms.vss-test-web.build-test-results-tab&runId=36060014&resultId=173000&paneView=debug

@runfoapp runfoapp bot mentioned this pull request Jun 25, 2021
Closed
@stephentoub
Copy link
Member

@danmoseley, that's #54713

@danmoseley
Copy link
Member

@stephentoub thanks, I pasted in the wrong output.

@antonfirsov @tmds
Update src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs
72c3f12 
Co-authored-by: Tom Deseyn <tom.deseyn@gmail.com>
tmds
tmds approved these changes Jun 29, 2021
View reviewed changes
Copy link
Member

@tmds tmds left a comment

Choose a reason for hiding this comment

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

LGTM

@antonfirsov antonfirsov merged commit 1b61065 into main Jun 29, 2021
@jkotas jkotas deleted the antonfirsov/disable-Connect_DualMode_DnsConnect_RetrievedEndPoints_Success branch July 1, 2021 04:33
@karelz karelz added this to the 6.0.0 milestone Jul 15, 2021
This was referenced Jul 20, 2021
Closed
Closed
Closed
Closed
@antonfirsov antonfirsov mentioned this pull request Jul 23, 2021
Merged
@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@karelz karelz karelz approved these changes

@tmds tmds tmds approved these changes

Assignees
No one assigned
Labels
area-System.Net.Sockets
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Connect_DualMode_DnsConnect_RetrievedEndPoints_Success - Assert ::1
5 participants
@antonfirsov @danmoseley @stephentoub @tmds @karelz