feat: add ESM support to jasmine_test()

[dgp1130]

Fixes #33.

This introduces a new module option which chooses between CommonJS and ES modules. Based on this input, jasmine_test() will choose the correct format of the JUnit reporter. This does not enforce that actual tests are executed as ES modules, those still need to be configured in Jasmine's config file. See: https://jasmine.github.io/setup/nodejs.html#using-es-modules.

I tried to keep things simple and just duplicated the reporter with native import statements. There's a bit of unnecessary duplication there, but I think the file is simple enough that it doesn't really matter too much. Fortunately the JUnit reporter is the only thing which needs to be changed to be compatible with ESM. As long as you name the test files .mjs, Jasmine will load them via dynamic import.

[gonzojive]

Worked for me. Thanks for this!

[jbedard]

Do you have a test that can be added? Something that fails without this...

[dgp1130]

Do you have a test that can be added? Something that fails without this...

Thanks for bringing this up, totally forgot about that. Actually looking at existing test cases, it seems they all already use ESM. I was a bit surprised about that and started digging a bit more into the failure I was seeing. Turns out I was experiencing a bug where junit_reporter.js incorrectly uses the "type": "module" from the root package.json file of the user's workspace. The reasoning for this is because files are laid out like so at runtime:

.../execroot/${my_wksp}/bazel-out/${cfg}/bin/external/jasmine/junit_reporter.js
.../execroot/${my_wksp}/bazel-out/${cfg}/bin/path/to/pkg/index.js
.../execroot/${my_wksp}/bazel-out/${cfg}/bin/package.json

The package.json file comes from the user's workspace at //:package.json and is intended to apply to only their code. However, Node walks up external/jasmine/junit_reporter.js and finds the same package.json, applying its configuration to the JUnit reporter. If the user puts "type": "module" in their package.json, it also applies to the JUnit reporter, which fails because it is authored in CommonJS.

There are a few potential solutions to this, but I think the best is to actually ship an empty package.json file at external/jasmine/package.json. That way we're protected from any configuration in there such as exports or imports which might cause similar problems.

Ironically, //e2e/jasmine_test/... had an exactly correct test case, but wasn't being run on CI and was failing at HEAD. I enabled that test and fixed it up to serve as a regression test. I also noticed that all the existing tests actually use ESM, so I made a CommonJS test leveraging the default behavior to make sure that wasn't broken as well.

We could probably migrate junit_reporter.js and runner.js to ESM or rename the files to .cjs. runner.js is a little more complicated so I didn't want to migrate that since it wasn't strictly necessary. I also didn't bother renaming the files since we'd want the package.json anyways to avoid future configuration issues. I ultimately opted not to migrate junit_reporter.js to ESM either for consistency with runner.js, but if we really want to migrate the source content for each, I can take a pass at that.

I suspect this same bug of having the user's package.json configuration apply to external workspaces could be causing other issues in other rulesets. I recently tried adding exports to my package.json the other day and encountered some very strange behavior which I haven't been able to narrow down to a specific bug I could file just yet. I wonder if that might have a similar root cause and wonder if we'll want to make this same change to all the other Aspect rulesets?

[jbedard]

Great 👍

[jbedard]

Looks like the e2e test is failing atm though?

[dgp1130]

Ah didn't realize the CI workflow expected the example to have a .bazelrc file. Added.

[jbedard]

@gonzojive does this latest change work for you?

[gonzojive]

@gonzojive does this latest change work for you?

It seems to work for me.

$ git clone https://github.com/gonzojive/rules_ts_proto
$ cd rules_ts_proto/e2e/typescript_proto_usage/
$ bazel test //...

That might not work for others due to local repositories in the workspace.

[jbedard]

Great, thanks for checking! 👍