Fix 1604: Add support for async skip()

[danielstjules]

This is for #1604

it('should skip', function(done) {
  var test = this;

  setTimeout(function() {
    test.skip();
  }, 0);
});

it('should run', function() {
  // do nothing
});

dstjules:~/git/mocha (master=)
$ ./bin/mocha example.js

  ․․

  1 passing (14ms)
  1 failing

  1)  should skip:
     Error: the object {
  "message": [undefined]
  "uncaught": true
} was thrown, throw an Error :)
      at Runner.fail (lib/runner.js:206:11)
      at Runner.uncaught (lib/runner.js:583:8)
      at process.uncaught (lib/runner.js:612:10)

dstjules:~/git/mocha (master=)
$ git checkout issue-1604
Switched to branch 'issue-1604'

dstjules:~/git/mocha (issue-1604)
$ ./bin/mocha example.js

  ․․

  1 passing (6ms)
  1 pending

[danielstjules]

@a8m @dasilvacontin :)

[a8m]

Thanks Dan, Looks good. but there's an issue with the global suite.
e.g:
for this one:

beforeEach(function() {
   this.skip()
})
it('should never run this test', function() {
  throw new Error('never thrown')
})

describe('Should run this suite', function() {
  it('run this test case', function() {
    // ..
  })
})

I get the following:

 - should never run this test
  Should run this suite
    ✓ run this test case


  1 passing (10ms)
  1 pending

but with this one:

beforeEach(function(done) {
  var hook = this
  setTimeout(function() {
    hook.skip()
  })
})
it('should never run this test', function() {
  throw new Error('never thrown')
})

describe('Should run this suite', function() {
  it('run this test case', function() {
    // ..
  })
})

I get the following:

  - "before each" hook

  0 passing (9ms)
  1 pending

let me know if something isn't clear.

[danielstjules]

Looks like you found a bug with the existing skip() behavior with your first example: run this test case should be skipped as well. It's not unique to the root suite, either:

describe('suite', function() {
  beforeEach(function() {
    console.log('hook');
    this.skip();
  });
  it('should never run this test', function() {
    throw new Error('never thrown');
  });
  describe('Should run this suite', function() {
    it('should never run this test', function() {
      throw new Error('never thrown');
    });
  });
});

// Result
$ mocha test.js

  hook

  ․hook
․

  0 passing (6ms)
  1 pending
  1 failing

We should probably open a second issue for that. As for the second example, I'll get a fix ready, thanks! :)

[danielstjules]

@a8m Fixed!

// test.js
beforeEach(function(done) {
  var hook = this
  setTimeout(function() {
    hook.skip()
  })
})
it('should never run this test', function() {
  throw new Error('never thrown')
})
describe('Should run this suite', function() {
  it('run this test case', function() {
    // ..
  })
});

// output
$ ./bin/mocha --reporter spec test.js


  - should never run this test
  Should run this suite
    ✓ run this test case


  1 passing (10ms)
  1 pending

The above output matches what you'd get using the synchronous skip.

It's probably worth discussing whether or not asynchronously skipping a test is functionality we'd like to support. Could be a rabbit hole. :) What do you think @boneskull ?

[dasilvacontin]

What's the benefit of async skipping a test vs delayed suite definition? Besides maybe cleaner?

[danielstjules]

That may be the only reason - it might look cleaner. The same applies to using skip() from a synchronous spec.

describe('suite', function() {
  var someCondition = getConditionValue();

  // In conditional block, which isn't ideal as it
  // won't show up as pending
  if (someCondition) {
    it('test', function() {
      assert(true);
    });
  }

  // Handling the spec so that it'll show as pending
  var test = (someCondition) ? it : it.skip;
  test('test', function() {
    assert(true);
  });

  // Using skip()
  it('test', function() {
    if (!someCondition) return this.skip();
    assert(true);
  });
});

I don't personally rely on this functionality. But if we see value in a synchronous skip, maybe it makes sense to support it for async specs as well? I'm not sure. I wouldn't want to rely on this sort of behavior since it involves uncaught exceptions. I'd probably go out of my way to avoid it, and stick to it.skip().

[dasilvacontin]

If it's well tested, I wouldn't mind using it.

Btw, not picky concretely with this case, as there are many more existing tests in this style, but I personally prefer making tests which must all pass. e.g., for this case, ideally I would run mocha inside mocha, asserting the quantity of skipped tests or which tests were skipped. The problem with this is the uncaughtException listener, that messes all up, since the exceptions of the mocha instance you are testing are caught by the mocha instance you are using to test.

I don't really like having tests that are expected to be skipped, since if they suddenly start passing one does not notice unless he/she reads carefully the npm test output line by line.

[danielstjules]

Completely agree! Full integration tests for some cases would be nice. You could execute mocha and check its output, kinda like https://github.com/danielstjules/buddy.js/blob/master/spec/integration/buddySpec.js If we were to pursue running mocha in mocha, it would be better to also rely on the last known good release of mocha, rather than using bin/mocha. Otherwise you end up with the same problem.

[danielstjules]

If it's well tested, I wouldn't mind using it.

I think the existing behavior of skip() (non-async) is likely inconsistent. See #1618 (comment) This async implementation mimics that output. Both could be updated if others agree.

I guess the real reason I'm not sure about async skip is that you're relying on there not being other issues, outside the currently running spec. E.g. if an exception is thrown from some random timeout and wrongly attributed to a spec that would be skipped. It becomes more difficult to reason about. I haven't experienced this scenario, but it seems common among those having trouble with their tests (or codebase).

$ cat test.js
setTimeout(function() {
  throw new Error('ohno!');
}, 100);

it('test', function(done) {
  setTimeout(function() {
    this.skip();
  }.bind(this), 100);
});

danielstjules:~/git/mocha (issue-1604 =)
$ ./bin/mocha --reporter spec test.js


  1) test

  0 passing (105ms)
  1 failing

  1)  test:
     Uncaught Error: ohno!
      at null._onTimeout (test.js:2:9)

[dasilvacontin]

True. Actually, I started creating tests for async errors ending up in other suites, but the .uncaughtError thing was a barrier.

[a8m]

Tested. works good @danielstjules .

I think the existing behavior of skip() (non-async) is likely inconsistent.

Agree, but at least 'async' aligned to behave the same way as 'non-async'.

[danielstjules]

Thanks @a8m!

@dasilvacontin I'm going to add some "integration" tests for this, under a new test directory.

[danielstjules]

@dasilvacontin Got some quick integration tests running.

@travisjeffery @boneskull Mind taking a look? :)

[travisjeffery]

lgtm!

[a8m]

LGTM

[travisjeffery]

before we merge it i just wanna think about whether we should add it, or just say used a delayed suite. considering pretty much no one in the thread has said they'd actually use this and since exceptions are a downside

[danielstjules]

Though I don't personally use skip(), I think that if it's available for sync code, it might makes sense to offer for async specs/hooks as well? The alternative doesn't seem as clean to me: #1618 (comment)

Edit: Happy to discuss. :) I was certainly on the fence with the idea at first, though I guess I'm more supportive of it now. Looking at #332 and #946 I think it was a popular request

[danielstjules]

Since the integration tests are a bit slower, and the acceptance tests are ran against the jsapi, I've moved them into their own directory. This way make test will still only run faster unit tests, but make test-integration, npm test or make test-all will include all integration tests.

[danielstjules]

@mochajs/mocha Any thoughts? :)

[dasilvacontin]

Couldn't look at it, but damn, those tests are sexy.

I'd rather let this be merged by either @travisjeffery or @boneskull too. Besides, @travisjeffery said he wanted to taco about it.

[a8m]

but damn, those tests are sexy.

I fully agree with you!
great work @danielstjules

[dasilvacontin]

Even if this doesn't get merged, I could use the helper for the tests I was doing for BaseReporter.

[boneskull]

@danielstjules Does this make --delay redundant?

[danielstjules]

I don't think so, though there's some overlap in the problems that they can solve.

[danielstjules]

@boneskull To achieve dynamic skip-like functionality using defer, I believe you end up having to resort to roughly the same logic as I mentioned in #1618 (comment)

[danielstjules]

@boneskull were you looking for additional details for this PR?

[danielstjules]

@mochajs/mocha If there's interest in bringing this into 2.3.0, I'll do another quick rebase. It would offer some additional flexibility, solving the problem discussed in #1480 (comment)

[jbnicolai]

@danielstjules big 👍 as far as I'm concerned.

And don't worry too much about being quick to catch 2.3.0, I think it'll be an other week before we publish the release, and we're discussing merging some breaking changes as well and bumping it up to 3.0.0 instead - see #1782

[boneskull]

@jbnicolai if we do release the major, we need to create a new milestone and retarget everything targeted to v3.0.0 to v4.0.0. then we can rebase branch v3.0.0 into master.

cc @mochajs/mocha

anyway, it looks like this needs rebasing (again), so whoever does that can merge it

[danielstjules]

@boneskull Rebased. Should I target 2.3.0, or is master fine?

[boneskull]

@danielstjules sorry, didn't see this one. we can release it in 2.3.1 if you wish

[danielstjules]

@boneskull Did you mean 2.3.0? If so, I can rebase again and get things ready to merge :) Doesn't look like 2.3.0 has been tagged yet https://www.npmjs.com/package/mocha

[boneskull]

@danielstjules I'm about to tag v2.3.0

[bcoe]

I'm jumping through hoops to get functionality similar to async skip() in the node_redis test-suite, would love this feature.

[danielstjules]

I'll pick this back up - should be pretty easy to get it working with latest master. However, I'm not sure when a realistic date would be for another minor release.

[laughinghan]

What's blocking this from merging? This doesn't appear to have made it to 2.3 nor 2.4.

before we merge it i just wanna think about whether we should add it, or just say used a delayed suite.

What is a "delayed suite"? The only mention of "delay" I can find in the docs is delaying the root suite, which, I can't imagine any use cases for async skipping tests that would be solved by that.

And yes, my use case does require skipping an async test, I'm testing an interaction between my code and .select() and .blur() and the resulting focus and blur events on a DOM textarea. The browser behavior that used to cause a bug in my library only happens when the browser window is focused, so this particular test is incorrectly failing when I run my test suite in a background tab, but why should I have to manually grep to exclude/include this test when I decide whether I want to run the tests in a foreground or background browser tab? The code is perfectly aware of whether the browser window is focused without me manually passing in a tag with grep, and should be perfectly able to report whether the test passed, failed, or was skipped because the browser window wasn't focused.

Is there any help I can offer to get this merged?

[boneskull]

I think @danielstjules just needs to clean it up a bit, but I'm not sure. perhaps he can comment.

[boneskull]

(the tests for this PR aren't actually passing, so something is broken)

[danielstjules]

This solution doesn't work anymore after a patch release changing how some error handling was done, haven't been able to come up with an alternative yet.