Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only run tests with title matching expression #476

Closed
matthewbauer opened this issue Jan 27, 2016 · 56 comments
Closed

Only run tests with title matching expression #476

matthewbauer opened this issue Jan 27, 2016 · 56 comments
Labels

Comments

@matthewbauer
Copy link

This is something that I would do with mocha. Here's the mocha help:

    -g, --grep <pattern>                    only run tests matching <pattern>

I'm hoping to get something like that working.

matthewbauer added a commit to matthewbauer/ava that referenced this issue Jan 27, 2016
This aims to resolve issue avajs#476. --grep can be passed to
ava which means to skip every test that doesn't match the
supplied pattern search for the test title. The pattern
can contain regexp strings and the skipped flag will be
set based on the output of "test" (so anywhere in the
title of the test will work).
matthewbauer added a commit to matthewbauer/ava that referenced this issue Jan 27, 2016
This aims to resolve issue avajs#476. --grep can be passed to
ava which means to skip every test that doesn't match the
supplied pattern search for the test title. The pattern
can contain regexp strings and the skipped flag will be
set based on the output of "test" (so anywhere in the
title of the test will work).
@sindresorhus
Copy link
Member

I agree we should have something like this, but I'd like to discuss how we can innovate on this instead of just copying the Mocha option. I also have a feeling if we add this, people will follow up and demand --fgrep (only run tests containing ) and then --invert (inverts --grep and --fgrep matches). I don't want more than one flag for this.

Maybe we could use globs instead?

$ ava --match='fo*' --match='!bar'

Regex might work well too. Just putting it out there.

@matthewbauer
Copy link
Author

I mean to be honest for my use case all that's needed is just to search for a string literal in the titles. I guess I just wanted to make sure it's versatile for different use cases, although globs would be more intuitive (and match the globs in the file args).

@wilmoore
Copy link

I personally think globs make more sense. Basic string literal searches only get us so far; however, constructing a valid RegExp in the shell feels awkward.

@sindresorhus
Copy link
Member

Can you think of any use-cases where a glob might not work?

@sindresorhus
Copy link
Member

And to be clear, !bar is a negated match, so we wouldn't need another flag for that.

@wilmoore
Copy link

Can you think of any use-cases where a glob might not work?

Off the top of my head, no; however, I can think of many times where mocha's grep was too limited to the point where I don't even use it so pretty much anything (especially globs) would be a vast improvement.

@wilmoore
Copy link

The only other thing I'd suggest is to change --match to --glob to be more explicit about it:

$ ava --glob='fo*' --glob='!bar'

@sindresorhus
Copy link
Member

I can think of many times where mocha's grep was too limited to the point where I don't even use it so pretty much anything (especially globs) would be a vast improvement.

Would be awesome if you could provide some examples. That would help make a decision.

matthewbauer added a commit to matthewbauer/ava that referenced this issue Jan 27, 2016
This aims to resolve issue avajs#476. --match can be passed to ava which
means to only run tests with a title matching the provided match
string. The skipped metadata flag will be added to each test not
matching the match string.
matthewbauer added a commit to matthewbauer/ava that referenced this issue Jan 27, 2016
This aims to resolve issue avajs#476. --grep can be passed to
ava which means to skip every test that doesn't match the
supplied pattern search for the test title. The pattern
can contain regexp strings and the skipped flag will be
set based on the output of "test" (so anywhere in the
title of the test will work).
@matthewbauer
Copy link
Author

For now, my PR is just going to do --match just because RegEx/glob matching is fairly expensive when done on large test suites. Since titles are pretty easy to change I think that should cover most if not all use cases.

@sindresorhus
Copy link
Member

@matthewbauer There's no "for now". Anything we add we'll have to support for a long time, and as I originally said, not interested in more than one flag for matching tests. I think we should figure out how to do it correctly. Not just a quick fix :)

because RegEx/glob matching is fairly expensive when done on large test suites

No. The regex and globbing are still just matching against strings. You'd have to have hundreds of thousands of tests for it to matter at all.

@wilmoore
Copy link

Would be awesome if you could provide some examples. That would help make a decision.

Sorry, I don't have any off the top of my head. I only mocha pretty infrequently and when I do I tend to use .only instead of --grep.

@matthewbauer
Copy link
Author

So here are the use cases for this flag as I see them:

  • Run a single test from the CLI.
  • To run only parts of a single large test file that for whatever reason can't be split up (tests are generated based on a JSON file for instance).
  • To skip tests when incompatible with certain environments (for instance skip tests that sudo in a container environment).

I'd be interested to see if there are any other possible use cases.

Additionally, I'd like to make it versatile enough so that it's not necessarily dependent on the CLI flag so it could work in a web context.

@matthewbauer
Copy link
Author

The main reason I'm hesitant with globs is that at least for me they only make since when you're dealing with file paths. I've certainly seen them used elsewhere but they can lead to some issues if you aren't aware of the file path association. For instance:

test("test group 1: 4 - 2 = 2", function(t){})
test("test group 1: 2 + 2 = 4", function(t){})
test("test group 1: 4 / 2 = 2", function(t){})

You might expect for --glob 'test group 1: *' to run all of them but the '/' would throw it off. I suppose --glob 'test group 1: **' would resolve it though.

@wilmoore
Copy link

You might expect for "--match 'test group 1: '" to run all of them but the '/' would throw it off. I suppose "--match 'test group 1: *'" would resolve it though.

Yup, agreed. That is the main PITA with globs for sure. Only other flexible option is RegExp which also is no panacea; however, would make it a little easier to get around issues like the one you've presented.

That being said, I concede that I have less skin in the game since I've gotten by without this feature entirely using .only or .skip and that's been fine.

@jamestalmage
Copy link
Contributor

Any reason we couldn't just convert * to ** before feeding it to minimatch/whatever?

@wilmoore
Copy link

I'm seeing what @matthewbauer is seeing unfortunately:

var multimatch = require('multimatch')

var matches = multimatch([
  'test group 1: 4 - 2 = 2',
  'test group 1: 2 + 2 = 4',
  'test group 1: 4 / 2 = 2'
], ['test group 1:**'])
// => [ 'test group 1: 4 - 2 = 2', 'test group 1: 2 + 2 = 4'  ]

@jamestalmage
Copy link
Contributor

Scratch that. I think we should just accept that having a / in your test name is going to create issues. I think that's pretty rare for real applications.

Maybe we should combine the test name with the file path:

// tests/foo.js
test('bar', t => {...})
// grep matches against `tests/foo/bar`

If we ever do nested groups, we could combine them with slashes:

// tests/foo.js

group('bar', test => {
  test('baz', t => {...});
  // tests/foo/bar/baz
});

@matthewbauer
Copy link
Author

Maybe we should combine the test name with the file path

I wasn't really considering multiple test files at this point. If you only wanted it to match tests/foo/bar.js couldn't you just modify your path glob? I suppose this would enable some even finer control over what's being run though: for instance, run all tests in tests/foo/bar/** and any other tests with the words "foo" or "bar" in them. I don't know if anyone would ever use that but kind of a neat feature.

@jamestalmage
Copy link
Contributor

Yeah - that's what I was thinking.

Back to the core question though: Should we do this?

The only time I've used this feature in mocha is by tagging a few tests as "slow" and avoiding them while in watch mode and doing TDD (preferring fast feedback over completeness).

@matthewbauer
Copy link
Author

I'm thinking there should be some way to selectively run tests on based from the CLI. For me, travis times out if I run the whole test file.

An alternative would be to let test scripts read command line flags and then skip or run a test based on its own rules. This might be a more KISS approach. A helper function could be defined like:

import test from 'ava'
function _test(title, fn) {
  if (title.indexOf(test.flags.match) !== -1) {
    test(title, fn);
  } else {
    test.skip(title, fn);
  }
}

@jamestalmage
Copy link
Contributor

For me, travis times out if I run the whole test file.

Whoa! That's a long test run. Why is it timing out? Might this fix it? Is there some way AVA could fix your timeout issues by just being faster? (Any code you could share that demonstrates a bottleneck in AVA would be hugely helpful).


As a hotfix, you could use environment variables:

$ TEST_ONLY=foo,bar ava
import test from 'ava';

const selectors = (process.env.TEST_ONLY || '').split(',');

function testIf(selector, title, testFn) {  
  if (selectors.indexof(selector) !== -1) {
    test(title, testFn);
  }
}

testIf('foo', 'it handles foo', t => {
  ...
});

@sindresorhus
Copy link
Member

I don't think we should combine test paths and titles.

Been thinking about it and maybe even using globs is overdoing it a bit. Globs have a lot of archaic rules and is actually really complex and really meant for paths. Maybe we could do our own simple replacers? * will match anything and ! at the start negates the match. Do we really anything more? The common use-case for non-exact matching is cases like foo - 1 foo - 2 foo - 3, and you could do foo - *.

@jamestalmage
Copy link
Contributor

At that point we are just replacing * with .*, so why not just let them specify regexps as strings and be done with it: foo - .* (with first character ! still meaning negation)

@sindresorhus
Copy link
Member

@jamestalmage My thinking is that some characters require escaping in a regex, so this would be simpler. And foo - * would really be ^foo - .*$.

@jamestalmage
Copy link
Contributor

With mocha I used to do:

describe('foo ', function () {
  it('bar', function () {
    ...
  });

  it('baz - @slow', function () {
    ...
  });
});

and then ran:

mocha --watch --invert --fgrep @slow

I don't see how to do that with what you are suggesting.


What about one of these options:

  1. Implement both --fgrep and --grep, with ! as a negation flag.

    --fgrep foo
    --fgrep !foo
    --grep ^foo.*
    --grep !^foo.*
    
  2. Do just --match, which defaults to behaving like --fgrep unless both the first and last characters are both /, at which point we make it a RegExp (and they are responsible for all their own escaping).

    --match foo
    --match !foo
    --match /^foo.*/
    --match !/^foo.*/
    

@SamVerschueren
Copy link
Contributor

@sindresorhus Oops, missed that one.

Would like to see support for the * as well. I think it offers a lot more flexibility without making it difficult for the user (like RegExp support for instance).

@sindresorhus
Copy link
Member

Regardless of the outcome here, I made a simple module for wildcard matching as I can see it being useful in other cases too. Could use some feedback on it: sindresorhus/matcher#1

@SamVerschueren
Copy link
Contributor

@sindresorhus Thanks! Need it in another module of mine as well :).

@vadimdemedes
Copy link
Contributor

@sindresorhus Looks good & simple, exactly what we need!

@sindresorhus
Copy link
Member

Alright. Seems we have an agreement :)

@matthewbauer Can you update your pull request to use matcher?

@sindresorhus
Copy link
Member

#477 went stale, so this is now PR welcome again.

@kasperlewau
Copy link
Contributor

I'd be happy to get cracking on this, unless of course someone beats me to it. Got 8 hours of day job to take care of now so there's a wide open window of opportunity to do so!

@develar
Copy link
Contributor

develar commented Mar 2, 2016

@kasperisager To save your time — grab https://gist.github.com/develar/da7d3c01c3b8e6e0eace — it is a patch to apply to master (original PR is outdated). You only need to use proposed matcher here.

It works for me, but I am very busy now to prepare PR (well, can do in two days if you will not do first :))

@kasperlewau
Copy link
Contributor

@develar Cheers! I managed to sneak in a couple o' minutes and I'm just about to push some changes and wire up a PR. Thanks.


@sindresorhus - Before I push my feature branch, I'd just like to verify whether the the aim is to support * or not. Using matcher.isMatch and it inherently allows * as far as I can tell.

I got a wee bit lost reading through the discussion.

@sindresorhus
Copy link
Member

@matthewbauer Yes * and ! should be possible. And it should be possible to use it multiple times, e.g. --match='*oo' --match='!foo', so it would be better to use matcher() against the test collection as matches() handles the filtering correctly for you.

@kasperlewau
Copy link
Contributor

@sindresorhus Cool!

I'm guessing we want to support multiple patterns on a package.json#ava level too? If so, I'd suggest we enforce that value to always be an array so as to reduce the amount of type checks we'd need to do in the runner, and also to keep single/multiple matcher(s) the same on a config level.

# yay
"ava": { 
  "match": ["*oo", "!foo"] 
}

# nay
"ava": { 
  "match": "*oo" 
}

@sindresorhus
Copy link
Member

👍 I usually prefer that too as it also has clearer intent.

@kasperlewau
Copy link
Contributor

Good stuff. I'll get back to this asap. Got some other business I need to take care of for now.

The one thing that bothers me just ever so slightly is how to get the test/runner specs working with an array, whereas on the CLI level we're expecting a string value - potentially multiple times.

That is to say;

// works
var runner = new Runner({
  match: ['*oo', '!foo']
});

// obviously this does not
var runner = new Runner({
  match: '*oo',
  match: '!foo'
});

The first example is what I would expect to utilise inside lib/runner along with matcher, whereas the second example is what I would expect to be passed through the CLI.

Or is there some magic on the CLI level, converting multiple occurrences of the same string option to an array? If so, I'm quite confident I'll have a PR posted tonight.

@SamVerschueren
Copy link
Contributor

@kasperlewau yes

const cli = meow();

console.log(cli.flags);
$ ./cli --foo=1 --foo=2
{ foo: [ 1, 2 ] }

@kasperlewau
Copy link
Contributor

@SamVerschueren Awesome! Not such a long ways to go then : )

@kasperlewau
Copy link
Contributor

Alright, on my lunch break. Thinking of the following;

How do we enforce the match option in package.json to be an Array? Seeing as how meow will only cast to an array if multiple values are supplied through the CLI, we still need a type check inside lib/runner to function with matcher.

How do we differentiate between a passed-in CLI flag and a configured value in package.json?
And how does one test the behaviour of options defined in package.json? I couldn't find a test suite that does that, not yet anyways.

I'm a little unsure as to what the relationship between the runner and cli looks like, if there even is one.

@novemberborn
Copy link
Member

How do we enforce the match option in package.json to be an Array?

This is a bit of a distraction. Just follow the precedent by the require and source options. They're singular CLI flags that can be repeated, and singular package.json options that are arrified in cli.js.

How do we differentiate between a passed-in CLI flag and a configured value in package.json?

The flag parser is passed the default config from package.json. You don't need to differentiate.

And how does one test the behaviour of options defined in package.json? I couldn't find a test suite that does that, not yet anyways.

There's a pkg-conf: cli takes precedence test in test/cli.js. I just noticed that --watch and --source aren't included in that test either though.

I'm a little unsure as to what the relationship between the runner and cli looks like, if there even is one.

cli.js creates an Api instance from api.js, passing options. That Api finds tests and creates child processes. You probably want to pass the match option down that way. See #477 for inspiration.

@kasperlewau
Copy link
Contributor

Thank you so much for that @novemberborn!

This is a bit of a distraction. Just follow the precedent by the require and source options. They're singular CLI flags that can be repeated, and singular package.json options that are arrified in cli.js.

Gotcha. I was casting to an array using util.isArray inside lib/runner, but I'll shift that back to cli.

There's a pkg-conf: cli takes precedence test in test/cli.js. I just noticed that --watch and --source aren't included in that test either though.

I should really get a new set of glasses (5 years since, I do believe), this makes it a lot clearer to me. As it's not part of the subject of the PR/issue I'd prefer not to add --watch and --source expectations.

@novemberborn
Copy link
Member

As it's not part of the subject of the PR/issue I'd prefer not to add --watch and --source expectations.

Yea that's fair. Just had a look and those are actually not testable in the same way.

@jamestalmage
Copy link
Contributor

There's a pkg-conf: cli takes precedence test in test/cli.js. I just noticed that --watch and --source aren't included in that test either though.

@novemberborn - do we need to add an issue to properly test that?

@novemberborn
Copy link
Member

@jamestalmage maybe. It's hard though without stubbing some dependencies in cli.js. Otherwise we'd need some rather complicated fixture setup. What do you think?

@jamestalmage
Copy link
Contributor

some rather complicated fixture setup

I would go with that option. The fixtures needn't be that complicated:

 // fixtures/match-pkg-conf/foo-passes.js
test('foo', t => t.pass());
test('bar', t => t.fail());
 // fixtures/match-pkg-conf/bar-passes.js
test('foo', t => t.fail());
test('bar', t => t.pass());
// fixtures/match-pkg-conf/package.json
{
  "ava": {
    "match": "foo"
  }
}

Then just call the CLI with cwd === fixtures/match-pkg-conf and run each test individually (expecting zero / non-zero exit codes as appropriate). Then run both again with --match=bar and expect the opposite. That's how I'd do it.

@jamestalmage
Copy link
Contributor

Usually our CLI tests aren't there to fully exercise the system (that's what our API unit tests are for), they just need to verify all the glue code works.

@novemberborn
Copy link
Member

@jamestalmage yes @kasperlewau added tests for --match like you described. I was talking about --source. Opened #626 to track.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants