allow skip() in async test context; closes #2334

[boneskull]

I can't tell if this is an actual bug fix, regression, or what (because where are the tests for async skip() calls?), but it allows this test:

it('should be skipped async', function (done) {
  const self = this
  setTimeout(function () {
    self.skip()
  }, 1000)
})

To be successfully marked as "pending".

cc @dasilvacontin @maggiesavovska

[boneskull]

Also I'm not sure if my solution to this is smart, terrible, or some combination thereof.

[boneskull]

@mochajs/core Was hoping someone could look at this and another couple PRs I have open--would rather not merge my own code w/o having somebody look at it.

[danielstjules]

related: #1618

Didn't think we supported it yet 😅

Edit: oops, looks like you pulled the specs from the other PR, nvm!

[boneskull]

@danielstjules I didn't even know there was another PR...

[danielstjules]

Footprint is so minimal, integration tests offer pretty good coverage, LGTM :) 👍

[dasilvacontin]

code smell: #946 (comment)

mocha/lib/runner.js

Lines 221 to 234 in 8a37e01

	Runner.prototype.fail = function(test, err) {
	++this.failures;
	test.state = 'failed';
	if (!(err instanceof Error || err && typeof err.message === 'string')) {
	err = new Error('the ' + type(err) + ' ' + stringify(err) + ' was thrown, throw an Error :)');
	}
	err.stack = (this.fullStackTrace || !err.stack)
	? err.stack
	: stackFilter(err.stack);
	this.emit('fail', test, err);
	};

Runner#fail is getting executed with an instance of Pending.

[boneskull]

@dasilvacontin throwing a plain object to mark a test as pending isn't particularly aromatic, no. I didn't write it in the first place but did nothing to improve it. I'm also unsure if there's really a better way to abort a test. Pending could subclass Error, but for what purpose?

[boneskull]

@dasilvacontin I'm not sure if you have issues w/ this PR or not. Can you confirm either way?

[dasilvacontin]

@boneskull No, the PR works. It's just that maybe investigating the code smell would yield a different fix / solution.

I'm okay with merging this and then iterating once someone investigates the code smell.

[maggiesavovska]

Hi, I'm using the most up to date mocha version: 3.7.3 and the async skip isn't working. Can anyone else confirm this?:

it('should skip async', function(done) { var self = this.test; setTimeout(function() { self.skip(); }, 500); }); //end it

[ScottFreeCode]

Double-check which version of Mocha you're using; the latest is 3.1.0 and that code should work on it.

(Also, a GitHub/Markdown tip: use ``` instead of ` for multi-line blocks of code.)

[maggiesavovska]

Thanks! I changed my package.json value for mocha version to 3.1.0, reinstalled, but I'm still getting "3.7.3" when I do "npm -v mocha". Why would this be? Now when I try to run the above "Should skip async" test I get the following error:

UPDATE: My bad. Just did "npm list mocha" and confirmed that I am on 3.1.0, but still getting below error.

1. it should prepare test should skip async:
   Error: the object {
   "message": [undefined]
   "uncaught": true
   } was thrown, throw an Error :)
   at Runner.fail (/usr/local/lib/node_modules/mocha/lib/runner.js:226:11)
   at Runner.uncaught (/usr/local/lib/node_modules/mocha/lib/runner.js:700:8)
   at process.uncaught (/usr/local/lib/node_modules/mocha/lib/runner.js:783:10)
   at emitOne (events.js:95:20)
   at process.emit (events.js:182:7)
   at process._fatalException (node.js:258:26)

[boneskull]

this.test is not what you want. var self = this should do it

[maggiesavovska]

I tried both (this.test and this). Both give the same error:
"it('should skip async', function (done) {
var self = this;
setTimeout(function(){
self.skip();
},500);
});//end it"

[ScottFreeCode]

(Heh, yeah, npm -v [anything] would print the version of NPM. I wondered where that number came from!)

Interestingly, I tried the var self = this.test version without noticing that line and it got marked as pending anyway, so that's shouldn't be the issue. (I guess it must be a circular reference and should therefore be equivalent in any case?) In fact, seeing as the test works for me on the same version of Mocha (and should work for anyone else as far as I know, it's supposed to be supported now), I suspect there's something else causing the error...

• Is this test the only code in the only test file being run?
• What is the mocha command used to run it, and the contents of the mocha.opts file if any?
• Does npm list mocha -g print anything? (If there's a global version and a local version both installed, weird things sometimes can happen.)

[maggiesavovska]

Thank you! Yes, that was the exact problem. I thought I was on 3.1.0 but actually I was using the globally installed version which was 2.4.3. I ran the test with node_modules/.bin/mocha and it worked great.