Skip to content

Conversation

@Xstoudi
Copy link

@Xstoudi Xstoudi commented Dec 22, 2025

Should fix #103

@Xstoudi Xstoudi changed the title feature(mocha-to-node-test-runner) : Migrate Mocha v8 tests to Node.js test runner (v22, v24+) feat(mocha-to-node-test-runner) : Migrate Mocha v8 tests to Node.js test runner (v22, v24+) Dec 22, 2025
Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

Oh nice that really cool !

Few points that can be improved:

  • md format on readme
  • little bit of JSdoc
  • small missing unit test

Xstoudi and others added 2 commits December 23, 2025 19:12
@Xstoudi
Copy link
Author

Xstoudi commented Dec 23, 2025

I wrote test for is-esm, let me know if you see something is missing.

I applied your recommendations then reverted two of them:

  • one was changing logic wrongly
  • replacing array.length === 0 with !array.length make it less readable, I'd prefer to avoid doing so

Let me now if there is anything else.

it('should return -1 when the value is not present', function() {
const arr = [1, 2, 3];
assert.strictEqual(arr.indexOf(4), -1);
});
Copy link
Member

Choose a reason for hiding this comment

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

maybe add an other it that use arrow function as call back
and a fonction as arg like

function myBasicTest = () => {
  assert(true)
}

The second case will be can super complicated to migrate.
in a first time let's just have test that proof that's ins't handled and document it.
In second time (follow up pr) having systems that handle this case. The logic to catch a refercen is already furnished by the codemod context https://docs.codemod.com/jssg/semantic-analysis

@Xstoudi
Copy link
Author

Xstoudi commented Dec 26, 2025

Let me know if anything more is required.

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

LGMT ! for the code transformation part missing the removing dep you should take code from chalk-to-styletext.

@Xstoudi
Copy link
Author

Xstoudi commented Dec 30, 2025

Added that, let me know if something is missing.

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

For me it's good ! we need to wait a second approval to land !

@AugustinMauroy
Copy link
Member

F*ck what happened here. !Windows!

cc @nodejs/userland-migrations any idea

},
},
rule: {
any: [{ pattern: '$MOCHA_GLOBAL_FN($$$)' }],
Copy link
Member

Choose a reason for hiding this comment

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

nit: unnecessary any

Suggested change
any: [{ pattern: '$MOCHA_GLOBAL_FN($$$)' }],
pattern: '$MOCHA_GLOBAL_FN($$$)',

Comment on lines +81 to +110
if (esm) {
const importStatements = rootNode.findAll({
rule: { kind: 'import_statement' },
});
const lastImportStatement = importStatements[importStatements.length - 1];
if (lastImportStatement !== undefined) {
return [
{
startPos: lastImportStatement.range().end.index,
endPos: lastImportStatement.range().end.index,
insertedText,
},
];
}
} else {
const requireStatements = rootNode.findAll({
rule: { pattern: 'const $_A = require($_B)' },
});
const lastRequireStatements =
requireStatements[requireStatements.length - 1];
if (lastRequireStatements !== undefined) {
return [
{
startPos: lastRequireStatements.range().end.index,
endPos: lastRequireStatements.range().end.index,
insertedText,
},
];
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (esm) {
const importStatements = rootNode.findAll({
rule: { kind: 'import_statement' },
});
const lastImportStatement = importStatements[importStatements.length - 1];
if (lastImportStatement !== undefined) {
return [
{
startPos: lastImportStatement.range().end.index,
endPos: lastImportStatement.range().end.index,
insertedText,
},
];
}
} else {
const requireStatements = rootNode.findAll({
rule: { pattern: 'const $_A = require($_B)' },
});
const lastRequireStatements =
requireStatements[requireStatements.length - 1];
if (lastRequireStatements !== undefined) {
return [
{
startPos: lastRequireStatements.range().end.index,
endPos: lastRequireStatements.range().end.index,
insertedText,
},
];
}
}
const dependenciesStmt = rootNode.findAll({
rule: {
any: [
{ kind: 'import_statement' },
{
kind: 'lexical_declaration',
has: {
stopBy: 'end',
kind: 'call_expression',
has: {
kind: 'identifier',
regex: '^require$',
},
},
},
],
},
});
const lastDepStmt = dependenciesStmt.at(-1);
if (lastDepStmt !== undefined) {
return [
{
startPos: lastDepStmt.range().end.index,
endPos: lastDepStmt.range().end.index,
insertedText,
},
];
}

Copy link
Member

Choose a reason for hiding this comment

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

Please add tests that cover esm implementations

Comment on lines +161 to +252
function transformThisSkip(root: SgRoot<JS>): Edit[] {
const rootNode = root.root();
const thisSkipCalls = rootNode.findAll({
rule: { pattern: 'this.skip($$$)' },
});

return thisSkipCalls.flatMap((thisSkipCall) => {
const edits: Edit[] = [];
const memberExpr = thisSkipCall.find({
rule: { kind: 'member_expression', has: { kind: 'this' } },
});
if (memberExpr !== null) {
const thisKeyword = memberExpr.field('object');
if (thisKeyword !== null) {
edits.push(thisKeyword.replace('t'));
}
}

const enclosingFunction = thisSkipCall
.ancestors()
.find((ancestor) =>
['function_expression', 'arrow_function'].includes(ancestor.kind()),
);
if (enclosingFunction === undefined) {
return edits;
}

const parameters =
enclosingFunction.field('parameters') ??
enclosingFunction.field('parameter');
if (parameters === null) {
return edits;
}

if (parameters.kind() === 'identifier') {
edits.push(parameters.replace(`(t, ${parameters.text()})`));
} else if (parameters.kind() === 'formal_parameters') {
edits.push({
startPos: parameters.range().start.index + 1,
endPos: parameters.range().start.index + 1,
insertedText: `t${parameters.children().length > 2 ? ', ' : ''}`,
});
}

return edits;
});
}

function transformThisTimeout(root: SgRoot<JS>): Edit[] {
const rootNode = root.root();
const thisTimeoutCalls = rootNode.findAll({
rule: { pattern: 'this.timeout($TIME)' },
});

return thisTimeoutCalls.flatMap((thisTimeoutCall) => {
const edits = [] as Edit[];
const thisTimeoutExpression = thisTimeoutCall.parent();

const source = rootNode.text();
const startIndex = thisTimeoutExpression.range().start.index;
const endIndex = thisTimeoutExpression.range().end.index;

let lineStart = startIndex;
while (lineStart > 0 && source[lineStart - 1] !== EOL) lineStart--;
let lineEnd = endIndex;
while (lineEnd < source.length && source[lineEnd] !== EOL) lineEnd++;
if (lineEnd < source.length) lineEnd++;

edits.push({
startPos: lineStart,
endPos: lineEnd,
insertedText: '',
});

const enclosingFunction = thisTimeoutCall
.ancestors()
.find((ancestor) =>
['function_expression', 'arrow_function'].includes(ancestor.kind()),
);
if (enclosingFunction === undefined) {
return edits;
}

const time = thisTimeoutCall.getMatch('TIME').text();
edits.push({
startPos: enclosingFunction.range().start.index,
endPos: enclosingFunction.range().start.index,
insertedText: `{ timeout: ${time} }, `,
});
return edits;
});
}
Copy link
Member

Choose a reason for hiding this comment

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

I think these this.skip and this.timeout handlers can be unified, looks like the same logic. what do you think?

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.

brainstorm: mocha-to-node-test-runner

3 participants