🛠️ Repo: e2e test helpers could be better (test/integration/helpers.js)

[boneskull]

As part of PR #4241, I was tweaking a few things in our test helper module, which helps Mocha call itself in a child process for end-to-end testing. It got me thinking a bit...

This is the current situation. I think we can do better:

1. runMocha/runMochaAsync spawns lib/cli/cli.js and gathers some basic information about the run. Attempts to parse some information from the output. Because of this, usually not safe to capture STDERR or make assertions about error-related behavior. No access to child process handle.
2. runMochaJSON/runMochaJSONAsync spawns lib/cli/cli.js --reporter=json and gathers more detailed information, but an error coming out of Mocha (or capturing of STDERR) will cause the JSON.parse() call to error out, so best used for "happy path" tests. No access to child process handle.
3. invokeMocha/invokeMochaAsync spawns bin/mocha (which may, in turn, spawn a child process) and provides raw output. The raw output can be subsequently parsed a la runMocha/runMochaAsync if Mocha doesn't output something interfering with the regex-based parsing code (getSummary()). This is best used for testing errors, exceptions, etc, or interacting (non-IPC) with the subprocess.
   • invokeMocha returns the child process handle
   • invokeMochaAsync returns a tuple of the child process handle and the Promise

Some observations:

• invokeMochaAsync is awkward to work with because of its return value.
• most of the time we don't need to send signals to the child process (only possible with invoke*)
• we do often need to check the output when Mocha has thrown an exception or errored out, and the best way to do that is with invoke*
• when dealing with errors, we almost always need to capture STDERR
• nearly all of the time, we don't need to pass flags to node, which only invoke* supports.
• when we capture STDERR, we weave it in with STDOUT into a single string in the result (output), which causes parsing issues in runMocha*
• it's not clear when to use any of these from the function names.
• "smart" fixtures names--where you don't need to provide a full fixture path--are not possible with invoke*
• only invoke* supports not passing a list of files to test

none of this is super-high-priority, but more obvious, consistent methods would absolutely make things easier for contributors.

if there was a proposal, I think it should be something like:

1. Make a single method for general-purpose use:
   1. It should accept an option which would contain node flags. If these are present, use bin/mocha, otherwise use lib/cli/cli.js
   2. It should use execa under the hood, which does an excellent job of managing STDERR, STDOUT, & the like; it will return a result which contains each, and a third property which contains the "woven" output. we won't have to concern ourselves with passing stdio or pipe or whatever
   3. It should attempt to parse the output (failed, passing, pending, total, etc) . If it cannot parse the output, it should not throw an exception, but it should add a property to the result containing an exception
   4. It should always return a Promise unless a callback is supplied.
   5. Another option flag, e.g., detailed, will attempt extra parsing; it will force use of --reporter=json. If this fails, again, an exception should not be thrown but it should be returned in the result object
   6. It would contain the "smart" fixture behavior, but would skip this if no fixture was provided.
   7. We should be able to use --watch, and --bail when running a test file using this function w/o breaking the tests.
   8. We should be able to choose whether or not to "echo" the STDERR of a child process to the terminal (inherit, I think?). This should not interfere with capturing of STDERR, which should always happen.
   9. The DEBUG env var should not reach a child process.
   10. Like the current situation, we should not invoke child processes in parallel mode unless it was explicitly requested
2. Maybe a sugar method or two for common configurations of the above function (e.g., detailed: true).
3. Another function for the case in which we need to communicate with the child process:
   1. If a callback was supplied, return the child process
   2. If a callback was not supplied, return a tuple a la invokeMochaAsync
   3. Otherwise should work identically to the main function in 1.
   4. Hopefully, this wouldn't get used very often--especially since the tuple-returning one will still be awkward to use (but, I concede, there's really no good way around it)
   5. Alternatively, we look for commonalities in the tests that talk to the child process. Are we just killing the process after n ms? If so, maybe we can just get away with a "timeout" option which would do this for us. What about sending signals?
4. Finally, the custom assertions (test/assertions.js) would need to be updated to support this new result object. Ideally, this will simplify them quite a bit, because we will only have a single object (a "type" in unexpected parlance) to match; currently we have three (3), corresponding to the return values of runMocha/runMochaJSON/invokeMocha, respectively

[JoshuaKGoldberg]

cc @Uzlopak - is this relevant to your work on canary-in-the-gold-mine (CIGTM) testing?

[JoshuaKGoldberg]

Closing out as it's been quite a few months without input. Presumably if this is still true, anybody working on the repo's e2e tests will be able to find more specific info & re-file.