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.

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

More testing of withListeningSocket #2069

Merged
merged 2 commits into from
Sep 28, 2020
Merged

Conversation

rvl
Copy link
Contributor

@rvl rvl commented Aug 25, 2020

Issue Number

This might fix #2067.

Overview

I initially had a theory that the function withListeningSocket only bound the socket, but did not start listening. Therefore, connections to the port which were made during the withListeningSocket action, but before the warp server starts listening, would be refused. However this was incorrect.

This PR adds another test case to check that the socket really is listening.

Comments

@rvl rvl self-assigned this Aug 25, 2020
@rvl rvl added the RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG label Aug 25, 2020
@Anviking
Copy link
Member

cardano-wallet-core-test-unit.exe --match "/Cardano.Wallet.Api.Server.Tls/TLS Client Authentication/Deny client with wrong certificate if TLS is enabled/"

With the binary from #2064 (and master I suspect) it failed, but with this fix, it passes for me.

@Anviking
Copy link
Member

Anviking commented Aug 25, 2020

Or the fix still failed when I retried a few times.

@rvl rvl force-pushed the rvl/2067/fix-windows-tests branch from 5daa76a to c5dac1d Compare August 26, 2020 04:39
@rvl rvl changed the title Fix race in withListeningSocket More testing of withListeningSocket Aug 26, 2020
@rvl
Copy link
Contributor Author

rvl commented Aug 26, 2020

Turns out my theory about there being a race against withListeningSocket was wrong.

@KtorZ
Copy link
Member

KtorZ commented Aug 26, 2020

These tests are pretty old and were working fine for long. Yet I recently updated them when we bumped to version of the tls package to include support for TLS 1.2+. So there could be some windows specific thing regarding TLS 1.2 🤔 ?

@rvl rvl force-pushed the rvl/2067/fix-windows-tests branch from c5dac1d to 551f0ce Compare August 26, 2020 10:38
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 28, 2020

Build succeeded:

@iohk-bors iohk-bors bot merged commit 002987b into master Sep 28, 2020
@iohk-bors iohk-bors bot deleted the rvl/2067/fix-windows-tests branch September 28, 2020 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cardano-wallet-core:test:unit TlsSpec fails on windows
3 participants