-
-
Notifications
You must be signed in to change notification settings - Fork 31
feat(mocha-to-node-test-runner) : Migrate Mocha v8 tests to Node.js test runner (v22, v24+) #313
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
base: main
Are you sure you want to change the base?
Conversation
AugustinMauroy
left a comment
There was a problem hiding this 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
Co-authored-by: Augustin Mauroy <[email protected]>
|
I wrote test for is-esm, let me know if you see something is missing. I applied your recommendations then reverted two of them:
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); | ||
| }); |
There was a problem hiding this comment.
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
|
Let me know if anything more is required. |
AugustinMauroy
left a comment
There was a problem hiding this 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.
|
Added that, let me know if something is missing. |
AugustinMauroy
left a comment
There was a problem hiding this 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 !
|
F*ck what happened here. !Windows! cc @nodejs/userland-migrations any idea |
| }, | ||
| }, | ||
| rule: { | ||
| any: [{ pattern: '$MOCHA_GLOBAL_FN($$$)' }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unnecessary any
| any: [{ pattern: '$MOCHA_GLOBAL_FN($$$)' }], | |
| pattern: '$MOCHA_GLOBAL_FN($$$)', |
| 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, | ||
| }, | ||
| ]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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, | |
| }, | |
| ]; | |
| } | |
There was a problem hiding this comment.
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
| 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; | ||
| }); | ||
| } |
There was a problem hiding this comment.
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?
Should fix #103