fix(iroh-net): Fix some flaky magicsock tests #2034
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 works for me, but used to be flaky. The way it is setup now should give use useful logs in case it fails.
this is somewhat more useful now that we use nextest
This is likely the bug that made all of these tests flaky. Let's see.
flub
changed the title
| @@ -227,7 +227,11 @@ impl Debug for PublicKey { | |||
|
|
|||
| impl Display for PublicKey { | |||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |||
| write!(f, "{}", base32::fmt(self.as_bytes())) | |||
| if f.alternate() { | |||
There was a problem hiding this comment.
when/how is this triggered?
There was a problem hiding this comment.
When you use a # formatter: {:#} (or {:#?} for Debug)
There was a problem hiding this comment.
shouldn’t it be the other way around then,? # prints longer more detailed messages
There was a problem hiding this comment.
I could be rather tempted to switch this around and do the short format by default and the long format for alternate. That would reduce rather a lot of verbosity in our code.
There was a problem hiding this comment.
the issue is that, even externally, we rely on to_string() roundtriping
There was a problem hiding this comment.
yeah, it'd probably break things if this can't be round-tripped. so maybe we'll have to settle for this.
dignifiedquire
approved these changes
Feb 21, 2024
flub
changed the title
fubuloubu
pushed a commit
to ApeWorX/iroh
that referenced
this pull request
Feb 21, 2024
## Description Fix flaky tests in iroh-net (hopefully). The main issue was that the testing code meshing together the sockets had a race-condition and would sometimes fail. Other improvements: - Logging improvements of various parts. These tests should now provide better info on failure. - Add alternative formatting of NodeId to use the short format. - Use NodeId in the public API, rather than PublicKey. - Move multi-threaded logging setup to the testing utils crate now that it can work properly. More code will have to switch. ## Notes & open questions The local_endpoints_change API is really difficult to use correctly. Even the way this testing code uses it now is wrong. Instead of the two current APIs there should be a single API which returns a stream. I'll do this as a separate followup PR though. ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant.
matheus23
pushed a commit
that referenced
this pull request
Nov 14, 2024
## Description Fix flaky tests in iroh-net (hopefully). The main issue was that the testing code meshing together the sockets had a race-condition and would sometimes fail. Other improvements: - Logging improvements of various parts. These tests should now provide better info on failure. - Add alternative formatting of NodeId to use the short format. - Use NodeId in the public API, rather than PublicKey. - Move multi-threaded logging setup to the testing utils crate now that it can work properly. More code will have to switch. ## Notes & open questions The local_endpoints_change API is really difficult to use correctly. Even the way this testing code uses it now is wrong. Instead of the two current APIs there should be a single API which returns a stream. I'll do this as a separate followup PR though. ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant.
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.
Description
Fix flaky tests in iroh-net (hopefully). The main issue was that the
testing code meshing together the sockets had a race-condition and
would sometimes fail.
Other improvements:
Logging improvements of various parts. These tests should now
provide better info on failure.
Add alternative formatting of NodeId to use the short format.
Use NodeId in the public API, rather than PublicKey.
Move multi-threaded logging setup to the testing utils crate now
that it can work properly. More code will have to switch.
Notes & open questions
The local_endpoints_change API is really difficult to use correctly.
Even the way this testing code uses it now is wrong. Instead of the
two current APIs there should be a single API which returns a stream.
I'll do this as a separate followup PR though.
Change checklist