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

add assertOps sanitizer in cli/js/ unit tests #4209

Merged
merged 8 commits into from Mar 3, 2020
Merged
Show file tree
Hide file tree
Changes from 7 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
2 changes: 1 addition & 1 deletion cli/js/net_test.ts
Expand Up @@ -54,7 +54,7 @@ testPerm({ net: true }, async function netTcpConcurrentAccept(): Promise<void> {
const p1 = listener.accept().catch(checkErr);
await Promise.race([p, p1]);
listener.close();
await [p, p1];
await Promise.all([p, p1]);
assertEquals(acceptErrCount, 1);
});
bartlomieju marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
7 changes: 5 additions & 2 deletions cli/js/performance_test.ts
@@ -1,10 +1,13 @@
// Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
import { testPerm, assert } from "./test_util.ts";
import { testPerm, assert, createResolvable } from "./test_util.ts";

testPerm({ hrtime: false }, function now(): void {
testPerm({ hrtime: false }, async function performanceNow(): Promise<void> {
const resolvable = createResolvable();
const start = performance.now();
setTimeout((): void => {
const end = performance.now();
assert(end - start >= 10);
resolvable.resolve();
}, 10);
await resolvable;
});
45 changes: 25 additions & 20 deletions cli/js/signal_test.ts
Expand Up @@ -4,7 +4,8 @@ import {
testPerm,
assert,
assertEquals,
assertThrows
assertThrows,
createResolvable
} from "./test_util.ts";

function defer(n: number): Promise<void> {
Expand Down Expand Up @@ -101,15 +102,13 @@ if (Deno.build.os === "win") {
);
});
} else {
testPerm({ run: true, net: true }, async function signalStreamTest(): Promise<
void
> {
testPerm({ run: true }, async function signalStreamTest(): Promise<void> {
const resolvable = createResolvable();
// This prevents the program from exiting.
const t = setInterval(() => {}, 1000);

let c = 0;
const sig = Deno.signal(Deno.Signal.SIGUSR1);

setTimeout(async () => {
await defer(20);
for (const _ of Array(3)) {
Expand All @@ -118,6 +117,7 @@ if (Deno.build.os === "win") {
await defer(20);
}
sig.dispose();
resolvable.resolve();
});

for await (const _ of sig) {
Expand All @@ -126,25 +126,30 @@ if (Deno.build.os === "win") {

assertEquals(c, 3);

clearTimeout(t);
clearInterval(t);
// Defer for a moment to allow interval promise to resolve
await defer(20);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is bad - it was not signal implementation that was leaking ops but interval. Because our internal timer implementation uses promises; after clearing an interval, one actually needs to wait until timer promise resolves. Otherwise that's considered a leak because timers are based on "ops".

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a TODO here explaining the situation? We should adjust the interval implementation to avoid this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added more description and FIXME in cli/js/timers.ts

await resolvable;
});

testPerm(
{ run: true, net: true },
async function signalPromiseTest(): Promise<void> {
// This prevents the program from exiting.
const t = setInterval(() => {}, 1000);
testPerm({ run: true }, async function signalPromiseTest(): Promise<void> {
const resolvable = createResolvable();
// This prevents the program from exiting.
const t = setInterval(() => {}, 1000);

const sig = Deno.signal(Deno.Signal.SIGUSR1);
setTimeout(() => {
Deno.kill(Deno.pid, Deno.Signal.SIGUSR1);
}, 20);
await sig;
sig.dispose();
const sig = Deno.signal(Deno.Signal.SIGUSR1);
setTimeout(() => {
Deno.kill(Deno.pid, Deno.Signal.SIGUSR1);
resolvable.resolve();
}, 20);
await sig;
sig.dispose();

clearTimeout(t);
}
);
clearInterval(t);
// Defer for a moment to allow interval promise to resolve
await defer(20);
await resolvable;
bartlomieju marked this conversation as resolved.
Show resolved Hide resolved
});

testPerm({ run: true }, async function signalShorthandsTest(): Promise<void> {
let s: Deno.SignalStream;
Expand Down
30 changes: 28 additions & 2 deletions cli/js/test_util.ts
Expand Up @@ -106,11 +106,37 @@ function normalizeTestPermissions(perms: TestPermissions): Permissions {
};
}

// Wrap `TestFunction` in additional assertion that makes sure
// the test case does not leak async "ops" - ie. number of async
// completed ops after the test is the same as number of dispatched
// ops. Note that "unref" ops are ignored since in nature that are
// optional.
function assertOps(fn: Deno.TestFunction): Deno.TestFunction {
return async function asyncOpSanitizer(): Promise<void> {
const pre = Deno.metrics();
await fn();
const post = Deno.metrics();
// We're checking diff because one might spawn HTTP server in the background
// that will be a pending async op before test starts.
assertEquals(
post.opsDispatchedAsync - pre.opsDispatchedAsync,
post.opsCompletedAsync - pre.opsCompletedAsync,
`Test case is leaking async ops.
Before:
- dispatched: ${pre.opsDispatchedAsync}
- completed: ${pre.opsCompletedAsync}
After:
- dispatched: ${post.opsDispatchedAsync}
- completed: ${post.opsCompletedAsync}`
);
};
}

// Wrap `TestFunction` in additional assertion that makes sure
// the test case does not "leak" resources - ie. resource table after
// the test has exactly the same contents as before the test.
function assertResources(fn: Deno.TestFunction): Deno.TestFunction {
return async function(): Promise<void> {
return async function resourceSanitizer(): Promise<void> {
const preResources = Deno.resources();
await fn();
const postResources = Deno.resources();
Expand All @@ -131,7 +157,7 @@ export function testPerm(perms: TestPermissions, fn: Deno.TestFunction): void {
return;
}

Deno.test(fn.name, assertResources(fn));
Deno.test(fn.name, assertResources(assertOps(fn)));
}

export function test(fn: Deno.TestFunction): void {
Expand Down
66 changes: 33 additions & 33 deletions cli/js/timers_test.ts
@@ -1,5 +1,11 @@
// Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
import { test, assert, assertEquals, assertNotEquals } from "./test_util.ts";
import {
test,
createResolvable,
assert,
assertEquals,
assertNotEquals
} from "./test_util.ts";

function deferred(): {
promise: Promise<{}>;
Expand Down Expand Up @@ -178,8 +184,6 @@ test(async function timeoutCallbackThis(): Promise<void> {
});

test(async function timeoutBindThis(): Promise<void> {
function noop(): void {}

const thisCheckPassed = [null, undefined, window, globalThis];

const thisCheckFailed = [
Expand All @@ -194,41 +198,37 @@ test(async function timeoutBindThis(): Promise<void> {
Object.prototype
];

thisCheckPassed.forEach(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(thisArg: any): void => {
let hasThrown = 0;
try {
setTimeout.call(thisArg, noop, 1);
hasThrown = 1;
} catch (err) {
if (err instanceof TypeError) {
hasThrown = 2;
} else {
hasThrown = 3;
}
for (const thisArg of thisCheckPassed) {
const resolvable = createResolvable();
let hasThrown = 0;
try {
setTimeout.call(thisArg, () => resolvable.resolve(), 1);
hasThrown = 1;
} catch (err) {
if (err instanceof TypeError) {
hasThrown = 2;
} else {
hasThrown = 3;
}
assertEquals(hasThrown, 1);
}
);
await resolvable;
assertEquals(hasThrown, 1);
}

thisCheckFailed.forEach(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(thisArg: any): void => {
let hasThrown = 0;
try {
setTimeout.call(thisArg, noop, 1);
hasThrown = 1;
} catch (err) {
if (err instanceof TypeError) {
hasThrown = 2;
} else {
hasThrown = 3;
}
for (const thisArg of thisCheckFailed) {
let hasThrown = 0;
try {
setTimeout.call(thisArg, () => {}, 1);
hasThrown = 1;
} catch (err) {
if (err instanceof TypeError) {
hasThrown = 2;
} else {
hasThrown = 3;
}
assertEquals(hasThrown, 2);
}
);
assertEquals(hasThrown, 2);
}
});

test(async function clearTimeoutShouldConvertToNumber(): Promise<void> {
Expand Down
1 change: 0 additions & 1 deletion cli/js/unit_tests.ts
Expand Up @@ -62,7 +62,6 @@ import "./utime_test.ts";
import "./write_file_test.ts";
import "./performance_test.ts";
import "./version_test.ts";
import "./workers_test.ts";

if (import.meta.main) {
await Deno.runTests();
Expand Down
26 changes: 0 additions & 26 deletions cli/tests/026_workers.ts

This file was deleted.

7 changes: 0 additions & 7 deletions cli/tests/026_workers.ts.out

This file was deleted.

12 changes: 4 additions & 8 deletions cli/tests/integration_tests.rs
Expand Up @@ -798,14 +798,10 @@ itest!(_026_redirect_javascript {
http_server: true,
});

itest!(_026_workers {
args: "run --reload 026_workers.ts",
output: "026_workers.ts.out",
});

itest!(workers_basic {
args: "run --reload workers_basic.ts",
output: "workers_basic.out",
itest!(workers {
args: "run --reload --allow-net workers_test.ts",
http_server: true,
output: "workers_test.out",
});

itest!(_027_redirect_typescript {
Expand Down
1 change: 0 additions & 1 deletion cli/tests/subdir/nested_worker.js
Expand Up @@ -9,7 +9,6 @@ jsWorker.onerror = _e => {
};

jsWorker.onmessage = e => {
console.log("js worker on message");
postMessage({ type: "msg", text: e });
close();
};
Expand Down
3 changes: 0 additions & 3 deletions cli/tests/subdir/test_worker.js
Expand Up @@ -5,8 +5,6 @@ if (self.name !== "jsWorker") {
}

onmessage = function(e) {
console.log(e.data);

if (thrown === false) {
thrown = true;
throw new SyntaxError("[test error]");
Expand All @@ -17,6 +15,5 @@ onmessage = function(e) {
};

onerror = function() {
console.log("called onerror in worker");
return false;
};
1 change: 0 additions & 1 deletion cli/tests/subdir/test_worker.ts
Expand Up @@ -3,7 +3,6 @@ if (self.name !== "tsWorker") {
}

onmessage = function(e): void {
console.log(e.data);
postMessage(e.data);
close();
};
2 changes: 0 additions & 2 deletions cli/tests/subdir/test_worker_basic.js
Expand Up @@ -6,12 +6,10 @@ if (self.name !== "jsWorker") {
}

onmessage = function(e) {
console.log("jsWorker onmessage", e.data);
postMessage(e.data);
close();
};

onerror = function() {
console.log("called onerror in worker");
return false;
};
Copy link
Member

Choose a reason for hiding this comment

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

I don’t understand why all these console.log statements are being removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not needed and was harder to match output in integration test

3 changes: 0 additions & 3 deletions cli/tests/workers_basic.out

This file was deleted.

11 changes: 0 additions & 11 deletions cli/tests/workers_basic.ts

This file was deleted.

7 changes: 7 additions & 0 deletions cli/tests/workers_test.out
@@ -0,0 +1,7 @@
running 4 tests
OK workersBasic [WILDCARD]
OK nestedWorker [WILDCARD]
OK workerThrowsWhenExecuting [WILDCARD]
OK workerCanUseFetch [WILDCARD]

test result: OK 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out [WILDCARD]