Supporting native ES imports for jasmine and mocha

[nicojs]

Is your feature request related to a problem? Please describe.

Native ECMAScript modules or ESM for short, are a new way to import files using the import and export keywords. NodeJS 12+ supports this out of the box, see https://nodejs.org/api/esm.html for more info. Contrary to CommonJS modules, or CJS for short, which is the way nodejs has always worked (module.exports and require). Both Mocha and Jasmine support ESM.

However, although Mocha and Jasmine support them, our @stryker-mutator/mocha-runner and @stryker-mutator/jasmine-runner plugins still need to clear files from esm cache between runs. For CJS, this is done using the DirectoryRequireCache helper class. However, clearing files from cache is a feature that is not supported with EMS. Since reloading the files is important for both Mocha and Jasmine, to support ESM, you will currently have to run with --maxTestRunnerReuse set to 1, so a fresh process is created for each test run.

There are 2 reasons why all files currently always need to reload between runs:

1. These test runner plugins currently don't support what we call "hot-reload", this means that mocha and jasmine can only run once after all files are loaded.
2. Some mutants are executed as soon as your file is loaded, instead of during test execution. We call these mutants static mutants, and all your tests are executed as a result. For example:

   // hello.js
   -export const hello ='👋'
   +export const hello = ''

   Mutation switching will place this mutant correctly, but since the file is only loaded once, it will never execute after being activated. Stryker does know exactly which mutant is static, as long as --coverageAnalysis is not "off"

Describe the solution you'd like
I can think of a couple of features to improve ESM support:

1. Implement hot reload for both jasmine and mocha (this is a minimal requirement).
2. Allow static mutants to be skipped (report them as Ignored). This is helpful for performance as well, since static mutants are a disaster for performance, especially with large test sets (we run all tests for them by default).
3. Restart a test runner worker just before it will run a static mutant.

Describe alternatives you've considered

• We could try to do some funky business with a custom loader, inspired by this blog article,
  • ❌ This will remove the ability for custom loaders in the user space
  • ❌ It might be impossible to workaround memory leaks
• Experiment with the VM module. Interesting read: esm-with-node-vm

Additional context

I don't think the Jest and karma test runner plugins currently have issues with ESM. Jest "supports" them by simply transpiling the import to (old) CJS syntax. Karma reloads the page between runs, which will automatically clear the internal ESM cache,

[nicojs]

I've taken a better look at the VM Module class. It is interesting stuff and might work, but it is still very experimental. I don't want to rely on an experimental feature.

Using a VM module will also not allow users to use a custom loader:

This implementation lies at a lower level than the ECMAScript Module loader. There is also no way to interact with the Loader yet, though support is planned.

[nicojs]

We've discussed it during the community meeting today.

We've currently decided to:
1 -> Try to add "hot reload" for mocha-runner and jasmine-runner without a setting, it should be the fastest way to run the tests anyway, so why provide a setting for it?
2 and 3: Add a staticMutants setting. It can have 3 values. Choose to "ignore", "run", or "restart" static mutants. We can keep the default "run" (what we have now) and change the default whenever ESM gets more traction.

We can also try to find out if we can discover projects that use ESM, so we can warn users that use the "run" setting. Simply looking in the files for import statements won't be good enough though, as a lot of users transpile those to CJS syntax before running the tests.

[nicojs]

With regards to staticMutants...

I've been thinking about this a lot lately. Some pain could be taken away if we simply knew whether a file is executed as an ESM or simple (cjs) script. This could eliminate the requirement for staticMutants being an enum. It instead could simply be a boolean renamed as ignoreStatic (default would be false).

Any static code analysis is pointless, as almost everyone uses a code bundler or transpiler. import foo from 'foo' is probably transpiled to const foo = require('foo'); or similar. But we might be able to solve it with runtime analysis.

See https://stackoverflow.com/questions/68631696/how-to-know-whether-a-piece-of-js-is-executed-in-an-es-module-or-a-regular-scrip/ (thanks @JohnGorter 😘)

We could add some instrumentation atop each file:

__stryker__.files = __stryker__.files || {};
__stryker__.files['current/file/name.ts'] = {
  esm: this === undefined
}

And require the test runner plugin to provide this information to the core StrykerJS process. That way we would know exactly which file is an ESM and which is a regular script.

There is one more point. Both Jest and Karma have the ability to run static mutants without having to restart the process. So there should be some way for test runners to communicate this to the core StrykerJS process. I think the simplest way would be to add an optional capabilities property.

interface TestRunner {
  capabilities?: TestRunnerCapabilities;
}

interface TestRunnerCapabilities {
  runStaticMutants?: boolean;  
}

[simondel]

Having a single boolean would be better as the user wouldn't have to have that much knowledge about what static mutants are.

As discussed in person:

• It would be nice to log an informational message when static mutants have been ran and how long it took to ran them. Combined with information on how to turn off the setting.
• Webpack might not work well with the check if this === undefined as the module may be called while applying a different this

[nicojs]

Note: running a Jasmine instance multiple times is now also supported: https://github.com/jasmine/jasmine/blob/main/release_notes/3.10.0.md#new-features-and-bug-fixes

[nicojs]

Note: work on --ignoreStatic has started here: #3282

[nicojs]

I don't feel comfortable in our abilities to "discover" the module type during dry-run. That's why I propose adding an option setting for it. I think it would be elegant to keep it in sync with nodejs and the browser:

{
  "type": "module"
}

The default value can be discovered from the package.json. That way, we do the right thing most of the time when the type is not explicitly set inst stryker configuration.

See https://nodejs.org/dist/latest/docs/api/packages.html#type

[nicojs]

An interesting development.

Apparently, even if your project is 100% cjs, it might still be imported in the import cache. Removing it from require.cache won't help.

See this example:

// a.cjs
module.exports.a = 'a';
console.log('a.cjs loaded');

// b.mjs
export const b = 'b';
console.log('b.mjs loaded');

// entry.cjs
async function importAB() {
  const [{ a }, { b }] = await Promise.all([import('./a.cjs'), import('./b.mjs')]);
  console.log('Imported: ', a, b);
}

function requireA(){
    const { a } = require('./a.cjs');
    console.log('Required', a);
}

async function main() {
    await importAB();
    requireA();
    delete require.cache[require.resolve('./a.cjs')];
    await importAB();
    requireA();
}

main().catch(console.error);

Running node entry.cjs results in this output:

b.mjs loaded
a.cjs loaded
Imported:  a b
Required a
Imported:  a b
a.cjs loaded
Required a

As you can see, the second a.cjs loaded occurs after the second Imported: a b. This means that the second import ('a.cjs') didn't execute the a.cjs module again, only require-ing it later did that.

It actually makes sense when you think about it, but I assumed the import cache would be rightly coupled to the require.cache in this respect.

This means that the "type" I've been thinking up in my previous comment is no longer valid. Even when you're type is "cjs", we might still be unable to clear the file from the cache, since the test runner might be using import expressions.

This is the case in the recently released Jasmine 4, which is the reason it isn't working, see #3340

I'm unsure how to proceed. It might be simpler (and easier) to stop clearing files from cache altogether.

[nicojs]

I've discussed it with @simondel and we've decided to do exactly that. Not clearing files from cache altogether.

This means that mocha and jasmine test workers will have to restart between runs for static mutants.

[nicojs]

Closed with #3396