Root hooks not executed when programmatically running Mocha in serial mode

[telaoumatenyanis]

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_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend that you not install Mocha globally.

Description

Hey team!

When running tests programmatically (using new Mocha), the root hooks provided in require are not executed if parallel is not set/set to false.

This is a discrepancy with the CLI method of invoking tests that correctly interprets the require flags and consume them before running the test suites.

Steps to Reproduce

index.js

const Mocha = require("mocha")

const mocha = new Mocha({
    require:["./root-hook.js"]
})
mocha.addFile("./test.js")
mocha.run()

test.js

describe("dummy test", () => {
    it("works", () => {
       console.log("inside the test")
    })
})

root-hook.js

exports.mochaHooks = {
   beforeAll() {
     console.log("before all")
   }
}

Expected behavior: Similar behavior as the CLI:

./node_modules/.bin/mocha --require root-hook.js --file test.js
(node:11908) ExperimentalWarning: The ESM module loader is experimental.


before all
  dummy test
inside the test
    ✔ works


  1 passing (3ms)

Actual behavior: The root hooks logging "before all" is not being executed

node index.js 


  dummy test
inside the test
    ✔ works


  1 passing (5ms)

Reproduces how often: 100%

Versions

• The output of mocha --version and node_modules/.bin/mocha --version: 10.2.0 (reproduced in 9.x as well)
• The output of node --version: v14.21.3
• Your operating system
  • name and version: Amazon Linux 2
  • architecture (32 or 64-bit): x86_64
• Your shell (e.g., bash, zsh, PowerShell, cmd): zsh
• Your browser and version (if running browser tests): N/A
• Any third-party Mocha-related modules (and their versions): N/A
• Any code transpiler (e.g., TypeScript, CoffeeScript, Babel) being used (and its version): N/A

Additional Information

I assume the discrepancy comes from lib/cli/run.js (cli runner) and lib/nodejs/worker.js (worker thread logic) executing handleRequires while the bare runner.js does not.

[JoshuaKGoldberg]

Huh, what an odd little discrepancy! Thanks for filing. This sounds reasonable to consider a straightforward bug to be fixed.

For quick use, I copy & pasted the OP's files into a repro here: https://github.com/JoshuaKGoldberg/repros/tree/mocha-api-not-respecting-root-hook-beforeAll.

[shawn-gang]

hi @JoshuaKGoldberg, would this be a good first issue for somebody looking to make their first open source contribution?

[JoshuaKGoldberg]

Hmm, I suspect not. It involves parallel stuff and parts of Mocha that most users don't touch. You can certainly try it but I'd be worried about getting into really deep things. We have a separate good first issue label I'd suggest trying first.

Thanks for asking though! 🙌

[shawn-gang]

@JoshuaKGoldberg challenge accepted

[shawn-gang]

hi @JoshuaKGoldberg, the API documentation states that the require parameter is the Pathname of rootHooks plugin for parallel runs., which could possibly mean that the behavior is undefined when the tests are run serially.

Changing the way the Mocha object is invoked by using the rootHooks parameter as opposed to require works around this issue.

What should the underlying fix for this issue be? I was thinking of pulling in the underlying hooks from the path in require only if the parameter require is defined, and the parameter rootHooks is not specified. But it does seem like there are multiple ways to approach this.

[JoshuaKGoldberg]

...heck if I know! 😄

#5027 - we're finishing up triaging existing issues and haven't yet started reviewing PRs in earnest. So we're not yet at the place where we can give advice like this. None of us have looked closely enough at the code to be able to be coherent in trying to ask questions like that.

It sounds like you're on the right track (yay! nicely done!) in general. But this is why I was not super enthusiastic in #5006 (comment). We can't really give you great support for this just yet.

[shawn-gang]

no worries at all. I'll take a stab at the most straightforward approach, just trying to mirror the behavior of the CLI

[shawn-gang]

Unfortunately wasn't able to come up with a solution that passed the unit and integration tests :( I think the helper function handleRequires used in the CLI should be used or mirrored in the constructor of the Mocha object, but it is async and it's necessary to avoid double dipping into that logic when called using the CLI

[tonyngfox]

Hi, do you guys have any update on this bug?
Thanks