mantoni / mocaccino.js

Mocha test runner as a Browserify plugin
MIT License
22 stars 12 forks source link

--grep option does not support regex #15

Closed islemaster closed 8 years ago

islemaster commented 8 years ago

I've wondered for a while why the grep option of our test runner wouldn't process a regular expression. Today I think I've tracked the issue down to Mocaccino.

Mocha's --grep option supports full JavaScript regular expressions. When taking an argument from the command line, Mocha's default behavior is to pass it to a RegExp constructor, and later use that RegExp to select tests:

  if (options.grep) {
    this.grep(new RegExp(options.grep));
  }

But for some reason, Mocaccino's --grep option bypasses this behavior and instead calls Mocha.grep with the argument wrapped as a string (here in setup-browser.js, and here in setup-node.js), so really it's acting like Mocha's --fgrep option:

  if (options.fgrep) {
    this.grep(options.fgrep);
  }

This behavior is surprising! Is this by design? It doesn't appear to be: Mocaccino's usage message documents the grep option as:

  --grep          Mocha grep option

Mochify, because it's using Mocaccino, has the same undocumented limitation.

To be fair, the Mocha API here is fairly confusing - it's odd that a grep option passed in the options object to the constructor is handled differently than the argument to the grep() function - but since grep() assumes you are able to give it a RegExp and Mocaccino is coercing our grep argument to a string, here's my proposed fix:

In setup-browser.js change:

var mocha = new Mocha();
mocha.grep('{{GREP}}');

to

var mocha = new Mocha({
  grep: '{{GREP}}'
});

And make a corresponding change in setup-node.js. This will take advantage of Mocha's more flexible handling of the grep option in its constructor. It might also be good to expose Mocha's --fgrep option for users who want the old behavior.

I understand that this would be a breaking change for any systems that depend on the existing behavior of Mocaccino's grep option and are using, for example, unescaped parentheses in that argument. If the breaking change is problematic, maybe you can at least document the limitation, or introduce an egrep option that correctly maps to Mocha's grep option?

Thanks!

mantoni commented 8 years ago

Nice catch. This is not by design and I would treat it as a bug. The idea is to forward Mochify options to Mocha transparently, so the behavior is defined by Mocha. Can you turn your findings into a pull request, ideally along with a test?

islemaster commented 8 years ago

Will do.

islemaster commented 8 years ago

Thanks for the quick turnaround!