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

refactor(cli): use new sanitizer for resources #22125

Merged
merged 10 commits into from
Jan 26, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
183 changes: 2 additions & 181 deletions cli/js/40_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,8 @@ const {
MapPrototypeGet,
MapPrototypeHas,
MapPrototypeSet,
ObjectKeys,
Promise,
SafeArrayIterator,
Set,
SymbolToStringTag,
TypeError,
} = primordials;
Expand Down Expand Up @@ -342,182 +340,6 @@ function assertOps(fn) {
};
}

function prettyResourceNames(name) {
switch (name) {
case "fsFile":
return ["A file", "opened", "closed"];
case "fetchRequest":
return ["A fetch request", "started", "finished"];
case "fetchRequestBody":
return ["A fetch request body", "created", "closed"];
case "fetchResponse":
return ["A fetch response body", "created", "consumed"];
case "httpClient":
return ["An HTTP client", "created", "closed"];
case "dynamicLibrary":
return ["A dynamic library", "loaded", "unloaded"];
case "httpConn":
return ["An inbound HTTP connection", "accepted", "closed"];
case "httpStream":
return ["An inbound HTTP request", "accepted", "closed"];
case "tcpStream":
return ["A TCP connection", "opened/accepted", "closed"];
case "unixStream":
return ["A Unix connection", "opened/accepted", "closed"];
case "tlsStream":
return ["A TLS connection", "opened/accepted", "closed"];
case "tlsListener":
return ["A TLS listener", "opened", "closed"];
case "unixListener":
return ["A Unix listener", "opened", "closed"];
case "unixDatagram":
return ["A Unix datagram", "opened", "closed"];
case "tcpListener":
return ["A TCP listener", "opened", "closed"];
case "udpSocket":
return ["A UDP socket", "opened", "closed"];
case "timer":
return ["A timer", "started", "fired/cleared"];
case "textDecoder":
return ["A text decoder", "created", "finished"];
case "messagePort":
return ["A message port", "created", "closed"];
case "webSocketStream":
return ["A WebSocket", "opened", "closed"];
case "fsEvents":
return ["A file system watcher", "created", "closed"];
case "childStdin":
return ["A child process stdin", "opened", "closed"];
case "childStdout":
return ["A child process stdout", "opened", "closed"];
case "childStderr":
return ["A child process stderr", "opened", "closed"];
case "child":
return ["A child process", "started", "closed"];
case "signal":
return ["A signal listener", "created", "fired/cleared"];
case "stdin":
return ["The stdin pipe", "opened", "closed"];
case "stdout":
return ["The stdout pipe", "opened", "closed"];
case "stderr":
return ["The stderr pipe", "opened", "closed"];
case "compression":
return ["A CompressionStream", "created", "closed"];
default:
return [`A "${name}" resource`, "created", "cleaned up"];
}
}

function resourceCloseHint(name) {
switch (name) {
case "fsFile":
return "Close the file handle by calling `file.close()`.";
case "fetchRequest":
return "Await the promise returned from `fetch()` or abort the fetch with an abort signal.";
case "fetchRequestBody":
return "Terminate the request body `ReadableStream` by closing or erroring it.";
case "fetchResponse":
return "Consume or close the response body `ReadableStream`, e.g `await resp.text()` or `await resp.body.cancel()`.";
case "httpClient":
return "Close the HTTP client by calling `httpClient.close()`.";
case "dynamicLibrary":
return "Unload the dynamic library by calling `dynamicLibrary.close()`.";
case "httpConn":
return "Close the inbound HTTP connection by calling `httpConn.close()`.";
case "httpStream":
return "Close the inbound HTTP request by responding with `e.respondWith()` or closing the HTTP connection.";
case "tcpStream":
return "Close the TCP connection by calling `tcpConn.close()`.";
case "unixStream":
return "Close the Unix socket connection by calling `unixConn.close()`.";
case "tlsStream":
return "Close the TLS connection by calling `tlsConn.close()`.";
case "tlsListener":
return "Close the TLS listener by calling `tlsListener.close()`.";
case "unixListener":
return "Close the Unix socket listener by calling `unixListener.close()`.";
case "unixDatagram":
return "Close the Unix datagram socket by calling `unixDatagram.close()`.";
case "tcpListener":
return "Close the TCP listener by calling `tcpListener.close()`.";
case "udpSocket":
return "Close the UDP socket by calling `udpSocket.close()`.";
case "timer":
return "Clear the timer by calling `clearInterval` or `clearTimeout`.";
case "textDecoder":
return "Close the text decoder by calling `textDecoder.decode('')` or `await textDecoderStream.readable.cancel()`.";
case "messagePort":
return "Close the message port by calling `messagePort.close()`.";
case "webSocketStream":
return "Close the WebSocket by calling `webSocket.close()`.";
case "fsEvents":
return "Close the file system watcher by calling `watcher.close()`.";
case "childStdin":
return "Close the child process stdin by calling `proc.stdin.close()`.";
case "childStdout":
return "Close the child process stdout by calling `proc.stdout.close()` or `await child.stdout.cancel()`.";
case "childStderr":
return "Close the child process stderr by calling `proc.stderr.close()` or `await child.stderr.cancel()`.";
case "child":
return "Close the child process by calling `proc.kill()` or `proc.close()`.";
case "signal":
return "Clear the signal listener by calling `Deno.removeSignalListener`.";
case "stdin":
return "Close the stdin pipe by calling `Deno.stdin.close()`.";
case "stdout":
return "Close the stdout pipe by calling `Deno.stdout.close()`.";
case "stderr":
return "Close the stderr pipe by calling `Deno.stderr.close()`.";
case "compression":
return "Close the compression stream by calling `await stream.writable.close()`.";
default:
return "Close the resource before the end of the test.";
}
}

// Wrap test function 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) {
/** @param desc {TestDescription | TestStepDescription} */
return async function resourceSanitizer(desc) {
const pre = core.resources();
const innerResult = await fn(desc);
if (innerResult) return innerResult;
const post = core.resources();

const allResources = new Set([
...new SafeArrayIterator(ObjectKeys(pre)),
...new SafeArrayIterator(ObjectKeys(post)),
]);

const details = [];
for (const resource of allResources) {
const preResource = pre[resource];
const postResource = post[resource];
if (preResource === postResource) continue;

if (preResource === undefined) {
const [name, action1, action2] = prettyResourceNames(postResource);
const hint = resourceCloseHint(postResource);
const detail =
`${name} (rid ${resource}) was ${action1} during the test, but not ${action2} during the test. ${hint}`;
ArrayPrototypePush(details, detail);
} else {
const [name, action1, action2] = prettyResourceNames(preResource);
const detail =
`${name} (rid ${resource}) was ${action1} before the test started, but was ${action2} during the test. Do not close resources in a test that were not created during that test.`;
ArrayPrototypePush(details, detail);
}
}
if (details.length == 0) {
return null;
}
return { failed: { leakedResources: details } };
};
}

// Wrap test function in additional assertion that makes sure
// that the test case does not accidentally exit prematurely.
function assertExit(fn, isTest) {
Expand Down Expand Up @@ -720,6 +542,8 @@ function testInner(
testDesc.name,
testDesc.ignore,
testDesc.only,
false, /*testDesc.sanitizeOps*/
testDesc.sanitizeResources,
testDesc.location.fileName,
testDesc.location.lineNumber,
testDesc.location.columnNumber,
Expand Down Expand Up @@ -910,9 +734,6 @@ function wrapTest(desc) {
if (desc.sanitizeOps) {
testFn = assertOps(testFn);
}
if (desc.sanitizeResources) {
testFn = assertResources(testFn);
}
if (desc.sanitizeExit) {
testFn = assertExit(testFn, true);
}
Expand Down
4 changes: 3 additions & 1 deletion cli/lsp/testing/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,9 @@ impl TestRun {
test::TestResult::Ignored => summary.ignored += 1,
test::TestResult::Failed(error) => {
summary.failed += 1;
summary.failures.push((description.clone(), error.clone()));
summary
.failures
.push(((&description).into(), error.clone()));
}
test::TestResult::Cancelled => {
summary.failed += 1;
Expand Down
4 changes: 4 additions & 0 deletions cli/ops/testing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ fn op_register_test(
#[string] name: String,
ignore: bool,
only: bool,
sanitize_ops: bool,
sanitize_resources: bool,
#[string] file_name: String,
#[smi] line_number: u32,
#[smi] column_number: u32,
Expand All @@ -141,6 +143,8 @@ fn op_register_test(
name,
ignore,
only,
sanitize_ops,
sanitize_resources,
origin: origin.clone(),
location: TestLocation {
file_name,
Expand Down
6 changes: 4 additions & 2 deletions cli/tests/testdata/test/resource_sanitizer.out
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ leak ... FAILED ([WILDCARD])

leak => ./test/resource_sanitizer.ts:[WILDCARD]
error: Leaking resources:
- The stdin pipe (rid 0) was opened before the test started, but was closed during the test. Do not close resources in a test that were not created during that test.
- A file (rid 3) was opened during the test, but not closed during the test. Close the file handle by calling `file.close()`.
[UNORDERED_START]
- The stdin pipe was opened before the test started, but was closed during the test. Do not close resources in a test that were not created during that test.
- A file was opened during the test, but not closed during the test. Close the file handle by calling `file.close()`.
[UNORDERED_END]
Comment on lines +9 to +12
Copy link
Member

Choose a reason for hiding this comment

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

Why is it unordered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR changes the order of the output but I don't think we should care about that from a test perspective (allowing me to tweak in the future).

The information is what's important here, not the order IMO


FAILURES

Expand Down