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

feat: Implement standalone mocha runner and loader #1

Merged
merged 28 commits into from
Apr 2, 2024

Conversation

Danielku15
Copy link
Member

@Danielku15 Danielku15 commented Mar 24, 2024

This PR adapts the https://github.com/microsoft/vscode-extension-test-runner/ fork codebase to a standalone mocha test runner extension.

  • Rebrand
  • Implement standalone testloading with JS (commonjs, ESM) and TypeScript support
  • Ensure proper sourcemap support
  • Ensure running and debugging of tests are working
  • Update and extend tests
  • Setup GitHub Actions for building and testing the extension
  • Implement proper logging to output window.
  • Reviewing all copyright and license details
  • Contribute code to Mocha Project / Rebrand to personal project
  • Publish extension.

@Danielku15 Danielku15 force-pushed the feature/mocha-standalone branch from 8578e3c to 93fd534 Compare March 31, 2024 13:17
@marcus-pousette
Copy link

marcus-pousette commented Mar 31, 2024

This is great! Really need this 👍 looking forward to the release. Perhaps it make sense to do a alpha release so people can report issues early on?

@Danielku15
Copy link
Member Author

@marcus-pousette Highly depends on the feedback at mochajs/mocha#5039 if the Mocha project will adopt this extension or if I publish it under my name 😁

@Danielku15 Danielku15 force-pushed the feature/mocha-standalone branch from dbd3ff6 to 33f0174 Compare March 31, 2024 14:58
@marcus-pousette
Copy link

marcus-pousette commented Mar 31, 2024

@Danielku15 great!

I started trying out this branch. I am not sure if you are getting this but with this simple test repo. The line numbers are wrong. I am using Node v20.12.0

https://github.com/marcus-pousette/mocha-vscode-test

Screenshot 2024-03-31 at 21 07 24

@Danielku15
Copy link
Member Author

@marcus-pousette Yeah, that's something I'm still tackling. Internally I use ESBuild to transpile TypeScript to JavaScript (including Source Maps) and then I need to translate back the positions correctly.

For plain JS the positoning is already fine, for TS (and JavaScript with source maps) the bits are still missing.

@marcus-pousette
Copy link

marcus-pousette commented Mar 31, 2024

Testing monorepos. I have two issues, see image

Screenshot 2024-03-31 at 21 25 16

  1. Sidebar have duplicates of the same tests. For some reason node_modules show up here. (Maybe I am missing some _node_modules ignore in the mocha config. But I dont have this problem in the simple repo I linked above)
  2. Codelens locations are wrong (same for my previous comment/test)

Node v20.12.0
Here is the repo

https://github.com/marcus-pousette/mocha-vscode-monorepo-test

@marcus-pousette
Copy link

marcus-pousette commented Mar 31, 2024

@marcus-pousette Yeah, that's something I'm still tackling. Internally I use ESBuild to transpile TypeScript to JavaScript (including Source Maps) and then I need to translate back the positions correctly.

For plain JS the positoning is already fine, for TS (and JavaScript with source maps) the bits are still missing.

Ah I see! I kind of spent some time testing different Node versions and noticed how later ones handle whitespace differently. Not sure if that is helpful here since this was also related to ts-node and the other vscode extension that used stack traces for finding the locations of functions.

I tried both 18.12.0 and 20.12.0 and get the same result so not related to this

@marcus-pousette
Copy link

Not sure if helpful but the jest-vscode extension uses https://github.com/jridgewell/trace-mapping behind the scenes through the istanbul-lib-source-maps package

@Danielku15
Copy link
Member Author

@marcus-pousette Thanks for the helpful test repos. I quickly fixed the TypeScript source map stuff, line numbering seem fine in my the projects I use for testing.

Regarding the mono-repo: Can you update the repository to a state where mocha itself (e.g. running npx mocha) is functional and delivers the correct test results when executed in respective directories?

Currently the repo seems not yet in a functional state to work with mocha standalone. I currently strive for consistency between the CLI and the VS Code extension. If you can fix that I can check easier where the inconsistency in the globbing occurs.

@marcus-pousette
Copy link

marcus-pousette commented Mar 31, 2024

Great! I will try the code out again if you have pushed all changes.

I updated

https://github.com/marcus-pousette/mocha-vscode-monorepo-test.git

now so that

npx mocha works in root and all packages

@marcus-pousette
Copy link

The line number issues still seem to persist sadly in both my test repos (after I pulled your changes from 9c25be6 )

@Danielku15
Copy link
Member Author

@marcus-pousette Be sure to rebuild and reload your vs code. I tested with your repos, everything is fine now. 😉

image
image

@marcus-pousette
Copy link

marcus-pousette commented Mar 31, 2024

My bad, I forgot to rebuild the extension 🙈 Yes, the line locations seem to work now. This is awesome!

When you have the monorepo open, do you see a "node_modules" folder in the test explorer sidebar? I kind of see duplicated tests here and the icons for running the tests only turn green if I run the node_modules one.

Will try out this extension now on a big repo I am working on

@Danielku15
Copy link
Member Author

Yes I see the node_modules and it seem to be also picked up like that in Mocha itself. Here I ran npx mocha --reporter=json with additional tests added to node_modules (+ TS_NODE_SKIP_IGNORE=true to make ts-node happy) and we can see how Mocha behaves:

image

Mocha filters out duplicates based on the suite and test names but the Plugin currently structures the tests in folders and files. This is where the difference becomes visible.

They have default ignore patterns when using watch but not when normally executing:

I just added support for the ignore option. If you change your mocharc like this, the tests are as you'd expect:

{
    "require": "ts-node/register",
    "loader": "ts-node/esm",
    "extensions": ["ts", "tsx"],
    "spec": [
      "**/test/**/*.spec.ts"
    ],
+    "ignore": [
+      "**/node_modules/**" 
+    ],
    "watch-files": [
      "src"
    ]
}

image

Not perfect from an end user perspective, I agree. But in-line with the Mocha behavior. Might be worth discussing over at https://github.com/mochajs/mocha that it should ship a default ignore for the node_modules.

@marcus-pousette
Copy link

marcus-pousette commented Apr 1, 2024

Ah, thanks for the explanation, that makes sense! Yes this update solves that problem. This is great!

I now tried it on a larger repository and these are the issues I am observing right now

  1. If the names of the tests are the same in two test files then it seems like the test runner is running both of the tests even if I only want to run one (by either click in the sidebar or the codelens run button). I updated the test mono-repo to showcase this problem. Try for running one of the tests and watch the output log

  2. I need a root tsconfig.json even if I can run npx mocha successfully in subfolders. I am not sure whether this is expected or not but normally I think you can have a monorepo without this and when you build the code the tsconfig in each sub folder will be used when building and testing. Without a root tsconfig.json you can't run npx mocha in the root, but I am not sure if this is needed/wanted?

  3. It seems like everytime I run a test it needs to build the entire project and do all type-checking etc. This might be related to (1) or (2). But it takes something like 10-15 seconds for me to run a test in a simple package in the larger repo I am working on. It feels like esbuild is building all packages even though I am running a test in one package. I was previously using the Jest vscode extension and that one did not have this problem

  4. Top level await inside of a test file does not seem to work, or experimental decorators even if they are allowed by the tsconfig

@Danielku15
Copy link
Member Author

If the names of the tests are the same in two test files then it seems like the test runner is running both of the tests...

Will look into that. Mocha does not have directly some "unique ids" we could use to reference tests so it rather works with passing in suite and test names to the npx mocha command. But it should be possible to generate some better arguments in such cases.

I need a root tsconfig.json even if I can run npx mocha successfully in subfolders...
It seems like everytime I run a test it needs to build the entire project and do all type-checking etc. ...

I think this is not related to this extension but Mocha in general. If you have concerns regarding this you might reach out to the Mocha devs to discuss potential improvements.

It's just a gut feeling but it could be caused by 'ts-node' you are using. I personally prefer tsx as package to run TypeScript directly in node. It uses ESBuild underneath which is very efficient in translation and rather skips most of the TSC stuff itself. This extension here also uses ESBuild to transpile TS to JS when discovering the tests.

Here an example mocharc from one of my projects: https://github.com/CoderLine/alphaTab/blob/develop/.mocharc.json

Top level await inside of a test file does not seem to work, or experimental decorators even if they are allowed by the tsconfig

What specific problems are you facing? Is it the test discovery (not seeing tests, getting errors? or the the test execution. Remember: the plugin just calls node <node args> <path to mocha> <mocha args>. If test execution fails it might be a general Mocha problem, running npx mocha is a good test to see if its in the extension here or Mocha itself.

If you have discovery problems it might be simply the fact that I do not load any tsconfig yet. Even though ultimately I want to add this bit, it is currently within my expectations.

https://github.com/Danielku15/mocha-vscode/blob/feature/mocha-standalone/src/extract/syntax.ts#L29C29-L29C45
https://github.com/Danielku15/mocha-vscode/blob/feature/mocha-standalone/src/extract/evaluate.ts#L150

I'm grateful for any examples you can provide for testingwith. I still have to dive in how ESBuild might auto-discover configs and then pass on the right tsconfig.

This PR is just the "starting point" and certainly quite some follow up work items are still needed to get it out of alpha/beta stage. The developments here effectively just a few days old 😁

@marcus-pousette
Copy link

Will look into that. Mocha does not have directly some "unique ids" we could use to reference tests so it rather works with passing in suite and test names to the npx mocha command. But it should be possible to generate some better arguments in such cases.

Thanks!

I think this is not related to this extension but Mocha in general. If you have concerns regarding this you might reach out to the Mocha devs to discuss potential improvements.

I see. Well, in my head I was just imagining that it should not matter if I open the project at the root or at specific package in vscode. Running the tests should go equally fast. Now if I put the .mocharc.json in a package and open that folder as the root I can run the same test almost instantly (that previously took seconds to launch). But might be a mocha thing as you said... I also need perhaps read up on whats possible with vscode extension but it feeeels like it should be possible to scan for package.json in all subfolders and use the mocha config file that is closest, and only force esbuild to interact with that specific folder when running the tests.

It's just a gut feeling but it could be caused by 'ts-node' you are using. I personally prefer tsx as package to run TypeScript directly in node. It uses ESBuild underneath which is very efficient in translation and rather skips most of the TSC stuff itself. This extension here also uses ESBuild to transpile TS to JS when discovering the tests.

Using tsx made things a bit faster ! but still about 10 seconds to launch some tests 🤔

What specific problems are you facing? Is it the test discovery (not seeing tests, getting errors? or the the test execution. Remember: the plugin just calls node . If test execution fails it might be a general Mocha problem, running npx mocha is a good test to see if its in the extension here or Mocha itself.

I see, well I tested that to make sure this was not the case. Also updated the monorepo now to showcase that this only occurs for the test explorer which is interesting.

If you have discovery problems it might be simply the fact that I do not load any tsconfig yet. Even though ultimately I want to add this bit, it is currently within my expectations.

Aaah that would explain this behaviour I think 🙂

Thanks for the links to the code, I willl play around with the esbuild transform function a bit to see if I get something simple working but the format: 'cjs' // vm.runInNewContext only supports CJS comment makes me think there will be issues down the road. Just setting that to esm and/or providing a tsconfigRaw with proper configuration does not seem to work as I expect..

This PR is just the "starting point" and certainly quite some follow up work items are still needed to get it out of alpha/beta stage. The developments here effectively just a few days old 😁

Totally understand. I am amazed how fast you accomplish things! 👍👍

@marcus-pousette
Copy link

marcus-pousette commented Apr 1, 2024

Might have to use https://nodejs.org/api/vm.html#class-vmmodule instead of vm.runInNewContext to get the esm parts to work. Looks like top level await does not work with cjs output format (evanw/esbuild#253)

@marcus-pousette
Copy link

I am just writing this here for memory but it also seems like when using tsx instead of ts-node the line numbers get wrong so the debugger breakpoints does not work and error message does not show up on the right place when test fails. This is not related to this extension because I also observe this behaviour when running npx mocha

@marcus-pousette
Copy link

marcus-pousette commented Apr 2, 2024

Seems like it is impossible to override the spec arg with specific files to run (?). '--run' options does not seem to have any effect. Anyway here is a pretty ugly interesting hack to make test run correctly

import crypto from 'crypto';
import { tmpdir } from 'os';

// @ts-ignore
import { loadConfig } from 'mocha/lib/cli/config'
... 
async getMochaSpawnArgs(customArgs: readonly string[]): Promise<string[]> {
    this._pathToMocha ??= await this._resolveLocalMochaPath('/bin/mocha.js');

    // if custom args contains one or more "--run" take each argument and store in array

    let runFiles:string[] = [];
    const path = require('path');
    let newCustomArgs: string[]  = [];
    for(let i = 0; i < customArgs.length; i++){
      if(customArgs[i] === "--run"){
        runFiles.push(path.resolve(customArgs[i+1]));
        i++;
      }
      else{
        newCustomArgs.push(customArgs[i]);
      }
    }


    // create a tmp .mocarc.json file with the run files inside of the spec and merge other settings found in the .mocarc.json file at path this.uri.fsPath
    let currentConfig = loadConfig(this.uri.fsPath) 
    
    // create a tmp file with a fixed name so that we don't unecessarely leak memory every test run
    // by hashing this.uri.fsPath
    const fileName = crypto.createHash('sha256').update(this.uri.fsPath).digest('hex') + "-mocharc.json";
     const tempPath = path.join(tmpdir(), fileName)
    const data = {
      ...currentConfig,
      spec: runFiles,
    };

     fs.writeFileSync(tempPath, JSON.stringify(data));

    return [
      await this.getPathToNode(),
      this._pathToMocha,
      '--config',
      tempPath,
      ...newCustomArgs,
    ];
  }

This change also seem to fix all the performance problems !

The other vscode extension (https://github.com/hbenl/vscode-mocha-test-adapter) seem to spawn a child process and interact with the 'mocha' lib directly to add all tests

@Danielku15
Copy link
Member Author

Might have to use https://nodejs.org/api/vm.html#class-vmmodule instead of vm.runInNewContext to get the esm parts to work. Looks like top level await does not work with cjs output format (evanw/esbuild#253)

I tapped into using ESM instead of CSJ in the beginning but it currently seems not possible to use it. Beside the fact that it is an experimental feature, you need to know in advance the named exports of a module which are imported by the test file. Returning a dynamic proxy as default export still leads to errors.
https://nodejs.org/api/vm.html#class-vmsyntheticmodule
https://nodejs.org/api/vm.html#modulelinklinker

I am just writing this here for memory but it also seems like when using tsx instead of ts-node the line numbers get wrong so the debugger breakpoints does not work and error message does not show up on the right place when test fails. This is not related to this extension because I also observe this behaviour when running npx mocha

The problem seems to be known: privatenumber/tsx#506

Seems like it is impossible to override the spec arg with specific files to run (?). '--run' options does not seem to have any effect.

An interesting finding. Its true that Mocha combines the --config (or autodetected config) and additional args parsed and this might result in problems filtering the tests.

https://github.com/mochajs/mocha/blob/e263c7a722b8c2fcbe83596836653896a9e0258b/lib/cli/options.js#L239-L243

Writing a config file for each test run seems an overkill and I would have also performance and synchronization concerns. According to this logic it might be best to --ignore everything and then re-add individual files via --file.

The other vscode extension (https://github.com/hbenl/vscode-mocha-test-adapter) seem to spawn a child process and interact with the 'mocha' lib directly to add all tests.

I wanted to avoid this path. I somehow have security and performance concerns evaluating all test files in full extend as part of the test discovery. Its more a gut feeling than something proven.

Using a custom UI combined with --dry-run should technically work but it will also require to either spawn and maintain a long running process with mocha for executing this part, or we would spawn processes for every test file.

The test discovery is already configurable. It could become an opt-in for devs to choose this mode in their project.

@Danielku15 Danielku15 force-pushed the feature/mocha-standalone branch from 8701325 to 3f1ee6a Compare April 2, 2024 12:13
@Danielku15 Danielku15 marked this pull request as ready for review April 2, 2024 12:24
@Danielku15 Danielku15 merged commit 57f8e7e into main Apr 2, 2024
12 checks passed
@Danielku15 Danielku15 deleted the feature/mocha-standalone branch April 2, 2024 12:24
@Danielku15
Copy link
Member Author

I merged this PR now as a foundation work.

I organized a bit the work in https://github.com/orgs/CoderLine/projects/15/views/1
Further discussions and bugreports we can handle over at https://github.com/CoderLine/mocha-vscode/discussions and https://github.com/CoderLine/mocha-vscode/issues

@marcus-pousette
Copy link

The problem seems to be known: privatenumber/tsx#506

Yeap. But with Node 18.12 I can get the locations to work with ts-node but never succeeded with tsx no matter Node version.

According to this logic it might be best to --ignore everything and then re-add individual files via --file

I tried thefile arg on its own and it did not work. but never used theignore, so it sounds possible. Hopefully this will get the same performance improvement as rewriting the spec everytime (although with this change the test runs instantly now in my big repo)

@marcus-pousette
Copy link

Further discussions and bugreports we can handle over at https://github.com/CoderLine/mocha-vscode/discussions and https://github.com/CoderLine/mocha-vscode/issues

Sounds good!

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.

2 participants