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

[rprieto]

⚠️ 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

[boneskull]

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]

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]

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]

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]

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]

Yes, I like the new implementation.

[boneskull]

these two lines can be rewritten as

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

[boneskull]

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

[rprieto]

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]

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]

@rprieto Can you try rebasing and squashing please?

[rprieto]

Done!

[rprieto]

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]

Docs are in the gh-pages branch.

[rprieto]

Should I raise a separate PR for the docs?

[dasilvacontin]

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

[rprieto]

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

[dasilvacontin]

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

[dasilvacontin]

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]

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.

[willclarktech]

+1
I just spent ages trying to work out why seemingly random extra tests were being run outside a .only before I discovered #1481. Tags would completely sidestep that issue.

[munkyjunky]

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

[hjalet]

+1

[jdmarshall]

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]

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]

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.

[MatthewRalston]

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

[jcrben]

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

[rprieto]

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]

@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

[dasilvacontin]

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

[dasilvacontin]

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

[rprieto]

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?

[dasilvacontin]

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

(like 12ms per 100 000 tests)

[dasilvacontin]

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')

[dasilvacontin]

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.

[rprieto]

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

[dasilvacontin]

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

[dasilvacontin]

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

[dasilvacontin]

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]

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]

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]

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]

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.

[jsf-clabot]

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]

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]

+1

[sn1ckers]

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]

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!