Skip to content

Conversation

@adam-cattermole
Copy link
Member

@adam-cattermole adam-cattermole commented Jan 26, 2026

Exposed waiting on the waitgroup instead of the fixed time which seems to be inconsistent/insufficient in the runners

Signed-off-by: Adam Cattermole <a.d.cattermole@gmail.com>
@codecov
Copy link

codecov bot commented Jan 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.14%. Comparing base (41a4f2b) to head (ccc7e0b).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1744      +/-   ##
==========================================
+ Coverage   74.91%   75.14%   +0.22%     
==========================================
  Files         120      120              
  Lines       10287    10369      +82     
==========================================
+ Hits         7707     7792      +85     
- Misses       2206     2211       +5     
+ Partials      374      366       -8     
Flag Coverage Δ
bare-k8s-integration 24.27% <0.00%> (+0.58%) ⬆️
controllers-integration 59.35% <0.00%> (-0.10%) ⬇️
envoygateway-integration 44.37% <0.00%> (+0.01%) ⬆️
gatewayapi-integration 19.21% <0.00%> (+0.25%) ⬆️
istio-integration 48.97% <0.00%> (+0.20%) ⬆️
unit 22.17% <100.00%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
api (u) 85.88% <ø> (ø)
internal (u) 76.91% <89.81%> (+0.24%) ⬆️
pkg (u) 31.00% <ø> (ø)
Files with missing lines Coverage Δ
internal/extension/oop.go 49.12% <100.00%> (+0.90%) ⬆️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@adam-cattermole adam-cattermole self-assigned this Jan 26, 2026
@adam-cattermole adam-cattermole moved this to In Progress in Kuadrant Jan 26, 2026
@adam-cattermole adam-cattermole changed the title Monitor the wait group for completion Fix flakey OOP log test Jan 26, 2026
@adam-cattermole adam-cattermole marked this pull request as ready for review January 26, 2026 12:14
@adam-cattermole adam-cattermole requested a review from a team as a code owner January 26, 2026 12:14
@adam-cattermole adam-cattermole moved this from In Progress to Ready For Review in Kuadrant Jan 26, 2026
didierofrivia
didierofrivia previously approved these changes Jan 26, 2026
Copy link
Member

@didierofrivia didierofrivia left a comment

Choose a reason for hiding this comment

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

much better approach! 🥇

@adam-cattermole
Copy link
Member Author

Damn it isn't fixed; managed to get it to fail on the 9th run..

Comment on lines 109 to 110
// wait for stderr
p.monitorWg.Wait()
Copy link
Member Author

@adam-cattermole adam-cattermole Jan 26, 2026

Choose a reason for hiding this comment

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

I think this is the problem.. I managed to reproduce:

"msg"="failed to read stderr" "error"="read |0: file already closed"

I think the cmd.Wait is closing the process and we're losing the stderr before the scanner gets scheduled/reads it

Signed-off-by: Adam Cattermole <a.d.cattermole@gmail.com>
@adam-cattermole
Copy link
Member Author

adam-cattermole commented Jan 26, 2026

Okay, if you run this you should be able to get it to reproduce the error (we use separate processes so that it makes the machine more busy):

cd internal/extension && for i in {1..16}; do go test -run TestOOPExtensionForwardsLog -count=1000 -parallel=16 -failfast & done; wait

I seem to be able to pretty reliably get this to error on main but haven't hit it yet with the latest changes

(They're quite a bit slower to run on main because that 50ms wait is adding up across all runs)

Copy link
Contributor

@KevFan KevFan left a comment

Choose a reason for hiding this comment

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

Nice! Ran the command and the CI a good few times and all looking good 💯

image

@adam-cattermole adam-cattermole added this pull request to the merge queue Jan 26, 2026
Merged via the queue into main with commit c254528 Jan 26, 2026
193 checks passed
@adam-cattermole adam-cattermole deleted the fix-flakey-oop-test branch January 26, 2026 17:12
@github-project-automation github-project-automation bot moved this from Ready For Review to Done in Kuadrant Jan 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants