Make the test signer easier to use in parallel tests #2373
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is especially useful in test code when you want to be able to bring up local servers in your tests without conflicting with a test that might be running in parallel. Currently, the tests tagged challengetest fail on CI when they are run in parallel with each other because only one test is able to bring up a Signer on port 1234. The others silently fail to bring up a signer, and the signer brought up by the first one may not still be around when subsequently started parallel tests need one.
This makes the linter happy.
Previously, tests running in parallel would interfere with each other because the hard-coded signer port (1234) could only be bound in one of the two tests. Now, the signer starts on an operating-system provided unused port.
cla-bot
bot
added
the
s
These are fully expected and shouldn't be considered errors.
`!=` would have missed when those errors were wrapped.
gligneul
reviewed
Jun 7, 2024
Both the httpServer struct's Addr and the SignerURL function can just use the address directly.
This allows us to know the port we're listening on from the moment of construction, but only start the server when Start is called.
|
Okay. This change now uses the new net.FreeTCPPortListener() function and uses the At this point, I'm "mildly" bothered that simply constructing the SigningServer also starts listening on the port. I think it's a bit counter-intuitive. If reviewers agree, I'll move the call to |
gligneul
previously approved these changes
Jun 10, 2024
tsahee
reviewed
Jun 11, 2024
Specifically: 1. Move the net package to util/testhelpers. 2. Remove the concurrency test. 3. Rename the SignerServer listner member.
gligneul
approved these changes
Jun 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously, tests running in parallel would interfere with each other because the hard-coded signer port (1234) could only be bound in one of the two tests.
Now, the signer starts on an operating-system provided unused port.