Skip to content

tests: minor patchbay test improvements#4091

Open
Frando wants to merge 3 commits intomainfrom
Frando/patchbay-fixes
Open

tests: minor patchbay test improvements#4091
Frando wants to merge 3 commits intomainfrom
Frando/patchbay-fixes

Conversation

@Frando
Copy link
Copy Markdown
Member

@Frando Frando commented Apr 9, 2026

Description

  • Collect nextest output in the downloadable dump together with the testdir for easier debugging
  • Remove stale "currently ignored" comments
  • Add explicit connection closing to all tests
  • Increase timeouts for degrade tests

Should make #4086 obsolete, I think

@Frando Frando requested a review from matheus23 April 9, 2026 10:06
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

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

Last updated: 2026-04-09T11:23:26Z

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

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: 98f0096

Copy link
Copy Markdown
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.

I don't understand where we call Endpoint::close in this version.

We should be calling Endpoint::close to ensure we actually properly send the close frame.

With the current version this kinda works out because you can "wait" for the conn.closed().await on the other side "out-of-band" via the barrier as the communication mechanism.

But we don't need the barrier at all, if we just endpoint.close().await on both sides of Pair::run.
You seem to intentionally avoid doing this, what's the reasoning for that?

@n0bot n0bot bot added this to iroh Apr 9, 2026
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Apr 9, 2026
@Frando
Copy link
Copy Markdown
Member Author

Frando commented Apr 9, 2026

I don't understand where we call Endpoint::close in this version.

We don't. We also don't on main. I removed it after a suggestion from @flub that we don't need it, because we are not testing for closing behavior but for holepunching here. Removing the endpoint close makes the tests run faster, too.

All tests await connection.closed() on one side before the run function returns, and the barrier makes sure no endpoint is dropped before both run functions are complete. Afterwards, the test is considered complete, and both endpoints are dropped dead on the floor.

I can change it back to calling endpoint.close() but I think I agree with flub that it is cleaner and faster not to await this if the tests are not about testing shutdown behavior.

@matheus23
Copy link
Copy Markdown
Member

matheus23 commented Apr 9, 2026

We don't. We also don't on main. I removed it after a suggestion from @flub that we don't need it, because we are not testing for closing behavior but for holepunching here. Removing the endpoint close makes the tests run faster, too.

I see. I intentionally added it back, thinking "we don't close properly, so obviously this will take longer than necessary". The idea being here that if we don't close properly, we might not even send the close message, leaving the other side hanging.

Can you add a comment next the barrier use, essentially explaining that we use this as an alternative mechanism to Endpoint::close, so we avoid having to wait for the draining period (which can get quite long in tests with lots of loss & delay).
Otherwise I'll just look at this in a month and think "we should actually call Endpoint::close!".

@flub
Copy link
Copy Markdown
Contributor

flub commented Apr 9, 2026

FWIW I'd love those patchbay close tests on noq ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🏗 In progress

Development

Successfully merging this pull request may close these issues.

3 participants