Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Revert grep to original functionality with strings converted to regex's.... #632

Closed
wants to merge 6 commits into from

6 participants

@nathanalderson

... Add Mocha#find() which escapes the string.

We have a bunch of grep-strings which we pass via URL using a script. With commit b00b6f5, the Mocha#grep() behavior changed so that the grep strings, which were previously treated as regular expressions, are now escaped. This broke our grep-patterns.

To me, the word "grep" implies regular expressions, and it is occasionally useful to use regular expressions when selecting tests/suites. I've reverted Mocha#grep() to the original functionality and implemented the new functionality in Mocha#find(), adding support for a new URL variable "find" which uses the new function.

You may not want this behavior in the main tree, but I thought I'd pass it along. Feel free to reject.

@tj
Owner
tj commented

yeah this is reasonable, I like it. I think we should change mocha's bin to use find as well, or at least add that flag, because often you may have something like describe('foo(bar)', where you dont really want to escape all the parens

@nathanalderson

Added '--find' option to the bin. Also added unit tests.

lib/mocha.js
@@ -174,6 +175,21 @@ Mocha.prototype._growl = function(runner, reporter) {
Mocha.prototype.grep = function(re){
this.options.grep = 'string' == typeof re
+ ? new RegExp(re)
+ : re;
+ return this;
+};
+
+/**
+ * Add regexp to grep, if `re` is a string it is escaped.
+ *
+ * @param {RegExp|String} re
@tj Owner
tj added a note

this might as well be string-only since .grep() is for regexps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tj
Owner
tj commented

i still like this direction, but I dont think "find" is the right name. We're not really finding anything, maybe just a simple --match but that sounds quite a bit like regexps

@wilmoore

Good opportunity to look to find(1) for it's semantics. You can pass it -F to ensure the given string is matched as fixed strings while -P considers the string to be a PCRE.

I propose the following:

  • --grep-string
  • --grep-pattern

Or something with a similar tone.

@tj
Owner
tj commented

yeah I think --grep string is fine, that's the more common use-case, and then --grep-regexp or --grep-pattern to behave like grep's --regexp, sounds good to me

@wilmoore

I like it:

  • --grep string
  • --grep-regexp pattern
@nathanalderson

Wilmoore's first post describes the the syntax of find, not grep. grep by default assumes a regexp pattern, and you must pass --fixed-string or -F to use strings. The "re" in "grep" stand for "regular expression" (http://en.wikipedia.org/wiki/Grep), so "--grep-regexp" just seems redundant. Personally, I think it should either be:

  • --find string
  • --find-regexp pattern

or

  • --grep pattern
  • --grep-string string

But I'll do whatever.

@wilmoore

True, true...+1.

@tj
Owner
tj commented

just the word "find" seems wrong, it's not finding anything. hmm..

@tj
Owner
tj commented

how about --only string and --only-regexp re

@nathanalderson

I like "only". It matches it.only(...).

@wilmoore

Seems to me that --only and --only-regexp have the right semantic.

+1

@tj
Owner
tj commented

@nathanalderson haha true good point, that too, ok let's go with that

@nathanalderson

Cool. I'll update the PR in the next few days.

@tj tj commented on the diff
support/tail.js
@@ -100,7 +100,8 @@ process.on = function(e, fn){
mocha.globals('location');
var query = Mocha.utils.parseQuery(window.location.search || '');
- if (query.grep) mocha.grep(query.grep);
+ if (query.only) mocha.only(query.only);
+ if (query['only-regexp']) mocha.only(new RegExp(query['only-regexp']));
@tj Owner
tj added a note

i'd remove this, I wouldn't expect anyone to quote an option, we can just pass it through { only: 'string' } or { only: /stuff/ }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jbnicolai
Collaborator

Not sure why this PR has been ignored for this long. If you would rebase against the current master I would be more than happy to have a look at it again, as it still seems useful.

Until then I'll close it as it has become un-mergable due to the accrued conflicts.

@jbnicolai jbnicolai closed this
@boneskull
Owner

@jbnicolai If we don't get a response soonish I'll go ahead and try the rebase myself.

@boneskull
Owner

@jbnicolai Also can't @nathanalderson simply rebase/squash/force-push etc? Why close the PR?

@jbnicolai
Collaborator

@boneskull I was making the assumption that @nathanalderson was probably no longer interested in this, after it having been ignored for 2 years. Rebasing is probably not as simple as it sounds, as a lot of conflicting changes have accrued over the years, and in effect would mean having to re-implement the feature against the current master.

I closed it because no more action on our side was required.

However, if you're interested in doing this: go for it! I gave it a quick try yesterday but couldn't work it out without digging in deeper.

Suggestion for the naming:

  • use grep for regular expressions, as the name implies
  • use fgrep for searching exact string matches

These are the unix standards, and probably work exactly the same on your terminal.

@jbnicolai jbnicolai reopened this
@nathanalderson

Yeah I ended up moving forward with my fork and then moved on to other stuff. Haven't kept up with this project. If I get some time, I may try to rebase or just reimplement, but I can't promise I'll get a chance. If someone else wants to take it on, that would be great. As I recall, it wasn't a difficult change. Definitely still a useful feature, IMO.

@boneskull boneskull self-assigned this
@danielstjules
Collaborator

@travisjeffery Thanks for merging #1579 ! This can probably be closed too now

@nathanalderson

Wow, thanks for picking this up! Does #1579 support URL variables for grep and fgrep? We were using mocha from the browser, so we needed URL support.

@danielstjules
Collaborator

@nathanalderson No problem! And the PR that was merged is almost exactly your changes applied to master, in addition to @jbnicolai's recommended naming, so behavior should be the same as what's here.

For example, if I understood correctly:

$ cat example.js
it('http://www.example.com/?foo=bar', function() {});

it('http://www.example.com/?foo=false', function() {});

$ ./bin/mocha --fgrep '?foo=bar' example.js

  ․

  1 passing (5ms)
@nathanalderson
@danielstjules
Collaborator

Sure does!

<html>
<head>
  <meta charset="utf-8">
  <title>Mocha Tests</title>
  <link rel="stylesheet" href="mocha.css" />
</head>
<body>
  <div id="mocha"></div>
  <script src="mocha.js"></script>
  <script>mocha.setup('bdd')</script>
  <script>
    it('grep()', function() {});
    it('greps', function() {});
  </script>
  <script>
    mocha.checkLeaks();
    mocha.run();
  </script>
</body>
</html>
example.html?fgrep=grep()
=> grep()

example.html?fgrep=grep
=> grep(), greps

example.html?grep=grep()
=> grep(), greps

example.html?grep=grep\(\)
=> grep()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
10 Makefile
@@ -30,7 +30,7 @@ lib-cov:
test: test-unit
-test-all: test-bdd test-tdd test-qunit test-exports test-unit test-grep test-jsapi test-compilers
+test-all: test-bdd test-tdd test-qunit test-exports test-unit test-only test-jsapi test-compilers
test-jsapi:
@node test/jsapi
@@ -72,16 +72,16 @@ test-exports:
--ui exports \
test/acceptance/interfaces/exports
-test-grep:
+test-only:
@./bin/mocha \
--reporter $(REPORTER) \
- --grep fast \
+ --only fast \
test/acceptance/misc/grep
test-invert:
@./bin/mocha \
--reporter $(REPORTER) \
- --grep slow \
+ --only slow \
--invert \
test/acceptance/misc/grep
@@ -123,4 +123,4 @@ tm:
mkdir -p $(TM_DEST)
cp -fr editors/$(TM_BUNDLE) $(TM_DEST)
-.PHONY: test-cov test-jsapi test-compilers watch test test-all test-bdd test-tdd test-qunit test-exports test-unit non-tty test-grep tm clean
+.PHONY: test-cov test-jsapi test-compilers watch test test-all test-bdd test-tdd test-qunit test-exports test-unit non-tty test-only tm clean
View
13 bin/_mocha
@@ -60,8 +60,8 @@ program
.option('-r, --require <name>', 'require the given module')
.option('-R, --reporter <name>', 'specify the reporter to use', 'dot')
.option('-u, --ui <name>', 'specify user-interface (bdd|tdd|exports)', 'bdd')
- .option('-g, --grep <pattern>', 'only run tests matching <pattern>')
- .option('-i, --invert', 'inverts --grep matches')
+ .option('-o, --only <string>', 'only run tests containing <string>')
+ .option('-i, --invert', 'inverts --only and --only-regexp matches')
.option('-t, --timeout <ms>', 'set test-case timeout in milliseconds [2000]')
.option('-s, --slow <ms>', '"slow" test threshold in milliseconds [75]')
.option('-w, --watch', 'watch files for changes')
@@ -78,6 +78,7 @@ program
.option('--interfaces', 'display available interfaces')
.option('--reporters', 'display available reporters')
.option('--compilers <ext>:<module>,...', 'use the given module(s) to compile files', list, [])
+ .option('--only-regexp <pattern>', 'only run tests matching <pattern>')
program.name = 'mocha';
@@ -217,9 +218,13 @@ if (program.timeout) mocha.suite.timeout(program.timeout);
mocha.suite.bail(program.bail);
-// --grep
+// --only
-if (program.grep) mocha.grep(new RegExp(program.grep));
+if (program.only) mocha.only(program.only);
+
+// --only-regexp
+
+if (program.onlyRegexp) mocha.only(new RegExp(program.onlyRegexp));
// --invert
View
15 lib/mocha.js
@@ -64,7 +64,8 @@ function Mocha(options) {
options = options || {};
this.files = [];
this.options = options;
- this.grep(options.grep);
+ if (options.only) this.only(options.only);
+ if (options['only-regexp']) this.only(new RegExp(options['only-regexp']));
this.suite = new exports.Suite('', new exports.Context);
this.ui(options.ui);
this.reporter(options.reporter);
@@ -165,17 +166,17 @@ Mocha.prototype._growl = function(runner, reporter) {
};
/**
- * Add regexp to grep, if `re` is a string it is escaped.
+ * Add a pattern to grep, if `pattern` is a string, it is escaped.
*
- * @param {RegExp|String} re
+ * @param {RegExp|String} pattern
* @return {Mocha}
* @api public
*/
-Mocha.prototype.grep = function(re){
- this.options.grep = 'string' == typeof re
- ? new RegExp(utils.escapeRegexp(re))
- : re;
+Mocha.prototype.only = function(pattern){
+ this.options.grep = 'string' == typeof pattern
+ ? new RegExp(utils.escapeRegexp(pattern))
+ : pattern;
return this;
};
View
3  support/tail.js
@@ -100,7 +100,8 @@ process.on = function(e, fn){
mocha.globals('location');
var query = Mocha.utils.parseQuery(window.location.search || '');
- if (query.grep) mocha.grep(query.grep);
+ if (query.only) mocha.only(query.only);
+ if (query['only-regexp']) mocha.only(new RegExp(query['only-regexp']));
@tj Owner
tj added a note

i'd remove this, I wouldn't expect anyone to quote an option, we can just pass it through { only: 'string' } or { only: /stuff/ }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
if (query.invert) mocha.invert();
return Mocha.prototype.run.call(mocha, function(){
View
38 test/grep.js
@@ -2,34 +2,46 @@
var Mocha = require('../');
describe('Mocha', function(){
- describe('"grep" option', function(){
+ describe('"only-regexp" option', function(){
it('should add a RegExp to the mocha.options object', function(){
- var mocha = new Mocha({ grep: /foo/ });
- mocha.options.grep.toString().should.equal('/foo/');
+ var mocha = new Mocha({ 'only-regexp': /foo()/ });
+ mocha.options.grep.toString().should.equal('/foo()/');
})
- it('should convert grep string to a RegExp', function(){
- var mocha = new Mocha({ grep: 'foo' });
- mocha.options.grep.toString().should.equal('/foo/');
+ it('should convert string to a RegExp', function(){
+ var mocha = new Mocha({ 'only-regexp': 'foo()' });
+ mocha.options.grep.toString().should.equal('/foo()/');
})
})
- describe('.grep()', function(){
+ describe('.only()', function(){
it('should add a RegExp to the mocha.options object', function(){
var mocha = new Mocha;
- mocha.grep(/foo/);
- mocha.options.grep.toString().should.equal('/foo/');
+ mocha.only(/foo()/);
+ mocha.options.grep.toString().should.equal('/foo()/');
})
- it('should convert grep string to a RegExp', function(){
+ it('should convert string to an escaped RegExp', function(){
var mocha = new Mocha;
- mocha.grep('foo');
- mocha.options.grep.toString().should.equal('/foo/');
+ mocha.only('foo()');
+ mocha.options.grep.toString().should.equal('/foo\\(\\)/');
})
it('should return it\'s parent Mocha object for chainability', function(){
var mocha = new Mocha;
- mocha.grep().should.equal(mocha);
+ mocha.only().should.equal(mocha);
+ })
+ })
+
+ describe('"only" option', function(){
+ it('should add a RegExp to the mocha.options object', function(){
+ var mocha = new Mocha({ 'only': /foo()/ });
+ mocha.options.grep.toString().should.equal('/foo()/');
+ })
+
+ it('should convert string to an escaped RegExp', function(){
+ var mocha = new Mocha({ 'only': 'foo()' });
+ mocha.options.grep.toString().should.equal('/foo\\(\\)/');
})
})
Something went wrong with that request. Please try again.