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: super basic test suite support #4

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

rickosborne
Copy link
Collaborator

This also includes the prototype for loading the runner via a script name instead of as a live object. Seems to work! This would mean we don't need to expose a Runner.addReporter function upstream.

@rickosborne rickosborne added the enhancement New feature or request label Jan 4, 2025
@rickosborne rickosborne self-assigned this Jan 4, 2025
@rickosborne rickosborne requested a review from mrazauskas January 4, 2025 17:03
test/AsMochaReporter.test.ts Outdated Show resolved Hide resolved
test/AsMochaReporter.test.ts Outdated Show resolved Hide resolved

describe("AssertionError", () => {
it("is an Error", () => {
expect<AssertionError>().type.toBeAssignableTo<Error>();
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

biome.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Because I forgot `import.meta.dirname` is only in more recent versions of node.

Also:

- added DEVELOPMENT.md, with docs on how to use fnm for testing
TIL biome's `lint` may not fail, even if `format` would make changes.  Odd, but okay.
@rickosborne
Copy link
Collaborator Author

Heads up ... you may want something like this in TSTyche's Options.ts:

  let reporterScript = path.resolve(scriptDir, "AsMochaReporter.js");
  if (/^\w:/.test(reporterScript)) {
    // Because Windows drive letters foul up ESM dynamic imports
    // with ERR_UNSUPPORTED_ESM_URL_SCHEME errors.
    reporterScript = pathToFileURL(reporterScript).toString();
  }

The one in Options.ts catches when the path starts with ., but not with a Windows drive letter:

if (optionValue.startsWith(".")) {
  optionValue = pathToFileURL(Path.relative(".", Path.resolve(rootPath, optionValue))).toString();
} else {
  optionValue = import.meta.resolve(optionValue);
}

DEVELOPMENT.md Show resolved Hide resolved
DEVELOPMENT.md Outdated Show resolved Hide resolved
DEVELOPMENT.md Outdated Show resolved Hide resolved
Comment on lines +30 to +40
You can then see everything as a user would, by running TSTyche against some example tests:

```shell
yarn test:tstyche
```

Which is just an alias for:

```shell
tstyche ./type-test
```
Copy link
Member

Choose a reason for hiding this comment

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

Hm.. Not sure this is applicable in this repo. Or?

@mrazauskas
Copy link
Member

mrazauskas commented Jan 5, 2025

The one in Options.ts catches when the path starts with ., but not with a Windows drive letter:

If I remember it right, support for absolute paths is not implemented. Ah, right! My thinking was that this must be a module specifier. Hence relative paths are resolved as URL strings to work with import().

source/mocha.ts Outdated
// Clear out the default Line and Summary reporters.
resolvedConfig.reporters = [];
resolvedConfig.reporters = [reporterScript];
Copy link
Member

@mrazauskas mrazauskas Jan 5, 2025

Choose a reason for hiding this comment

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

See #4 (comment). And also this example from TSTyche repo: https://github.com/tstyche/tstyche/blob/0261af0c5f4591329e650fad602f41ccd19d46cb/examples/example-plugin.tst.js#L10

Perhaps it is worth to simply normalize into URL string here:

Suggested change
resolvedConfig.reporters = [reporterScript];
resolvedConfig.reporters = [pathToFileURL(reporterScript).toString()];

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to rubber duck this out, because it's a little convoluted.

Use-case

Given a developer using an IDE
And a project configured with `tstyche`, `mocha`, and `tstyche-as-mocha`
And the project has some unit tests which use TSTyche `expect` and `describe`
When the developer adds a run configuration for those tests with tstyche-as-mocha
And the developer runs the configuration
Then tstyche-as-mocha will run TSTyche
And tstyche-as-mocha will translate TSTyche test results into the expected Mocha format
And the developer should be able to see those results in their IDE

Details

The execution chain looks like:

  1. IDE thinks it is running Mocha. Actually runs tstyche-as-mocha:
    node node_modules/tstyche-as-mocha --reporter /some/path/to/IDE/reporter.js --additional --mocha --options
  2. Node.js runs the bin script specified in tstyche-as-mocha/package.json, which just points to the compiled mocha.ts.
  3. The mocha.js script translates the Mocha CLI options to something TSTyche understands, specifying the path to the (compiled) AsMochaReporter.js as the only TSTyche Reporter, and runs TSTyche.
  4. TSTyche runs with the provided config, loading up the AsMochaReporter via the path provided to it.
  5. The AsMochaReporter takes the path provided by the IDE, to its Mocha Reporter, and loads it.
  6. As TSTyche runs, the AsMochaReporter translates its events to Mocha format, passing them on to the IDE's Mocha Reporter.
  7. The user sees the TSTyche results. Magic!

There are several imports happening, with various formats of specifiers:

  • The tstyche-as-mocha CLI loads TSTyche. This is straightforward, as the import specifier is just the package subpath: tstyche/tstyche.
  • As part of that config, the as-mocha CLI passes the AsMochaReport's import specifier to TSTyche.

    Hmm. I wonder if we could be clever here and make the import specifier something another subpath, like tstyche-as-mocha/reporter, and not a filesystem path? It should work, as both packages should be installed. It would break in scenarios where the developer hasn't actually added a dependency on tstyche-as-mocha ... but do we care?

  • TSTyche loads the AsMochaReporter, using whatever it's given.
  • The AsMochaReporter loads the IDE's Mocha Reporter, using the filesystem path given to the CLI.

The trick for that last one is that these paths will, in most cases, be provided as absolute filesystem paths by the IDE, and completely out of the user's control. And in many cases, will be completely obscured from the user, as all they did was click the little green "run this test" button in their IDE. But, ultimately, the AsMochaReporter will need to correctly translate them into import specifiers. For example, the one from JetBrains on macOS will end up looking like:

--reporter "/Users/ricko/Applications/IntelliJ IDEA Ultimate.app/Contents/plugins/nodeJS/js/mocha-intellij/lib/mochaIntellijReporter.js"

So I think that gives me two avenues to work on:

  1. Set up a subpath in tstyche-as-mocha so its reporter can be accessed via something like tstyche-as-mocha/reporter and not a filesystem path. This will eliminate all that really gross scriptDir stuff if it works. Yay!
  2. Add some more unit tests for the AsMochaReport to handle the various types of import specifiers it should be ready to handle.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the details. Now I understand the internals better. Using subpath sounds like a good idea indeed.


By the way, this made me intrigued:

--reporter "/Users/ricko/Applications/IntelliJ IDEA Ultimate.app/Contents/plugins/nodeJS/js/mocha-intellij/lib/mochaIntellijReporter.js"

I did not expect to see IntelliJ using JavaScript. Very surprising. Does this mean IntelliJ IDEs would be able somehow to spawn tstyche and they just need to get back report in certain shape? Not today, but I will dig around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In theory, yes. But I admit, I don't know how they accept submissions for supporting a new reporter. Maybe an issue in YouTrack? For example:

https://youtrack.jetbrains.com/issue/WEB-70517/Test-reporter-for-vitest-should-display-more-information-about-errors

For the most part, it's actually a pretty seamless UX for devs. You just write some code which looks vaguely like a describe block and the IDE gives you a set of 🟩▶️ affordances for it:

jetbrains-test-ux-1mb.mp4

You don't even have to configure anything explicitly. The IDE figures out that your project has mocha installed and just dynamically sets up a run config for it the first time you use one of the affordances. Same for Jest, Vitest, etc.

Copy link
Member

@mrazauskas mrazauskas Jan 5, 2025

Choose a reason for hiding this comment

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

What about vendoring mocha-intellij into this repo? (This is the same code they ship, right?) It did not change for eight years. Looks stable.

Might be I missed something. The benefits of vending would be:

And some years later that TSTyche reporter can be used to build native IntelliJ plugin. At the moment, this is just a dream, but I feel happy looking at this plan. What you think?

@rickosborne
Copy link
Collaborator Author

Okay, I spent some time on the CommonJS code. I did see that you can get tsc to output CommonJS, regardless of the errors it produces. However, when you try to actually run the code, it doesn't get very far:

node ./build/cjs/mocha.js --ui bdd --reporter /path/to/.../mochaIntellijReporter.js
Error [ERR_REQUIRE_ESM]: require() of ES Module ./node_modules/tstyche/build/tstyche.js from ./build/cjs/AsMochaReporter.js not supported.
Instead change the require of tstyche.js in ./build/cjs/AsMochaReporter.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (./build/cjs/AsMochaReporter.js:29:19)
    at async #addReporters (file://./node_modules/tstyche/build/tstyche.js:4355:45)
    at async Runner.run (file://./node_modules/tstyche/build/tstyche.js:4370:9) {
  code: 'ERR_REQUIRE_ESM'
}

But the funny thing is, you can't actually get that to work because tsc always transpiles the dynamic import to a require if the target module is CommonJS. (It looks like it used to work with Node16 for moduleResolution, but that loophole has since been removed.)

I even spent a bit of time doing really icky stuff like this:

const dynamicImport = new Function("return import('tstyche/tstyche')") as () => Promise<LibLike>;

That works for the mocha.ts CLI, awkwardly, but the AsMochaReporter is trickier. Its exported class is, of course:

export default class AsMochaReporter extends BaseReporter {

And that BaseReporter is the one imported from tstyche/tstyche. Which I'd have to await for, and thus can't use as a default export.

So ... long story short, as long as tstyche/tstyche is ESM-only, it looks like its Reporters are ESM-only. (You can use expect and describe from CJS, you just can't build a Reporter with it.) Which is ... unfortunate, because the JB/IJ/WS/TeamCity mocha code cannot be executed from an ESM context. It uses require.main.filename, which only exists in a CommonJS context.

I did attempt to build a CommonJS springoard:

module.exports = function requireCommon(specifier) {
  return require(specifier);
};

Which I can then use in the AsMochaReporter:

    const springBoard = (await import("../cjs/springboard.cjs")).default as unknown;
    if (typeof springBoard !== "function") {
      throw new Error("SpringBoard is not a function.");
    }
    const reporterModule = (springBoard as (specifier: string) => unknown)(reporterName);

This does actually import everything correctly ... but because node started in an ESM context, the CommonJS require.main structure is not populated, so you're back to the JetBrains Mocha reporter spewing really ugly warnings.

Yuck.

And, going through the Mocha docs:

Custom reporters and custom interfaces can only be CommonJS files

So, filing a YouTrack ticket to have JetBrains add ESM support for their Mocha Reporter library seems like it probably wouldn't go anywhere.

Okay ... path forward for me:

  • I'll look into the other built-in options (Jest, Vitest, etc) to see which are ESM-compatible. I'm not super married to Mocha, it was just the most straightforward and I figured it was the oldest and most stable.

@mrazauskas
Copy link
Member

mrazauskas commented Jan 6, 2025

Out of curiosity I created a simple project with Mocha and used mocha-intellij as reporter. All what it emits is rather straightforward. I think it is trivial to created TSTyche reporter with equivalent output. If that sounds useful, I could put it together next week.

And I also looked into IDEs source (the dmg file). There are several other reporters for different JavaScript test runners. Looks like they all produce similar output.

@mrazauskas
Copy link
Member

Quick update. I was putting together tstyche-intellij-reporter. It translates TSTyche results directly into strings consumed by IntelliJ. A shim like tstyche-as-mocha is still needed, but this will be just few lines of code.

Here is current output:

Screenshot 2025-01-10 at 13 28 00

All is left is to shape up error messages. I will do that next week and will open a PR for testing.

@mrazauskas
Copy link
Member

Almost there:

Screenshot 2025-01-14 at 10 26 21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants