Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughTest stability improvements and code refactoring across multiple components. Changes include batching concurrent test operations, replacing direct field access with public methods, adding shutdown timing delays, and loosening assertions to handle race conditions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
janezpodhostnik
requested review from
a team,
fxamacker,
holyfuchs,
peterargue and
zhangchiqing
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@network/test/cohort2/unicast_authorization_test.go`:
- Around line 466-470: Fix the comment typo above the require.ErrorContains
check: change "strem reset" to "stream reset" in the inline comment that
explains the receiver-reset case (the comment immediately preceding the
require.ErrorContains(u.T(), err, "stream reset", ...)). Ensure the corrected
spelling matches the string being asserted so the comment and assertion stay
consistent.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| "fmt" | ||
| "sync" | ||
|
|
||
| lru "github.com/hashicorp/golang-lru/v2" |
There was a problem hiding this comment.
I'm attempting to fix this in another PR by completely refactoring the connection cache:
#7859
it needs some more rounds of review though.
my preference is to leave this logic as is, and push that PR forward to fix the tests.
There was a problem hiding this comment.
I'll take a look 👀.
Would you be ok with adding a delay in the test to temporarily address the issue?
Are you ok with the other two?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@engine/access/rpc/connection/cache_test.go`:
- Around line 180-188: The test spawns goroutines that call require.NoError
which calls t.FailNow and must only run in the main test goroutine; replace the
in-goroutine require.NoError usage in the goroutine that calls
cache.GetConnected (and the similar case in the first subtest near
cachedConn.AddRequest/connectFn) by either using assert.NoError and returning
from the goroutine on error or sending the error back on a channel/atomic and
having the main test goroutine fail; specifically update the anonymous goroutine
that calls cache.GetConnected("foo", cfg, nil, connectFn) to check the error
without t.FailNow (use assert.NoError from testify or propagate the err to the
main goroutine and handle fail there) and bail the goroutine early if an error
occurs.
🧹 Nitpick comments (1)
engine/access/rpc/connection/cache_test.go (1)
202-204: Lower bound assertion is very loose — consider tightening.Since each of the 100 batches starts after the previous batch's
invalidate("foo"), every batch must create at least one new connection. The lower bound could beint32(numBatches)instead ofint32(1), which would make the test more meaningful without risking flakiness.Proposed tighter bound
- assert.Greater(t, callCount.Load(), int32(1)) + assert.GreaterOrEqual(t, callCount.Load(), int32(numBatches))
| go func() { | ||
| defer wg.Done() | ||
| cachedConn, err := cache.GetConnected("foo", cfg, nil, connectFn) | ||
| require.NoError(t, err) | ||
|
|
||
| done := cachedConn.AddRequest() | ||
| time.Sleep(1 * time.Millisecond) | ||
| done() | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the test file
find . -name "cache_test.go" -path "*/engine/access/rpc/connection/*"Repository: onflow/flow-go
Length of output: 102
🏁 Script executed:
# Examine the specific lines mentioned (180-188 and surrounding context)
sed -n '170,195p' engine/access/rpc/connection/cache_test.goRepository: onflow/flow-go
Length of output: 664
🏁 Script executed:
# Also check line 149 mentioned in the note
sed -n '140,160p' engine/access/rpc/connection/cache_test.goRepository: onflow/flow-go
Length of output: 736
🏁 Script executed:
# Search for all goroutines + require patterns in this file
rg -n "go func" -A 15 engine/access/rpc/connection/cache_test.go | head -100Repository: onflow/flow-go
Length of output: 2885
🏁 Script executed:
# Check imports to see if both require and assert are available
head -30 engine/access/rpc/connection/cache_test.goRepository: onflow/flow-go
Length of output: 785
require.NoError inside a goroutine can cause undefined test behavior.
require.NoError calls t.FailNow(), which invokes runtime.Goexit(). The testing package explicitly states this must only be called from the goroutine running the test function. In a spawned goroutine, it can cause a panic, a hang, or a silently passing test.
Use assert.NoError and return early instead, or collect the error via a channel/atomic.
Note: the same pattern exists at line 149 in the unchanged first subtest.
Proposed fix
go func() {
defer wg.Done()
cachedConn, err := cache.GetConnected("foo", cfg, nil, connectFn)
- require.NoError(t, err)
+ if !assert.NoError(t, err) {
+ return
+ }
done := cachedConn.AddRequest()
time.Sleep(1 * time.Millisecond)
done()
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go func() { | |
| defer wg.Done() | |
| cachedConn, err := cache.GetConnected("foo", cfg, nil, connectFn) | |
| require.NoError(t, err) | |
| done := cachedConn.AddRequest() | |
| time.Sleep(1 * time.Millisecond) | |
| done() | |
| }() | |
| go func() { | |
| defer wg.Done() | |
| cachedConn, err := cache.GetConnected("foo", cfg, nil, connectFn) | |
| if !assert.NoError(t, err) { | |
| return | |
| } | |
| done := cachedConn.AddRequest() | |
| time.Sleep(1 * time.Millisecond) | |
| done() | |
| }() |
🤖 Prompt for AI Agents
In `@engine/access/rpc/connection/cache_test.go` around lines 180 - 188, The test
spawns goroutines that call require.NoError which calls t.FailNow and must only
run in the main test goroutine; replace the in-goroutine require.NoError usage
in the goroutine that calls cache.GetConnected (and the similar case in the
first subtest near cachedConn.AddRequest/connectFn) by either using
assert.NoError and returning from the goroutine on error or sending the error
back on a channel/atomic and having the main test goroutine fail; specifically
update the anonymous goroutine that calls cache.GetConnected("foo", cfg, nil,
connectFn) to check the error without t.FailNow (use assert.NoError from testify
or propagate the err to the main goroutine and handle fail there) and bail the
goroutine early if an error occurs.
3 participants
This fix is three fixes in a trench coat:
closeRequestedSummary by CodeRabbit
Tests
Bug Fixes