mantoni / mochify.js

☕️ TDD with Browserify, Mocha, Headless Chrome and WebDriver
MIT License
346 stars 57 forks source link

Forcefully exit 1 in cli when calling mochify throws #258

Closed m90 closed 2 years ago

m90 commented 2 years ago

Warning: this one is ugly and more of a workaround than a fix. It also does not address the issue for consumers of the API instead of the CLI.


I noticed that when you're bad at typing (I heard it happens) and misspell the name of the bundle command, Mochify will hang forever instead of exiting:

mochify src/{,**}/*.test.js --config package.json --driver-option.executablePath $(which google-chrome)

and

  "mochify": {
    "driver": "puppeteer",
    "bundle": "browseryfi",
    "driver_options": {
      "args": ["--no-sandbox", "--allow-chrome-as-root"]
    }
  },

will print:

Error: Command failed with ENOENT: browseryfi thing1.test.js thing2.test.js ...
spawn browseryfi ENOENT
    at Process.ChildProcess._handle.onexit (internal/child_process.js:268:19)
    at onErrorNT (internal/child_process.js:468:16)
    at processTicksAndRejections (internal/process/task_queues.js:84:21)

and then just hang until you Ctrl+C which interestingly just kills the bundler process so that npm will still run posttest hooks and the like.


What seems to happen is the following: when calling mochify from the cli throws an error and we end up here: https://github.com/mantoni/mochify.js/blob/4f10f3a23bb952ca77a3ffaca669716f553beb73/cli/index.js#L49

the process spawned by execa is still alive, seemingly idling forever. Trouble is I really don't know how to make it exit correctly. What I tried is doing something like this:

  let result;
  try {
    result = await execa(cmd, args.concat(files), {
      preferLocal: true
    });
  } catch (err) {
    result.cancel(); // cannot call `cancel` on undefined
    throw err;
  }

I also looked at the source code of lint-staged: https://github.com/okonet/lint-staged/blob/101ad5ea21a5cab51a9a2a9d4701defe26e346ca/lib/resolveTaskFn.js#L103-L105 but it seems they do the same thing (potentially running into the same issue).

m90 commented 2 years ago

The more I dig into this, the less I understand. When changing resolve-bundle.js to something like this:

async function resolveBundle(command, files) {
  if (!command) {
    return concatFiles(files);
  }

  const [cmd, ...args] = parseArgsStringToArgv(command);

  const cp = execa(cmd, args.concat(files), {
    preferLocal: true
  });

  cp.on('error', (err) => {
    throw err;
  });

  const result = await cp;

  if (result.failed || result.killed) {
    throw new Error(result.shortMessage);
  }

  return result.stdout;
}

and you just throw the error on the child process into somewhere (I really don't know "where" this is being thrown) it works as expected and the CLI can exit without having to call process.exit. It seems there is nothing that can be done with cp however in the error callback, I tried killing, canceling and what not, but it all just resulted in the stalled process.

m90 commented 2 years ago

This however, will not work (i.e. we have a stalled process still):

async function resolveBundle(command, files) {
  if (!command) {
    return concatFiles(files);
  }

  const [cmd, ...args] = parseArgsStringToArgv(command);

  const result = await new Promise((resolve, reject) => {
    const cp = execa(cmd, args.concat(files), {
      preferLocal: true
    });

    cp.on('error', (err) => {
      reject(err);
    });

    cp.then(resolve, reject);
  })

  if (result.failed || result.killed) {
    throw new Error(result.shortMessage);
  }

  return result.stdout;
}
m90 commented 2 years ago

All of this execa stuff was a red herring, proper fix coming up.