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

Support for --tags to include/exclude tests based on some optional tagging #1445

Closed
wants to merge 1 commit into from

Conversation

rprieto
Copy link
Contributor

@rprieto rprieto commented Nov 24, 2014

⚠️ Before you read
the suggested tagging API has changed since the initial proposal, see new comments further down


As discussed in #928, this is a tentative PR for tagging support in the bdd interface. I hope this is not against the guidelines (PR for a new feature), but it will be easier to discuss what's feasible with some visibility on the code changes.

  • you can optionally pass an array of tags as the first argument to describe and it
  • by default, all tests always run
  • you can specify --tags to run a subset of the tests
  • tests that don't match the filter are marked as pending
describe('my service', function() {
  it('test 1', function() {});
  describe(['integration'], 'backend connectivity', function() {
    describe(['fast'], 'quick tests', function() {
      it('test 2', function() {});
    });
    it(['slow'], 'test 3', function() {});
  });
});
  • mocha runs everything
  • mocha --tags "not:integration" runs test 1
  • mocha --tags "is:integration" runs tests 2 and 3
  • mocha --tags "is:integration not:slow" runs test 2

You can also programmatically update the filter with

if (/* out of business hours */) {
  require('mocha').options.tags.add('not:backend');
}

If this looks good, happy to look into how to unit test & document it.
Cheers

Review on Reviewable

@boneskull
Copy link
Contributor

As you wrote, --grep works mightily well most of the time. And for one-offs, there's always only(). And of course you can use naming conventions for the titles...

So if this is going to get merged, and someone is going to maintain it, it has to be extremely concise. I like the idea of tagging my tests, and keeping that meta-info separate from the title.

Please see my notes in 953f018c6de6cdf3b3443eec143a8f58ca7701fa

@rprieto
Copy link
Contributor Author

rprieto commented Nov 24, 2014

Yes this is definitely aimed at meta-tagging for selective automated testing, more than grep and .only which to me are more for local debugging (e.g. "let me just run these 3 tests").

For example, use cases we've always wished we could handle are:

  • we have a CI environment where Redis is not available, so these integration tests should be skipped (--skip-tag "redis")
  • ideally, we only want to run "fast" integration tests as part of our pre-commit hook (--tag "unit" --tag "integration,fast")
  • we have a smoke-test suite that runs every hour, but must skip certain tests at night (the systems are down) (--skip-tag "integration,mainframe")

So far we've kind of managed to handle these by using --grep on the title, but it gets extremely messy...

@rprieto
Copy link
Contributor Author

rprieto commented Nov 24, 2014

Thanks a lot for all the feedback @boneskull. I simplified the PR to support the following:

  • --tags "one,two" only runs tests with either of the tags
  • --skip-tags "three,four" excludes tests with either of the tags
  • they can also be combined together (e.g. --tags unit --skip-tags slow)

I've also removed any third-party dependencies, and added tests.
I'll just need to add test coverage for the BDD interface itself.

@rprieto
Copy link
Contributor Author

rprieto commented Nov 25, 2014

Hi @boneskull, I have another proposal. I re-implemented it with a slightly different API, which I believe makes the implementation a lot cleaner (see Files changed now - I kept both commits but will squash them later).

To use tags:

describe('my thing', function() {
  this.tag('integration', 'slow');
  describe('some tests', function() {
    this.tag('ie8');
    it('...', function() {});
  });
  it('...', function() {});
});

To filter at runtime:

mocha --tags "browser,integration" --skip-tags "slow"
  • Pros
    • a lot more similar to other context operations (this.slow, this.timeout...)
    • can apply to all interfaces, since the tag() method is on Suite.prototype
    • the filtering logic now belongs in Runner, right next to the existing grep logic
  • Cons
    • we can only add tags to a suite, not an individual test. However this is similar to how you can't set a timeout value to a single test, you'd have to set that at the describe level.

I believe that's much more in line with the rest of Mocha, and easier to understand... thoughts?

@nguyenchr
Copy link

I prefer the new implementation

describe('my thing', function() {
  this.tag('integration', 'slow');
  describe('some tests', function() {
    this.tag('ie8');
    it('...', function() {});
  });
  it('...', function() {});
});

It's a bit more restrictive in that you can only add it to suites.

But that can be worked around by nesting tests in suites if the tagging needs to be different.

Which is the same workaround you need to do currently for timeouts

@boneskull
Copy link
Contributor

Yes, I like the new implementation.

@@ -36,6 +36,11 @@ exports.create = function(parent, title){
return suite;
};

Suite.prototype.tag = function(tags) {
var tags = Array.prototype.slice.apply(arguments);
this.ctx._tags = this.parent.ctx._tags.concat(tags);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two lines can be rewritten as

this.ctx._tags = Array.prototype.concat.apply(this.parent.ctx._tags, arguments);

@boneskull
Copy link
Contributor

I'd like to merge this when my comments are resolved unless @travisjeffery declines

@rprieto
Copy link
Contributor Author

rprieto commented Dec 3, 2014

Thanks! I made changes according to your comments, and will add add unit tests around Suite and Runner. Only other changes I made:

  • skip tests suites early if none of their children match the tag filter (similar to grepTotal)
  • update the this.total value in the Runner to also take tag filtering into account (instead of just grep total)

For now, there is a new TagFilter class to look after tag filtering. Maybe it would actually be nicer if it was renamed to Filter, and contained all filtering logic, like --grep, --invert, --tags and --skip-tags. This would mean all filtering done in one place, and no need to duplicate the whole total logic for example. So we would end up with:

function Runner(suite) {
  this._filter = new Filter();

  // depending on command line args we can set
  // this._filter.grep = /myregex/
  // this._filter.invertGrep = false
  // this._filter.tags = ['integration']
  // this._filter.skipTags = ['slow']

  this.total = this._filter.total(suite);
}

Runner.prototype.runSuite = function(...) {
  // check how many tests would be run given the current filter
  var total = this._filter.total(suite);
  if (!total) return fn();
}

Runner.prototype.runTests = function(...) {
   // if this test shouldn't run, skip it
   if (!this._filter.run(test)) return fn();
}

@rprieto
Copy link
Contributor Author

rprieto commented Dec 3, 2014

A nice side effect of making Filter a first class citizen is that something like issue #1060 becomes incredibly easy.

You can just set this._filter.only = some_suite / some_test and the Filter class can take that into account, no other change required in the Runner or anywhere else.

I just pushed a working / unit-tested version as a new commit.

@boneskull
Copy link
Contributor

@rprieto Can you try rebasing and squashing please?

@rprieto
Copy link
Contributor Author

rprieto commented Dec 15, 2014

Done!

@rprieto
Copy link
Contributor Author

rprieto commented Dec 17, 2014

Are you happy to merge it @boneskull? I'm not sure how the docs get updated though, since they're not part of this repo.

@boneskull
Copy link
Contributor

Docs are in the gh-pages branch.

@rprieto
Copy link
Contributor Author

rprieto commented Dec 17, 2014

Should I raise a separate PR for the docs?

@dasilvacontin
Copy link
Contributor

I think it's more consistent to add both the feature, the tests and the docs on the same PR/commit.

@rprieto
Copy link
Contributor Author

rprieto commented Dec 17, 2014

@dasilvacontin I agree. But how can I raise a PR that spans across 2 branches? (master + gh-pages)

@dasilvacontin
Copy link
Contributor

@rprieto, ouch, good point. No, a PR can't target two branches. Separate PR then.

@dasilvacontin
Copy link
Contributor

That's something we talked about early on at #1445, with a special syntax like api+integration, but it raised a few questions (can you do (api AND integration) OR app, what about (api AND integration) not (slow OR redis)...).

Hmm, I can't find it. Was it on diff comments? Maybe I'm just blind. 😄

I think @boneskull's comment was "make it as simple as possible", which I think is nice. So we ended up with --tags x,y,z to run tests with either tags, and --skip-tags x,y,z to skip tests with either tags.

I agree. Simpler tends to be easier to understand and to use, and it's less code to maintain.

It's true that it would be nice, since "run api integration tests" sounds like a common scenario (e.g. mocha --tags api+integration). I'm happy to revisit the PR if the Mocha team thinks it's useful, but otherwise some of these scenarios should be solvable with more custom tags (e.g. --tags api-integration,app-integration).

Indeed, it can be solved with more custom tags. Unless you had 3 or more tags and now you have to write all combinations, which is a pain.

We can just sit back and wait for people to request the feature, though. There's no much point on extending it if no one's going to use it.

@rprieto
Copy link
Contributor Author

rprieto commented Dec 18, 2014

You're right it must have been on a commit that got squashed. The only thing I'd keep in mind is that moving from x,y,z meaning x or y or z to x and y and z in the future would be big breaking change.

If we do decide to implement --tags x,y --tags z, I don't think it makes the simple cases more complex. It would just mean the current --tags app,api (either of) becomes --tags app --tags api.

I'm happy either way.

@munkyjunky
Copy link

+1, getting this in would be a huge benefit to my testing suite!

@hjalet
Copy link

hjalet commented Apr 7, 2016

+1

@jdmarshall
Copy link

Do we have a way in this configuration to run all the tests without a tag and all the tests with a particular tag?

I could run multiple passes of Mocha but that could potentially change the test behavior (it shouldn't, but that's very different from "it won't")

@rprieto
Copy link
Contributor Author

rprieto commented Apr 9, 2016

to run all the tests without a tag

If you mean tests without a particular tag, then yes you can do --tags this --skip-tags that. If you mean tests without any tags, then unfortunately the PR as it stands doesn't support it. There's no way to refer to untagged tests specifically for the moment.

@jdmarshall
Copy link

Besides things like 'slow', 'fast', 'phantomjs-broken', the next logical place to need tags is for feature toggles.

Feature toggles are not particularly interesting until you have at least 1 of them, and to have one toggle you need to mark your tests as unaffected by the toggle, only working when disabled, or only working when enabled.

To run a full tests suite you need to run all of the unaffected tests and the affected tests that apply to the current configuration. That would be group A and B, or A and C depending on which way it's configured at present.

Now add a second flag, and you see the --skip-tags flag doesn't scale very well without the catchall. In order to run all of the unaffected tests you have to update your --skip-tags arguments to include all feature toggles in the project.

If there were a special case for 'none' (empty string perhaps?) then you could run mocha twice to get any combination of tests to run. That may be good enough, but as hard as we try, tests do tend to behave differently when executed in a different order.

@hollomancer hollomancer added the status: needs review a maintainer should (re-)review this pull request label Aug 28, 2016
@MatthewRalston
Copy link

MatthewRalston commented Sep 20, 2016

I'd love to see this. FWIW, no one has addressed this in a while.

@jcrben
Copy link

jcrben commented Sep 20, 2016

@MatthewRalston please use the thumbs up emoji reaction rather than spamming people with a message like that

@rprieto
Copy link
Contributor Author

rprieto commented Sep 27, 2016

Hi @jcrben, any thoughts about this PR? Or more specifically, what do you think is missing so there can be a merge/close decision?

@jcrben
Copy link

jcrben commented Sep 28, 2016

@rprieto I'm not a team member. It looks like @dasilvacontin who is a member gave me a reaction, tho, so maybe we should ask him 😉

looks like he was waiting on word from @boneskull who never got around to it. @travisjeffery wanted to stick with grep but grep really doesn't handle this in a truly clean way

Copy link
Contributor

@dasilvacontin dasilvacontin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested this, but I have some comments about the code. Good job, ty!

var include = (!this.tags.length) || matchTags(test.ctx._tags, this.tags);
var exclude = this.skipTags.length && matchTags(test.ctx._tags, this.skipTags);
// final decision
return grepMatch && include && !exclude;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could early return once we calculate a condition that fails so that we skip unnecessary calculus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. I thought grepMatch && include && !exclude was simple and easy to understand, and chose that over performance. Do you think performance matters a lot here?

Copy link
Contributor

@dasilvacontin dasilvacontin Oct 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my testing, the performance gain is negligible, and this reads easier. LGTM, good call.

(like 12ms per 100 000 tests)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sharing so that someone points out my test is bad or wrong:

const filterTags = ['ci']
const testTags = ['slow', 'vendor', 'thing', 'ci']

function matchTags (actualTags, against) {
  return against.some((tag) => {
    return actualTags.indexOf(tag) !== -1
  })
}

console.time('shouldRun')

let tests = 100 * 1000
while (--tests) matchTags(testTags, filterTags)

console.timeEnd('shouldRun')

Runner.prototype.skipTags = function(tags){
debug('skip tags: %s', tags);
this._filter.skipTags = tags;
this.total = this._filter.count(this.suite);
Copy link
Contributor

@dasilvacontin dasilvacontin Sep 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing this line quite a few times – might be worth refactoring into Runner#updateTotal.

total is not very descriptive imo, but we can leave renaming that to an additional issue/PR and move on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point thanks, I'll change extract & for a more descriptive name.

}
}

return retLines;
Copy link
Contributor

@dasilvacontin dasilvacontin Sep 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't build mocha.js. That's done for releases – it's confusing and it clutters the PR / commit. No worries though!

@dasilvacontin dasilvacontin removed the status: needs review a maintainer should (re-)review this pull request label Sep 28, 2016
@dasilvacontin
Copy link
Contributor

Please rebase since we added a linter to CI recently. 👌

@dasilvacontin
Copy link
Contributor

Also, since we are now under the JS Foundation, you'll need to sign the JS Foundation CLA before this gets merged. Just pointing it out, in case it's missed between the other CI checks. :)

@aseemk
Copy link

aseemk commented Dec 2, 2016

Hey folks,

I'm just a regular Mocha user, and I don't feel strongly one way or the other about this feature.

I just wanted to understand: it seems all the mentions of --grep here are talking about fuzzy searching over titles, which I agree is imprecise. But what we do — which we learned straight from the Mocha wiki — is include tags (by convention) in the title, and search for exactly those tags.

E.g. examples from our real-world code:

// This test suite relies on hitting third-party services,
// so skip it if we're running offline.
describe('Foo bar #skipoffline', function () {

    // This individual test can be pretty slow,
    // because we have to wait for yada yada,
    // so skip it if we're running fast tests only.
    it('lorem ipsum #skipfast', ...);

});

Then we can do e.g. --grep '#skipoffline' --invert, and combine multiple tags just like normal regex, e.g. --grep '#skipoffline|#skipfast' --invert. We have ~5 such tags, including #skipci (modeled after Travis's [skip ci] commit message syntax).

Is the difference between --grep like this, and this PR, that the filtering is more powerful? E.g. I can run tests tagged "X and Y", or "X or Y", or "X but not Y"? I think --grep can only do the middle one.

Thanks!

@aseemk
Copy link

aseemk commented Dec 5, 2016

Funny story: I added a feature today where I found it much easier to use a conditional everywhere (with this.pending = true if the conditional was true) rather than any #skipfoo tag.

Specifically, I could have done --grep '#skipfoo' --invert, but this is a cross-cutting concern, and doing this in a generic way, while still allowing helpers/wrappers around Mocha for business concerns, would have been challenging and error-prone.

So +1 to this feature now. =) Thanks!

@noahsilas
Copy link

I wonder if there is an opportunity to do something more along the lines of RSpec's "User-defined metadata" tooling: https://www.relishapp.com/rspec/rspec-core/docs/metadata/user-defined-metadata. It seems like tags provide a fair amount of power, but have some limitations:

  • Tags seem to be additive only - that is, a nested context can add a tag, but can't remove one that was applied by its parent. While this may seem convoluted, I can imagine scenarios where (for instance) shared behaviors may want to remove a "slow" tag.

  • Allowing more structured metadata opens up new extension options. As an example, writing tests for an API requiring an API key to authenticate could make use of something like this:

    // common test suite setup
    beforeEach(function () {
      let auth = this.test.meta.authenticatedWith || {};
      if (auth.apiKey) {
        httpClient.defaults({ headers: {'Authorization': 'APIKEY ' + auth.apiKey }});
      }
    });
    
    // tests:
    describe("POST /widgets", function () {
      it("fails(401 unauthorized)", ...);
      context("logged in", { authenticatedWith: { apiKey: '123' }}, function () {
        it('succeeds', ...)
      });
    });
    

@boneskull
Copy link
Contributor

that could be useful, but rspec doesn't seem to have any filtering functionality w the metadata (though i only skimmed). "tags" are absolutely for filtering, and expanding that to user-defined metadata isn't too bad--except we'd need to get clever on the CLI end.

@boneskull boneskull closed this Dec 6, 2016
@boneskull boneskull reopened this Dec 6, 2016
@jsf-clabot
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@boneskull
Copy link
Contributor

wrong button. anyway this is on hold until we hammer down the roadmap. we will have a forum for discussion about what goes in the roadmap.

@alexjv89
Copy link

alexjv89 commented Jan 5, 2017

+1

@sn1ckers
Copy link

sn1ckers commented Mar 9, 2017

this would be a really nice feature to have, we are in need to run tests based on inverted form factor '@not_mobile', '@not_tablet' for test that don't support all form factors and also other tags, like slow/fast etc.

I have tried to use grep but can't find any good solution to achieve what I wan't to do above. So this feature will sort out that problem.

@stale stale bot added the stale this has been inactive for a while... label Jul 31, 2017
@stale
Copy link

stale bot commented Jul 31, 2017

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale this has been inactive for a while...
Projects
None yet
Development

Successfully merging this pull request may close these issues.