Skip to content

Conversation

@onelapahead
Copy link
Contributor

@onelapahead onelapahead commented Dec 12, 2025

Occasionally we see builds fail due to "flaky" tests in fftls.

We can recreate locally if we repeatedly run:

 go test -count 1 -v -race ./pkg/fftls/...

We can avoid these failures by:

  1. Using a done channel to signal to the listener routine when its safe to stop, and avoid unnecessary errors/panics/logs
  2. Avoid polluting the logs with EOF's as those are actually expected
  3. Not asserting errors when Writeing, as those might happen if we shutdown right as the listener routine tries to write

Rerunning tests with -race I can't reproduce this issue.

Agentic assistance via cursor_fftls_test_eof_panic_in_gha.md

@onelapahead onelapahead requested a review from a team as a code owner December 12, 2025 14:06
Comment on lines +127 to +129
if err != io.EOF {
t.Logf("read failed: %s", err)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering if we should actually log EOF's here - as they are not expected if we are not <-done now that I think about it.

The bug was that we do a t.Logf after the test completed

Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't make sense, t should still be captured in the context of this go thread..

Copy link
Contributor

Choose a reason for hiding this comment

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

or does it get set to nil by the framework?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry it never gets nil'ed, its just that if you call Logf on a t who's test has completed, it will panic.

That could occur bc this was in a separate Goroutine that might not shut down quick enough as the test concludes. So the <-done helps prevent all that primarily.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see so it's the test framework that panics gotcha

Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

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

Thanks for sorting this one out - has been in there for a long time

@EnriqueL8 EnriqueL8 merged commit c1fccfd into hyperledger:main Dec 15, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants