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

fix(test): Improve reliability of deno test's op sanitizer with timers #12908

Merged

Conversation

andreubotella
Copy link
Contributor

Although not easy to replicate in the wild, the deno test op sanitizer can fail when there are intervals that started before a test runs, since the op sanitizer can end up running in the time between the timer op for an interval's run resolves and the op for the next run starts.

This change fixes that by adding a new macrotask callback that will run after the timer macrotask queue has drained. This ensures that there is a timer op if there are any timers which are unresolved by the time the op sanitizer runs.

Although not easy to replicate in the wild, the `deno test` op sanitizer
can fail when there are intervals that started before a test runs, since
the op sanitizer can end up running in the time between the timer op for
an interval's run resolves and the op for the next run starts.

This change fixes that by adding a new macrotask callback that will run
after the timer macrotask queue has drained. This ensures that there is
a timer op if there are any timers which are unresolved by the time the
op sanitizer runs.
@andreubotella andreubotella changed the title fix(test): Improve reliability deno test's op sanitizer around timers fix(test): Improve reliability of deno test's op sanitizer with timers Nov 27, 2021
@andreubotella andreubotella force-pushed the op-sanitizer-after-timer-macrotasks branch from cbe41d5 to 237d44a Compare November 27, 2021 02:10
@andreubotella
Copy link
Contributor Author

This is needed to fix some of the CI failures in #12862

@andreubotella
Copy link
Contributor Author

andreubotella commented Nov 28, 2021

@bartlomieju @caspervonb PTAL

Copy link
Contributor

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I verified that provided test case fails on main. Good fix @andreubotella

@bartlomieju bartlomieju merged commit d335343 into denoland:main Nov 28, 2021
@andreubotella andreubotella deleted the op-sanitizer-after-timer-macrotasks branch November 28, 2021 15:40
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Nov 28, 2021
…ers (denoland#12908)

Although not easy to replicate in the wild, the `deno test` op sanitizer
can fail when there are intervals that started before a test runs, since
the op sanitizer can end up running in the time between the timer op for
an interval's run resolves and the op for the next run starts.

This change fixes that by adding a new macrotask callback that will run
after the timer macrotask queue has drained. This ensures that there is
a timer op if there are any timers which are unresolved by the time the
op sanitizer runs.
@kt3k
Copy link
Member

kt3k commented Nov 29, 2021

This change seems breaking some test cases in deno_std (e.g. node/_util/_util_callbackify_test.ts, node/crypto_test.ts, node/http_test.ts) ref: denoland/std#1651

@kt3k
Copy link
Member

kt3k commented Nov 29, 2021

Reproduced the issue with a smaller example. It seems related to nextTick:

import { nextTick } from "https://deno.land/std@0.116.0/node/_next_tick.ts";
Deno.test("test 1", async () => {
  await new Promise<void>((resolve) => nextTick(resolve));
});
Deno.test("test 2", async () => {
  await new Promise<void>((resolve) => nextTick(resolve));
});

The above example is stuck at the end of test 2

kt3k added a commit to kt3k/deno that referenced this pull request Nov 29, 2021
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Nov 29, 2021
bartlomieju added a commit that referenced this pull request Nov 29, 2021
andreubotella pushed a commit to andreubotella/deno that referenced this pull request Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants