Skip to content

Conversation

@AugustinMauroy
Copy link
Member

@AugustinMauroy AugustinMauroy commented Dec 10, 2025

Related issue

Close #260

@AugustinMauroy AugustinMauroy marked this pull request as ready for review December 15, 2025 19:28
@AugustinMauroy AugustinMauroy requested review from a team and ljharb December 15, 2025 19:28
@AugustinMauroy AugustinMauroy added the awaiting reviewer Author has responded and needs action from the reviewer label Dec 15, 2025
Comment on lines +66 to +136
const tapeImports = getNodeImportStatements(root, 'tape');
const tapeRequires = getNodeRequireCalls(root, 'tape');
const tapeImportCalls = getNodeImportCalls(root, 'tape');

if (
tapeImports.length === 0 &&
tapeRequires.length === 0 &&
tapeImportCalls.length === 0
) {
return null;
}

let testVarName = 'test';

// 1. Replace imports
for (const imp of tapeImports) {
const defaultImport = imp.find({
rule: { kind: 'import_clause', has: { kind: 'identifier' } },
});
if (defaultImport) {
const id = defaultImport.find({ rule: { kind: 'identifier' } });
if (id) testVarName = id.text();
edits.push(
imp.replace(
`import { test } from 'node:test';${EOL}import assert from 'node:assert';`,
),
);
}
}

for (const req of tapeRequires) {
const id = req.find({
rule: { kind: 'identifier', inside: { kind: 'variable_declarator' } },
});
if (id) testVarName = id.text();
const declaration = req
.ancestors()
.find(
(a) =>
a.kind() === 'variable_declaration' ||
a.kind() === 'lexical_declaration',
);
if (declaration) {
edits.push(
declaration.replace(
`const { test } = require('node:test');${EOL}const assert = require('node:assert');`,
),
);
}
}

for (const call of tapeImportCalls) {
const id = call.find({
rule: { kind: 'identifier', inside: { kind: 'variable_declarator' } },
});
if (id) testVarName = id.text();
const declaration = call
.ancestors()
.find(
(a) =>
a.kind() === 'variable_declaration' ||
a.kind() === 'lexical_declaration',
);
if (declaration) {
edits.push(
declaration.replace(
`const { test } = await import('node:test');${EOL}const { default: assert } = await import('node:assert');`,
),
);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This logic is quite the same for the 3 difference scenarios, I think it can be simplified to code below

Suggested change
const tapeImports = getNodeImportStatements(root, 'tape');
const tapeRequires = getNodeRequireCalls(root, 'tape');
const tapeImportCalls = getNodeImportCalls(root, 'tape');
if (
tapeImports.length === 0 &&
tapeRequires.length === 0 &&
tapeImportCalls.length === 0
) {
return null;
}
let testVarName = 'test';
// 1. Replace imports
for (const imp of tapeImports) {
const defaultImport = imp.find({
rule: { kind: 'import_clause', has: { kind: 'identifier' } },
});
if (defaultImport) {
const id = defaultImport.find({ rule: { kind: 'identifier' } });
if (id) testVarName = id.text();
edits.push(
imp.replace(
`import { test } from 'node:test';${EOL}import assert from 'node:assert';`,
),
);
}
}
for (const req of tapeRequires) {
const id = req.find({
rule: { kind: 'identifier', inside: { kind: 'variable_declarator' } },
});
if (id) testVarName = id.text();
const declaration = req
.ancestors()
.find(
(a) =>
a.kind() === 'variable_declaration' ||
a.kind() === 'lexical_declaration',
);
if (declaration) {
edits.push(
declaration.replace(
`const { test } = require('node:test');${EOL}const assert = require('node:assert');`,
),
);
}
}
for (const call of tapeImportCalls) {
const id = call.find({
rule: { kind: 'identifier', inside: { kind: 'variable_declarator' } },
});
if (id) testVarName = id.text();
const declaration = call
.ancestors()
.find(
(a) =>
a.kind() === 'variable_declaration' ||
a.kind() === 'lexical_declaration',
);
if (declaration) {
edits.push(
declaration.replace(
`const { test } = await import('node:test');${EOL}const { default: assert } = await import('node:assert');`,
),
);
}
}
const tapeImports = getNodeImportStatements(root, 'tape');
const tapeRequires = getNodeRequireCalls(root, 'tape');
const tapeImportCalls = getNodeImportCalls(root, 'tape');
const modDeps = [
...tapeImports.map((node) => ({
node,
import: `import { test } from 'node:test';${EOL}import assert from 'node:assert';`,
})),
...tapeRequires.map((node) => ({
node,
import: `const { test } = require('node:test');${EOL}const assert = require('node:assert');`,
})),
...tapeImportCalls.map((node) => ({
node,
import: `const { test } = await import('node:test');${EOL}const { default: assert } = await import('node:assert');`,
})),
];
if (modDeps.length === 0) {
return null;
}
let testVarName = 'test';
// 1. Replace imports
for (const mod of modDeps) {
if (mod.node.kind() === 'variable_declarator') {
mod.node = mod.node.parent();
}
const binding = mod.node.find({
rule: {
any: [
{ kind: 'identifier', inside: { kind: 'variable_declarator' } },
{
kind: 'identifier',
inside: { kind: 'import_clause', stopBy: 'end' },
},
],
},
});
if (binding) testVarName = binding.text();
edits.push(mod.node.replace(mod.import));
}

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

(btw the diff would read cleaner if it was "output" instead of "expected", because then "input" would be sorted before "output" instead of the reverse)

some of my comments apply on all the input/expected combos, so i'll post this now before being exhaustive

Comment on lines +16 to +18
// TODO: test.onFinish(() => {
// console.log('finished');
// });
Copy link
Member

Choose a reason for hiding this comment

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

if this is a TODO for you, then this PR should probably still be a draft; if not, then it would be better to make it fail VERY LOUDLY so that users are aware they have to manually fix something. it's not ok to just silently elide their code :-)

Comment on lines +4 to +6
test('sync test with no args', async () => {
const a = 1;
});
Copy link
Member

Choose a reason for hiding this comment

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

a sync text needs to always be an async function in the test runner? O.o that seems like it introduces an unnecessary performance hit

Comment on lines +4 to +6
test("require test", async (t) => {
assert.strictEqual(1, 1);
// t.end();
Copy link
Member

Choose a reason for hiding this comment

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

this line should either be removed, or it should end up with whatever the node test runner has to say "this test is done and if you get more assertions later, it should error". (if it doesn't have that, then the node runner doesn't have sufficient features for a codemod from tape to be appropriate)

@brunocroh brunocroh added awaiting author Reviewer has requested something from the author and removed awaiting reviewer Author has responded and needs action from the reviewer labels Jan 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting author Reviewer has requested something from the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spec: tape-to-node-test-runner

4 participants