-
Notifications
You must be signed in to change notification settings - Fork 41
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
chore: use graceful timeout mechanism #1841
Conversation
size-limit report 📦
|
|
||
export function withGracefulTimeout( | ||
asyncOperation: () => Promise<void>, | ||
timeoutDuration: number, |
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.
let's make it same as MOCHA_HOOK_MAX_TIMEOUT
global config and optional to pass
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.
Yes, makes sense
import { Logger } from "@waku/utils"; | ||
const log = new Logger("test:mocha-hook"); | ||
|
||
export function withGracefulTimeout( |
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.
to decrease amount of lines changed - I want to propose following:
export const beforeEachCustom = function (self, cb: Promise<void>, setter: any) {
self.beforeEach((done) => {
this.timeout(MOCHA_HOOK_MAX_TIMEOUT);
withGracefulTimeout(async () => {
const [nwaku, waku] = await runMultipleNodes(
this,
[DefaultPubsubTopic],
strictCheckNodes
);
subscription = await waku.filter.createSubscription();
setter({ nwaku, waku/*, nwaku1, ...etc */});
}, 20000, done);
});
});
do you think it would work, @fbarbu15 ?
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.
I did made it work but there are multiple functions that we need to wrap:
runMultipleNodes
runNodes (2 of them)
tearDownNodes
teardownNodesWithRedundancy
and direct usage of
nwaku.start + other commands like subscribe
So we would probably need to create multiple customFunctions (we will complicate stuff even further) or unify the mentioned methods to have only one general runNodes function and only one general tearDownNodes function (this one looks like a big refactoring but it may be worth it)
Wdyt?
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.
I think this is solvable the same way:
in utils file:
export const beforeEachCustom = function (self, cb: Promise<void>) {
self.beforeEach((done) => {
this.timeout(MOCHA_HOOK_MAX_TIMEOUT);
withGracefulTimeout(cb, 20000, done);
});
});
export const afterEachCustom = function (self, cb: Promise<void>, setter: any) {
self.afterEach((done) => {
this.timeout(MOCHA_HOOK_MAX_TIMEOUT);
withGracefulTimeout(cb, 20000, done);
});
});
in test file:
describe("test", () => {
let nwaku, waku;
beforeEachCustom(async () => {
[nwaku, waku] = await runMultipleNodes(
this,
[DefaultPubsubTopic],
strictCheckNodes
);
subscription = await waku.filter.createSubscription();
});
afterEachCustom(async () => {
await teardownNodesWithRedundancy(serviceNodes, waku);
});
test("test case", () => {});
});
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.
worked with some change regarding global timeout restoring
…ocha-hooks-timeouts
…ocha-hooks-timeouts
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.
LGTM
Problem
Looking at the flaky tests from the last months it seems most are failing at beforeEach and afterEach hooks, usually with a Timeout error when staring or stopping the nwaku container.
After investigating it seems that mocha retry mechanism only retries the test if the failure was in the test itself (inside it ) and it fails immediately, without any retry, if the failure is in those beforeEach/afterEach hooks.
Solution
To address those failures I've increased the timeouts for all beforeEach and afterEach hooks
Also added a function to gracefully handle timeouts in the staring/stopping of the nwaku containers, and making the hook pass in such cases, so the failure gets detected in the test itself and hence triggering the retry mechanism.