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

tests(iroh): Add some context to test errors #3066

Merged
merged 2 commits into from
Jan 9, 2025
Merged

tests(iroh): Add some context to test errors #3066

merged 2 commits into from
Jan 9, 2025

Conversation

flub
Copy link
Contributor

@flub flub commented Dec 20, 2024

Description

The flaky tests are being super weird and making no sense. This is
clutching at straws for any hint.

Breaking Changes

Notes & open questions

/cc @ramfox

Two slightly odd things I spotted while looking at this:

  • The use of 240.0.0.1 as "bad" IP address returned in discovery. Maybe one from the test nets in RFC 5737 would be better?
  • The endpoints accept a connection and immediately drop it. This could potentially send the CONNECTION_CLOSE before the ACK from the connection resulting in the connect call returning an error (BUT THAT WOULD BE A NORMAL READABLE TEST FAILURE). This probably doesn't happen because it's all local and very fast and Quinn has already ACKed the connection before the code manages to drop it. Still, essentially racy code.

Fine, I added a fix for the 2nd. But it's not the issue.

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

The flaky tests are being super weird and making no sense.  This is
clutching at straws for any hint.
Copy link

github-actions bot commented Dec 20, 2024

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3066/docs/iroh/

Last updated: 2024-12-20T08:34:21Z

Copy link

github-actions bot commented Dec 20, 2024

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 062d9a8

@flub flub requested review from a team and ramfox and removed request for a team December 20, 2024 08:33
@flub
Copy link
Contributor Author

flub commented Jan 8, 2025

@ramfox ping

@matheus23
Copy link
Member

I think the code you're modifying git blames back to me, actually. So I'll just go ahead and comment on this ✌️

The flaky tests are being super weird and making no sense. This is
clutching at straws for any hint.

I'd love to know what specific errors you're trying to fix, so if you could link to the failing run you were looking at, that'd be great.

Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

Okay, so I think what you're trying is to be better prepared, should the flake occur again, do I understand that correctly?

If so, I think this can just be merged.

@flub
Copy link
Contributor Author

flub commented Jan 9, 2025

I think the code you're modifying git blames back to me, actually. So I'll just go ahead and comment on this ✌️

The flaky tests are being super weird and making no sense. This is
clutching at straws for any hint.

I'd love to know what specific errors you're trying to fix, so if you could link to the failing run you were looking at, that'd be great.

Yeah, classic mistake of assuming it's obvious and now it's lost somewhere... but still, if it comes back this should make understanding it a bit easier.

@flub flub added this pull request to the merge queue Jan 9, 2025
Merged via the queue into main with commit 1ae820d Jan 9, 2025
27 checks passed
@matheus23 matheus23 deleted the flub/test-context branch January 9, 2025 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants