-
-
Notifications
You must be signed in to change notification settings - Fork 412
test: fix few e2e flaky tests #7762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 21 commits
fbbb54f
c21125a
2771067
55f7694
c93bda3
4ca8281
2c4c5ed
16b19f0
cf7e41d
51afe51
516b534
8c09150
22c5513
8019844
365c142
c25f592
0fd48c6
b29b2aa
a07ef3c
d70f69d
158b99a
c742959
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ import {chainConfig} from "@lodestar/config/default"; | |
| import {ForkName} from "@lodestar/params"; | ||
| import {RequestError, RequestErrorCode, ResponseOutgoing} from "@lodestar/reqresp"; | ||
| import {Root, SignedBeaconBlock, altair, phase0, ssz} from "@lodestar/types"; | ||
| import {sleep as _sleep} from "@lodestar/utils"; | ||
| import {sleep as _sleep, sleep} from "@lodestar/utils"; | ||
nazarhussain marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| import {afterEach, beforeEach, describe, expect, it, vi} from "vitest"; | ||
| import {Network, ReqRespBeaconNodeOpts} from "../../../src/network/index.js"; | ||
| import {GetReqRespHandlerFn, ReqRespMethod} from "../../../src/network/reqresp/types.js"; | ||
|
|
@@ -33,24 +33,21 @@ function runTests({useWorker}: {useWorker: boolean}): void { | |
| ...chainConfig, | ||
| ALTAIR_FORK_EPOCH: 0, | ||
| }); | ||
| let controller: AbortController; | ||
|
|
||
| const afterEachCallbacks: (() => Promise<void> | void)[] = []; | ||
|
|
||
| beforeEach(() => { | ||
| controller = new AbortController(); | ||
| }); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does it make sense to abort the controller in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Had it before but does not make sense of it with these changes. Mostly it's used for sleep and peer connection, if any of those hangs up the test will already timeout. |
||
|
|
||
| afterEach(async () => { | ||
| while (afterEachCallbacks.length > 0) { | ||
| const callback = afterEachCallbacks.pop(); | ||
| if (callback) await callback(); | ||
| } | ||
| }); | ||
|
|
||
| let controller: AbortController; | ||
| beforeEach(() => { | ||
| controller = new AbortController(); | ||
| }); | ||
| afterEach(() => controller.abort()); | ||
| async function sleep(ms: number): Promise<void> { | ||
| await _sleep(ms, controller.signal); | ||
| } | ||
|
|
||
| async function createAndConnectPeers( | ||
| getReqRespHandler?: GetReqRespHandlerFn, | ||
| opts?: ReqRespBeaconNodeOpts | ||
|
|
@@ -68,11 +65,15 @@ function runTests({useWorker}: {useWorker: boolean}): void { | |
| await closeA(); | ||
| await closeB(); | ||
| }); | ||
|
|
||
| const connected = Promise.all([onPeerConnect(netA), onPeerConnect(netB)]); | ||
| await connect(netA, netB); | ||
| await connect(netA, netB, controller.signal); | ||
| await connected; | ||
|
|
||
| controller.signal.addEventListener("abort", async () => { | ||
| await closeA(); | ||
| await closeB(); | ||
| }); | ||
|
|
||
| return [netA, netB, await getPeerIdOf(netA), await getPeerIdOf(netB)]; | ||
| } | ||
|
|
||
|
|
@@ -242,15 +243,15 @@ function runTests({useWorker}: {useWorker: boolean}): void { | |
| ); | ||
| }); | ||
|
|
||
| it("trigger a TTFB_TIMEOUT error", async () => { | ||
| it("should trigger TTFB_TIMEOUT error if first response is delayed", async () => { | ||
| const ttfbTimeoutMs = 250; | ||
|
|
||
| const [netA, _, _0, peerIdB] = await createAndConnectPeers( | ||
| (method) => | ||
| async function* onRequest() { | ||
| if (method === ReqRespMethod.BeaconBlocksByRange) { | ||
| // Wait for too long before sending first response chunk | ||
| await sleep(ttfbTimeoutMs * 10); | ||
| await sleep(ttfbTimeoutMs * 10, controller.signal); | ||
| yield wrapBlockAsEncodedPayload(config, config.getForkTypes(0).SignedBeaconBlock.defaultValue()); | ||
| } | ||
| }, | ||
|
|
@@ -263,20 +264,21 @@ function runTests({useWorker}: {useWorker: boolean}): void { | |
| ); | ||
| }); | ||
|
|
||
| it("trigger a RESP_TIMEOUT error", async () => { | ||
| const respTimeoutMs = 250; | ||
| it("should trigger a RESP_TIMEOUT error if first byte is on time but later delayed", async () => { | ||
| const ttfbTimeoutMs = 250; | ||
| const respTimeoutMs = 300; | ||
|
|
||
| const [netA, _, _0, peerIdB] = await createAndConnectPeers( | ||
| (method) => | ||
| async function* onRequest() { | ||
| if (method === ReqRespMethod.BeaconBlocksByRange) { | ||
| yield getEmptyEncodedPayloadSignedBeaconBlock(config); | ||
| // Wait for too long before sending second response chunk | ||
| await sleep(respTimeoutMs * 5); | ||
| await sleep(respTimeoutMs * 5, controller.signal); | ||
| yield getEmptyEncodedPayloadSignedBeaconBlock(config); | ||
| } | ||
| }, | ||
| {respTimeoutMs} | ||
| {ttfbTimeoutMs, respTimeoutMs} | ||
| ); | ||
|
|
||
| await expectRejectedWithLodestarError( | ||
|
|
@@ -285,16 +287,19 @@ function runTests({useWorker}: {useWorker: boolean}): void { | |
| ); | ||
| }); | ||
|
|
||
| it("Sleep infinite on first byte", async () => { | ||
| it("should trigger TTFB_TIMEOUT error if respTimeoutMs and ttfbTimeoutMs is the same", async () => { | ||
| const ttfbTimeoutMs = 250; | ||
| const respTimeoutMs = 250; | ||
|
|
||
| const [netA, _, _0, peerIdB] = await createAndConnectPeers( | ||
| (method) => | ||
| // biome-ignore lint/correctness/useYield: No need for yield in test context | ||
| async function* onRequest() { | ||
| if (method === ReqRespMethod.BeaconBlocksByRange) { | ||
| await sleep(100000000); | ||
| await sleep(100000000, controller.signal); | ||
| } | ||
| }, | ||
| {respTimeoutMs: 250, ttfbTimeoutMs: 250} | ||
| {respTimeoutMs, ttfbTimeoutMs} | ||
| ); | ||
|
|
||
| await expectRejectedWithLodestarError( | ||
|
|
@@ -303,13 +308,13 @@ function runTests({useWorker}: {useWorker: boolean}): void { | |
| ); | ||
| }); | ||
|
|
||
| it("Sleep infinite on second response chunk", async () => { | ||
| it("should trigger a RESP_TIMEOUT error if first byte is on time but sleep infinite", async () => { | ||
| const [netA, _, _0, peerIdB] = await createAndConnectPeers( | ||
| (method) => | ||
| async function* onRequest() { | ||
| if (method === ReqRespMethod.BeaconBlocksByRange) { | ||
| yield getEmptyEncodedPayloadSignedBeaconBlock(config); | ||
| await sleep(100000000); | ||
| await sleep(100000000, controller.signal); | ||
| } | ||
| }, | ||
| {respTimeoutMs: 250, ttfbTimeoutMs: 250} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a behavior change, but will help to make sure that behavior does not change unintentionally in future.