Broken IntelliJ integration when tests are running in parallel

[segrey]

Prerequisites

• Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
• Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
• 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
• Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend that you not install Mocha globally.

Description

IntelliJ integration with Mocha is broken when tests are running with --parallel (https://youtrack.jetbrains.com/issue/WEB-46034).

Steps to Reproduce

Open IntelliJ IDEA / WebStorm and run Mocha tests with --parallel added in "Extra Mocha options" in Mocha run/debug configuration.

Expected behavior:

Test results are presented in UI correctly (as they are presented when running without --parallel).

Actual behavior:

No test results. IntelliJ mocha reporter fails with exceptions like:

warn mocha-intellij: TypeError: Cannot read property 'length' of undefined
    at Object.joinList (/home/segrey/work/intellij-master/plugins/NodeJS/src/js/mocha-intellij/lib/mochaIntellijUtil.js:95:23)
    at getLocationPath (/home/segrey/work/intellij-master/plugins/NodeJS/src/js/mocha-intellij/lib/mochaIntellijReporter.js:56:15)
    at findOrCreateAndRegisterSuiteNode (/home/segrey/work/intellij-master/plugins/NodeJS/src/js/mocha-intellij/lib/mochaIntellijReporter.js:20:26)
    at registerTestNode (/home/segrey/work/intellij-master/plugins/NodeJS/src/js/mocha-intellij/lib/mochaIntellijReporter.js:92:19)
    at startTest (/home/segrey/work/intellij-master/plugins/NodeJS/src/js/mocha-intellij/lib/mochaIntellijReporter.js:108:16)
    at ParallelBufferedRunner.<anonymous> (/home/segrey/work/intellij-master/plugins/NodeJS/src/js/mocha-intellij/lib/mochaIntellijReporter.js:328:5)
    at ParallelBufferedRunner.<anonymous> (/home/segrey/work/intellij-master/plugins/NodeJS/src/js/mocha-intellij/lib/mochaIntellijUtil.js:129:17)
    at ParallelBufferedRunner.emit (events.js:315:20)
    at /home/segrey/temp/mocha-mising-title/node_modules/mocha/lib/nodejs/parallel-buffered-runner.js:111:16
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

Reproduces how often: always

Versions

• The output of mocha --version and node node_modules/.bin/mocha --version: 8.1.1
• The output of node --version: v14.4.0
• Your operating system
  • name and version: Linux
  • architecture (32 or 64-bit): 64-bit
• Your shell (e.g., bash, zsh, PowerShell, cmd): bash
• Your browser and version (if running browser tests):-
• Any third-party Mocha-related modules (and their versions):none
• Any code transpiler (e.g., TypeScript, CoffeeScript, Babel) being used (and its version):none

Additional Information

Found the following problems when debugging the issue:

• Impossible to traverse from test up to the root using test.parent.parent.(etc.).
• Root suite doesn't have suite.root === true
• For the same test, reporter's callbacks are invoked with different test objects. For example, 'test' and 'pass' callbacks get called with different test objects for the same test. This makes it impossible to associate extra data with tests.

Here is a small reporter reproducing these issues:

const BaseReporter = require('mocha/lib/reporters/base');
const INTERNAL_DATA_KEY = '_my_reporter_internal_data';

function getPathFromRoot(test) {
  const path = [];
  let s = test;
  while (s != null && !s.root) {
    path.push(s);
    s = s.parent;
  }
  path.reverse();
  let result = '';
  path.forEach((n) => {
    if (result.length > 0) {
      result += " > ";
    }
    result += "'" + n.title + "'";
  });
  return result;
}

function isRunning(test) {
  const data = test[INTERNAL_DATA_KEY];
  return data != null && data.running;
}

function markAsRunning(test) {
  return test[INTERNAL_DATA_KEY] = {running:true};
}

function MyReporter(runner) {
  BaseReporter.call(this, runner);

  runner.on('test', function (test) {
    console.log('Test started: ' + getPathFromRoot(test));
    markAsRunning(test);
  });

  runner.on('pass', function (test) {
    console.log('Test passed: ' + getPathFromRoot(test) + ', started: ' + isRunning(test));
  });

  runner.on('fail', function (test, err) {
    console.log('Test failed: ' + getPathFromRoot(test) + ', started: ' + isRunning(test));
  });
}

module.exports = MyReporter;

Here is expected and actual outputs when running the reporter with the following test data:

// test.js
it('foo', () => {
});

describe('bar', () => {
  it('foo2', () => {
  });
});

describe('baz', () => {
    describe('bar', () => {
        it('foo3', () => {
        });
    });
});

Expected (running without --parallel)

$ mocha --reporter ./my-reporter.js test.js 
Test started: 'foo'
Test passed: 'foo', started: true
Test started: 'bar' > 'foo2'
Test passed: 'bar' > 'foo2', started: true
Test started: 'baz' > 'bar' > 'foo3'
Test passed: 'baz' > 'bar' > 'foo3', started: true

Actual (running with --parallel)

$ mocha --parallel --reporter ./my-reporter.js test.js 
Test started: 'undefined' > 'foo'
Test passed: 'undefined' > 'foo', started: false
Test started: 'undefined' > 'foo2'
Test passed: 'undefined' > 'foo2', started: false
Test started: 'undefined' > 'foo3'
Test passed: 'undefined' > 'foo3', started: false

The difference:

• 'undefined' > 'foo3' instead of 'baz' > 'bar' > 'foo3'
• 'undefined' > 'foo' instead of 'foo'
• started: false instead of started: true

[segrey]

@boneskull What do you think about this issue?

[boneskull]

It's likely not going to work. The test objects are not the same; they are shallow copies and lack certain properties, as you see.

To support yr use-case, IntelliJ would need to attach a reporter to the underlying worker process. This is currently not possible, but happy to discuss an API that'd make it work.

[segrey]

Sure, let's discuss an API. Would be great to use the same API for both parallel and non-parallel runners.

There is no such requirement that the test objects are the same. We just need a way to associate some data with a test object. For example, if a test object could provide an unique key (if different test objects reference the same test, the keys would be the same), that would work.

[boneskull]

the way the parallel mode works is that the main process uses the user's chosen reporter, but it has a special Runner. this Runner creates a pool of subprocesses, and those subprocesses run the tests. each subprocess uses fixed reporter ("buffered" reporter) that doesn't do anything other than aggregate events in memory. at the end of a test file run, the subprocess takes the events stored by the buffered reporter and sends them to the main process, which then emits the events to the user's reporter like usual.

here are two approaches we could take to solve yr issue:

1. since we can't send a self-referential object between processes, we could use these unique IDs to rebuild the object hierarchy (parents etc). this will have a negative performance impact (I tried this already) and is not usually necessary (mocha itself does not need them in its builtin reporters), but maybe we could expose an API to do so. it would be helpful in debug mode at any rate--we could automatically enable it if we detect the process has an attached debugger. and your reporter could use it as well.

2. expose an API to define a custom reporter for worker processes. as mentioned, ours currently just aggregates events in memory, but perhaps you'd like to receive those events via some other method. not sure what's ergonomic here. IMO not suitable for a command-line option. maybe just a method on the Mocha instance you could call. this should not require a custom reporter on the main process; one should be able to define a custom reporter for the subprocess w/o needing to do so for the main process.

also cc @jkrems (wrong jan; @jan-molak instead), I think has a similar use case.

[boneskull]

@segrey please see #4409

[segrey]

@boneskull Sorry for the delay. Just tried it and it worked great! However, I've played a bit more with parallel mode and looks like I missed that the main process reporter receives events after the test file run has been finished. Unfortunately, it leads to the following UX problems:

• Less interactive test run: UI cannot show currently running test. For example, it's convenient to see currently running test when debugging it.
• Messages printed to stdout/stderr during test run cannot be associated with the test in the test tree in UI, because IDE receives the printed messages right away, and test/pass/fail events are buffered and delivered later.

Seems like the easiest way is to take the second approach and attach the reporter to worker processes. Well, when two tests from different worker processes would print messages to stdout, the messages would still be attached to wrong test, but probably this can be fixed in IntelliJ reporter if process.stdout/process.stdout streams would be substituted with its own.

maybe just a method on the Mocha instance you could call. this should not require a custom reporter on the main process; one should be able to define a custom reporter for the subprocess w/o needing to do so for the main process.

Yes, a method on the Mocha instance look good. How can one fetch the Mocha instance? Up until now, IntelliJ consumes Mocha API via attaching a reporter. Maybe it's possible to attach the reporter to worker processes and deattach it from the main process in the reporter constructor? When running in parallel mode of course and do nothing when running in non-parallel mode. In this case, IntelliJ would still pass --reporter /path/to/mochaIntellijReporter.js as previously.

[boneskull]

Less interactive test run: UI cannot show currently running test. For example, it's convenient to see currently running test when debugging it.

this means there will be many "currently running" tests, but you probably realize that.

I think regarding the API, since the reporter only interfaces with a Runner instance, it makes the most sense to expose a method on Runner to do this--override the default "worker" reporter with a new reporter at path. It would still require a main process reporter, however. Something like this:

// parallel-reporter.js
class IntelliJParallelReporter {
  constructor(parallelRunner, options = {}) {
    parallelRunner.workerReporter(require.resolve('./worker-reporter.js'));

    // whatever else is already there
  }
}

// worker-reporter.js

const ParallelBufferedReporter = require('mocha/lib/nodejs/reporters/parallel-buffered');

class IntelliJWorkerReporter extends ParallelBufferedReporter {
  constructor(workerRunner, options = {}) {
    // do stuff
  }
}

In the above, I'm assuming IntelliJParallelReporter is a reporter used in parallel mode only (you'd be responsible for --reporter parallel-reporter.js. If you'd rather only have a single "main process" reporter--used whether the user is in serial or parallel mode--we can ensure that a flag, e.g., isParallelMode is passed into the reporter's constructor (in the options object), and you can check it and tailor the behavior accordingly.

Also FYI: in serial mode, the first parameter to a reporter's constructor is a Runner instance, and in parallel mode that instance is ParallelBufferedRunner which extends Runner. This would enable an runner instanceof Runner check, which would return true in serial mode (though it's unlikely this will ever change, I can't guarantee it).

This will also necessitate making ParallelBufferedReporter a public API.

[boneskull]

@segrey Please let me know if that sounds OK, and I'll add a proof-of-concept to #4409.

[segrey]

this means there will be many "currently running" tests, but you probably realize that.

Yes, that's right.
Regarding debugging tests in parallel: let's consider a case when execution is suspended by debugger in one worker process and a user debugs something. Seems it'd inconvenient if executions of other worker processes are suspended by debugger too. Because a user will be disturbed by new opened debug sessions, lost focus, changed UI, etc. Am I right that passing --jobs=1 will do the trick and tests will be run in serial mode?

Thank you for the details! It sounds very good.

If you'd rather only have a single "main process" reporter

Yes, a single "main process" reporter is preferable, because IDE don't know exactly whether --parallel option is passed or not, when a configuration file is used (e.g. .mocharc.js).

e.g., isParallelMode is passed into the reporter's constructor

Seems isParallelMode is a bit more preferable than runner instanceof ParallelBufferedRunner as IntelliJ knows less about Mocha's internals.

There is a thing. Since IntelliJ reporter doesn't depend on mocha in its package.json, it might not be able to access mocha's internal classes when require is redefined to reject requiring packages not listed as direct dependencies, e.g. it's the case with Yarn 2 PnP. What do you think about exposing in API needed mocha's internal classes, e.g. require('mocha/lib/nodejs/reporters/parallel-buffered')?

BTW, IntelliJ reporter already tries to require('mocha/lib/reporters/base.js') and require('mocha/lib/utils'). Would be great to take care of them too.

[boneskull]

Yes, those modules should be public APIs (if they are not already--the reporter you mentioned is not but should be)

[boneskull]

rather sorry the parallel buffered reporter is not but should be. base should already be public

[segrey]

@boneskull Hmm, sorry, I looked through mocha's code and couldn't find a way to access require('mocha/lib/reporters/base.js') via API. Could you please suggest the right way?
BTW, --jobs=1 suppresses parallel mode as expected. Great! It will be passed when debugging tests.

[boneskull]

that should be in Mocha.reporters.Base or require('mocha').reporters.Base

I've updated #4409 to include an isParallelMode() method on the Runner. this will always return false for Runner, and true for ParallelBufferedRunner.

I've also:

• added a workerReporter() method to ParallelBufferedRunner which you can use to swap out the reporter the worker uses
• refactored the default parallel reporter, ParallelBuffered (lib/nodejs/reporters/parallel-buffered.js) to be public and have some methods that you may want to override if you choose to subclass it (you probably should).

so in your 'main' reporter, use runner.isParallelMode(), and if that returns true, you can call runner.workerReporter('/path/to/custom-worker-reporter.js'). that reporter will now be used by the worker process.

it sounds like you may want the ability to fiddle with the stdio of the worker processes, but please let me know how far you get with this.

[segrey]

that should be in Mocha.reporters.Base or require('mocha').reporters.Base

Ah, thanks a lot, missed that! How can I access Mocha object from the main reporter?
As I can see, #4409 doesn't expose require('mocha/lib/nodejs/reporters/parallel-buffered') on Mocha.reporters? Any plans?

Fixed IntelliJ reporters as you suggested and it's possible to run tests in parallel mode! Awesome!
A small question:
is it guaranteed that:

• runner.on('start', ...) (of the main IntelliJ reporter) gets called before spawning any worker process
• runner.on('end', ...) (of the main IntelliJ reporter) gets called after all worker process are terminated
  ?

It's needed to send testingStarted and testingFinished events to IntelliJ and we need to make sure that all events from worker processes are sent strictly between them.

have some methods that you may want to override if you choose to subclass it (you probably should).

Yes, I subclassed it, but didn't override any methods.

it sounds like you may want the ability to fiddle with the stdio of the worker processes, but please let me know how far you get with this.

So far it works good without any changes to stdio. I'll let you know if it is needed.

[segrey]

@boneskull Sorry for bothering, any updates on this? Seems it's mostly fixed, at least it works for me locally with #4409.

[boneskull]

Ah, thanks a lot, missed that! How can I access Mocha object from the main reporter?
As I can see, #4409 doesn't expose require('mocha/lib/nodejs/reporters/parallel-buffered') on Mocha.reporters? Any plans?

To get at Base, you can do require('mocha').reporters.Base.

The problem with exposing the ParallelBuffered reporter this way is that it should not get shipped to a browser. Further, ParallelBuffered is not designed to be used directly (e.g., via --reporter parallel-buffered). So it breaks the convention. I'll try to think of a better way to expose it, but just use the module path for now. It should also be made a public API, if it is not already.

A small question:
is it guaranteed that:
runner.on('start', ...) (of the main IntelliJ reporter) gets called before spawning any worker process
runner.on('end', ...) (of the main IntelliJ reporter) gets called after all worker process are terminated
?
It's needed to send testingStarted and testingFinished events to IntelliJ and we need to make sure that all events from worker processes are sent strictly between them.

• EVENT_RUN_BEGIN ('start') is guaranteed to be emitted by ParallelBufferedRunner before spawning any worker processes.
• EVENT_RUN_END ('end') is not guaranteed to be called after all worker processes are terminated. This should be guaranteed--but isn't--because Better worker termination josdejong/workerpool#32. Once PR Pool#terminate() waits until subprocesses are dead to fulfill josdejong/workerpool#176 is merged, released, and Mocha pulls it in, we can make this guarantee.

Suggestion: require('mocha').Runner.constants to get at the event names instead of hardcoding them.

[segrey]

Thanks for the information. The problem with require('mocha') or require('mocha/some/inner/module') calls from mocha-intellij is that the calls will fail in Yarn 2 PnP environment (and possible some other environments) as mocha is not a dependency of mocha-intellij actually. So far, IntelliJ integration assumes that these calls might fail and it still functions normally without it (kind of graceful degradation). The difference with require('mocha/lib/nodejs/reporters/parallel-buffered.js') is that IntelliJ won't be able to run tests in parallel mode in such environments (Yarn 2 PnP). For now, we can simply agree on such behavior, but in future, I'd really love to have a more reliable access to mocha's internal. For example, what do you think about exposing needed internal objects on runner object as it's already available in reporters? Something like

function IntellijReporter(runner, opts) {
  // runner.internals.utils === require('mocha/lib/utils.js')
  // runner.internals.BaseReporter === require('mocha/lib/reporters/base.js')
  // runner.internals.ParallelBufferedReporter === require('mocha/lib/nodejs/reporters/parallel-buffered.js')
}

Suggestion: require('mocha').Runner.constants to get at the event names instead of hardcoding them.

Great, thanks!

[boneskull]

Mocha makes the assumption that a custom reporter will have a peerDependency of mocha. I'm not sure what that means for IntelliJ. But it does mean that require('mocha/some/deep/module') is expected to work. I don't think this is an unreasonable assumption... you need mocha somewhere if you expect a Mocha reporter to do anything, right?

I want to be clear, though: I am not going to go out of my way to support Yarn 2 PNP, and won't consider Yarn's limitations when making design decisions in Mocha. You may have more luck with other Mocha maintainers, though 😄

that said, proposing a more ergonomic way of defining a reporter is definitely on my to-do list. ideally, you will not need to require('mocha') in a reporter or an interface. if you don't mind, I'll @-mention you when I have a PR ready; your feedback would be very helpful!

[segrey]

Thanks for clarifying, understandable.

Mocha makes the assumption that a custom reporter will have a peerDependency of mocha. I'm not sure what that means for IntelliJ. But it does mean that require('mocha/some/deep/module') is expected to work.

For normal reporters that are declared in user's package.json, require('mocha/some/deep/module') will work in Yarn 2 PnP for sure. But things are different for IntelliJ reporter - it's not declared in user's package.json and is located inside IntelliJ installation. That's why even in normal environment require('mocha') from IntelliJ reporter fails, so IntelliJ reporter uses an absolute path to require mocha: require('/path/to/mocha'). Hopefully this gives a clearer understanding of how IntelliJ integrates with mocha.

Regarding PR, thanks, looking forward to it!