Skip to content
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

test_runner: Fix mock timer issues #50384

Merged
merged 5 commits into from Nov 12, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 16 additions & 13 deletions lib/internal/test_runner/mock/mock_timers.js
Expand Up @@ -260,20 +260,23 @@ class MockTimers {

#createTimer(isInterval, callback, delay, ...args) {
const timerId = this.#currentTimer++;
this.#executionQueue.insert({
const timer = {
__proto__: null,
id: timerId,
callback,
runAt: this.#now + delay,
interval: isInterval,
interval: isInterval ? delay : undefined,
args,
});

return timerId;
};
this.#executionQueue.insert(timer);
return timer;
}

#clearTimer(position) {
this.#executionQueue.removeAt(position);
#clearTimer(timer) {
if (timer.priorityQueuePosition !== undefined) {
this.#executionQueue.removeAt(timer.priorityQueuePosition);
timer.priorityQueuePosition = undefined;
}
}

#createDate() {
Expand Down Expand Up @@ -397,10 +400,10 @@ class MockTimers {
emitter.emit('data', result);
};

const timerId = this.#createTimer(true, callback, interval, options);
const timer = this.#createTimer(true, callback, interval, options);
const clearListeners = () => {
emitter.removeAllListeners();
context.#clearTimer(timerId);
context.#clearTimer(timer);
};
const iterator = {
__proto__: null,
Expand Down Expand Up @@ -453,11 +456,11 @@ class MockTimers {
}

const onabort = () => {
clearFn(id);
clearFn(timer);
return reject(abortIt(options.signal));
};

const id = timerFn(() => {
const timer = timerFn(() => {
return resolve(result);
}, ms);

Expand Down Expand Up @@ -620,11 +623,11 @@ class MockTimers {
FunctionPrototypeApply(timer.callback, undefined, timer.args);

this.#executionQueue.shift();
timer.priorityQueuePosition = undefined;

if (timer.interval) {
if (timer.interval !== undefined) {
timer.runAt += timer.interval;
this.#executionQueue.insert(timer);
return;
}

timer = this.#executionQueue.peek();
Expand Down
85 changes: 85 additions & 0 deletions test/parallel/test-runner-mock-timers.js
Expand Up @@ -479,6 +479,59 @@ describe('Mock Timers Test Suite', () => {
code: 'ERR_INVALID_ARG_TYPE',
});
});

// Test for https://github.com/nodejs/node/issues/50365
it('should not affect other timers when aborting', async (t) => {
const f1 = t.mock.fn();
const f2 = t.mock.fn();
t.mock.timers.enable({ apis: ['setTimeout'] });
const ac = new AbortController();

// id 1 & pos 1 in priority queue
nodeTimersPromises.setTimeout(100, undefined, { signal: ac.signal }).then(f1, f1);
// id 2 & pos 1 in priority queue (id 1 is moved to pos 2)
nodeTimersPromises.setTimeout(50).then(f2, f2);

ac.abort(); // BUG: will remove timer at pos 1 not timer with id 1!

t.mock.timers.runAll();
await nodeTimersPromises.setImmediate(); // let promises settle

// First setTimeout is aborted
assert.strictEqual(f1.mock.callCount(), 1);
assert.strictEqual(f1.mock.calls[0].arguments[0].code, 'ABORT_ERR');

// Second setTimeout should resolve, but never settles, because it was eronously removed by ac.abort()
assert.strictEqual(f2.mock.callCount(), 1);
});

// Test for https://github.com/nodejs/node/issues/50365
it('should not affect other timers when aborted after triggering', async (t) => {
const f1 = t.mock.fn();
const f2 = t.mock.fn();
t.mock.timers.enable({ apis: ['setTimeout'] });
const ac = new AbortController();

// id 1 & pos 1 in priority queue
nodeTimersPromises.setTimeout(50, true, { signal: ac.signal }).then(f1, f1);
// id 2 & pos 2 in priority queue
nodeTimersPromises.setTimeout(100).then(f2, f2);

// First setTimeout resolves
t.mock.timers.tick(50);
anonrig marked this conversation as resolved.
Show resolved Hide resolved
await nodeTimersPromises.setImmediate(); // let promises settle
assert.strictEqual(f1.mock.callCount(), 1);
assert.strictEqual(f1.mock.calls[0].arguments.length, 1);
assert.strictEqual(f1.mock.calls[0].arguments[0], true);

// Now timer with id 2 will be at pos 1 in priority queue
ac.abort(); // BUG: will remove timer at pos 1 not timer with id 1!

// Second setTimeout should resolve, but never settles, because it was eronously removed by ac.abort()
t.mock.timers.runAll();
await nodeTimersPromises.setImmediate(); // let promises settle
assert.strictEqual(f2.mock.callCount(), 1);
});
});

describe('setInterval Suite', () => {
Expand Down Expand Up @@ -626,6 +679,38 @@ describe('Mock Timers Test Suite', () => {
});
assert.strictEqual(numIterations, expectedIterations);
});

// Test for https://github.com/nodejs/node/issues/50381
it('should use the correct interval', (t) => {
mika-fischer marked this conversation as resolved.
Show resolved Hide resolved
t.mock.timers.enable({ apis: ['setInterval'] });
const fn = t.mock.fn();
setInterval(fn, 1000);
assert.strictEqual(fn.mock.callCount(), 0);
t.mock.timers.tick(1000);
assert.strictEqual(fn.mock.callCount(), 1);
t.mock.timers.tick(1);
t.mock.timers.tick(1);
t.mock.timers.tick(1);
assert.strictEqual(fn.mock.callCount(), 1);
});

// Test for https://github.com/nodejs/node/issues/50382
it('should not prevent due timers to be processed', async (t) => {
t.mock.timers.enable({ apis: ['setInterval', 'setTimeout'] });
const f1 = t.mock.fn();
const f2 = t.mock.fn();

setInterval(f1, 1000);
setTimeout(f2, 1001);

assert.strictEqual(f1.mock.callCount(), 0);
assert.strictEqual(f2.mock.callCount(), 0);

t.mock.timers.tick(1001);

assert.strictEqual(f1.mock.callCount(), 1);
assert.strictEqual(f2.mock.callCount(), 1);
});
});
});
});
Expand Down