Jest: `jest` fails to generate coverage when using the `testRegex` config option

[rafeca]

🐛 Bug Report

[email protected] fails to generate coverage when using the testRegex config param. This issue has been introduced recently by https://github.com/facebook/jest/pull/7209.

To Reproduce

A very simple end to end test can be created in e2e/__tests__/coverage_report.test.js to reproduce the issue:

test('generates coverage when using the testRegex config param ', () => {
  const {stdout, status} = runJest(DIR, [
    '--no-cache',
    '--testRegex=__tests__',
    '--coverage',
  ]);
  expect(status).toBe(0);
});

Expected behavior

jest should execute correctly when generating coverage with the testRegex config param.

Additional information

This is the stack trace from the error:

ERROR: regex.test is not a function
      STACK: TypeError: regex.test is not a function
          at config.testRegex.config.testRegex.some.regex (~/jest/packages/jest-runtime/build/should_instrument.js:64:42)
          at Array.some (<anonymous>)
          at shouldInstrument (~/packages/jest-runtime/build/should_instrument.js:64:22)
          at Function.shouldInstrument (~/packages/jest-runtime/build/index.js:241:74)
          at exports.default (~/packages/jest-cli/build/generateEmptyCoverage.js:14:51)
          at Object.worker (~/packages/jest-cli/build/reporters/coverage_worker.js:52:80)
          at execFunction (~/packages/jest-worker/build/child.js:146:17)
          at execHelper (~/packages/jest-worker/build/child.js:128:5)
          at execMethod (~/packages/jest-worker/build/child.js:132:5)
          at process.on (~/packages/jest-worker/build/child.js:43:7)

The issue seems to be caused by the fact that the coverage is generated from a worker, so the config object gets serializer when it's sent to the worker by jest-worker.

jest-worker does not have any special logic to serialize regular expressions, so it defers it to Node's child_process logic, which converts a RegExp into an empty object when serializing and deserializing it.

Potential solutions

A couple of potential solutions that occur to me:

1. Make the serialization of objects a bit smarter in jest-worker: it could even use jest-serializer for it, which handles RegExps (this may affect the performance though).
2. Do not normalize the testRegex config param to an array of RegExp objects, but to an array of strings, and generate the RegExp object on demand when needed (similarly than how it was done before https://github.com/facebook/jest/pull/7209).

[SimenB]

I prefer number 1. It's way easier to reason with the config if the type is consistent after normalisation at all times

[SimenB]

Also odd we didn't have an integration test using testRegex...

[mjesun]

We can't use jest-serializer to pass the config around. #5613 tried to do this, but it was a such a big performance killer that I abandoned the PR.

My personal opinion is that configuration files should be JS files (and not JSON, even if we support reading from package.json), and pass the config path to the workers, where they can read it. Looking at other big projects (e.g. Babel) this is exactly what they've started doing.

Probably the right moment to achieve something like this, if we wanted to, would be fixing all config inconsistencies @SimenB mentioned in #7185.

[SimenB]

How do we serialize now, using JSON.stringify? We can pass a custom serializer and deserializer to it if we want (which for RegExp might be .source to string, and new RegExp from).

Since we know the shape of the config, we might consider using e.g. https://github.com/fastify/fast-json-stringify with a schema (it serializes RegExp using .source already)

[SimenB]

However, we might want to go for option 2 in the OP, and rather fix normalize to do All The Things in conjunction with #7185. #6910 really should work (e.g. having a JS config file and using actual RegExp instances)

[mjesun]

We rely on Node's IPC communication for forked processes, which uses JSON.stringify + \n terminated messages under the hood. However, the IPC mechanism is hooked more directly onto the fd calls, so it's way faster than manually serializing and establishing the connection ourselves. In general it's going to be complicated to beat Node's protocol.

[SimenB]

I don't follow how some serializer doing e.g. worker.send({globalConfig: serialize(globalConfig)}) would be particularly slower than worker.send({globalConfig: globalConfig}) if it uses JSON.stringify under the hood? It would have to escape the string we pass I guess, but is the hit so bad? We would also only use this when passing the global config, all other operations would be untouched.

---

Alternative approach would be to write the write the config to disk somewhere (as a js file?) and pass the path that the worker calls require on.

[mjesun]

You'd have an extra serializer operation which is not really needed, and consumes time. If we want to do that hoever, we can already do it and there is no need to change jest-worker.

[rubennorte]

I had similar issues when I replaced Object with Map in the data structures used by the haste map, and I did a custom serialization there (serialization, deserialization). We could do the same here until we find a general solution to this problem.

[rubennorte]

I just remembered that @rafeca implemented the setup and teardown methods for workers. Can't we pass the configuration in the worker startup? Using a custom serialization function wouldn't impact that much in that case.

[mjesun]

Yeah, it is implemented for that specific purpose. However, the setup call is automagic, and now it's firing back us in Metro when trying to just require('./worker') when using --maxWorkers=1, since you need to manually perform the call.

It's still solvable with an if but it's increasing complexity for free. I'd go for a setup method, but we should explicitly call it; for instance:

const myWorker = maxWorkers === 1
  ? require('./worker')
  : new JestWorker(require.resolve('./worker'));

await myWorker.setupConfig(config);

This way you don't need any additional ifs.

[rubennorte]

@mjesun not totally related to this, but given that the maxWorkers: 1 is quite common both in Metro and in Jest, have you considered implementing it directly in jest-worker? E.g.:

const myWorker = new JestWorker(require.resolve('./worker'), {
  maxWorkers,
  createNewProcesses: maxWorkers > 1, // or !config.runInBand
});

[rafeca]

As a short-term fix, the easiest thing IMHO would be to implement the second solution that I suggested. It implies very short amount of changes and it will leave the logic as it was before #7209 (but without removing the feature of having an array of regexps).

After we get this fixed (this issue is blocking the deploy of the new version of jest at FB) we can discuss what's going to be the best way to pass the configuration to the workers, what do you folks think?

[rubennorte]

@rafeca see #7251 ;)