p2p/simulations: fix flaky test TestHTTPNodeRPC #30245
Conversation
|
I don't think this is a great way to solve this test. A few would be to extend the |
There is a race condition between subscribing and generating an event in the test. The TestAPI now exposes the number of active subscriptions so clients can ensure their subscription is active before calling the api.
bearpebble
force-pushed
the
p2p-simulations-fix-flaky-test
branch
from
9bf6995
to
33dbe85
Compare
|
Hey @lightclient, thanks for the feedback. I agree that it's not a great way to solve it. Performing a loop until subscriptions start working also feels kind of wrong though, since it could mask some rpc calls not working properly. I was initially thinking about keeping a map of active subscriptions in the I also saw your other PR about removing the directory completely, so if that goes through or you the approach sucks, feel free to close this PR. |
| state *atomic.Value | ||
| peerCount *int64 | ||
| counter int64 | ||
| activeSubscriptions int64 |
There was a problem hiding this comment.
I think you want sync.WaitGroup here. Also maybe just subCount, no need to be overly verbose.
| if err := rpcClient1.CallContext(ctx, &expectedActiveSubscriptions, "test_getNumActiveSubscriptions"); err != nil { | ||
| t.Fatalf("error calling RPC method: %s", err) | ||
| } | ||
| expectedActiveSubscriptions += 1 |
There was a problem hiding this comment.
Are you not able to know statically the expected number of active subs? (1) ?
| @@ -565,6 +581,22 @@ func TestHTTPNodeRPC(t *testing.T) { | |||
| } | |||
| defer sub.Unsubscribe() | |||
|
|
|||
| // make sure the subscription becomes active | |||
| var numActiveSubscriptions int64 | |||
| for i := 0; i < 3; i++ { | |||
There was a problem hiding this comment.
I think you should do a for and select loop with a timeout here. You're basically saying "timeout after 300 milliseconds" here, but in a more complicated way.
|
p2p/simulations has been deleted in #30250 |
There is a race condition between subscribing and generating an event in the test. This PR extends the
TestAPIto expose the number of active subscriptions, allowing clients to wait until their subscription becomes active.Tested via
See issue #29830