Repository: mochajs/mocha

Problem statement:
`this.skip()` completely broken if html reporter used.
Demo: https://github.com/Kirill89/mocha-test

`Error: TypeError: test.err is undefined (http://localhost:1111/node_modules/mocha/mocha.js:2786)`
- Firefox 38.0.5
- Chrome 43.0.2357.124 (64-bit)

this.skip causes: Cannot read property 'toString' of undefined
Error line in mocha.js

``` js
    var el;
    if (test.state === 'passed') {
      var url = self.testURL(test);
      el = fragment('<li class="test pass %e"><h2>%e<span class="duration">%ems</span> <a href="%s" class="replay">‣</a></h2></li>', test.speed, test.title, test.duration, url);
    } else if (test.pending) {
      el = fragment('<li class="test pass pending"><h2>%e</h2></li>', test.title);
    } else {
      el = fragment('<li class="test fail"><h2>%e <a href="%e" class="replay">‣</a></h2></li>', test.title, self.testURL(test));
      var stackString; // Note: Includes leading newline
      var message = test.err.toString(); // mocha.js:2427
```

My test cases are

``` js
describe('mod', function(){
  describe('mod1', function(){
    before(function(){
       if(!modLoaded) this.skip(); // causes the error
    });

    it('should do stuff', function(){
       // stuff
    });
  });
});
```

Not sure why `this.skip()` is causing an error...



Hints:
what are you trying to accomplish by skipping the hook?

@boneskull I'm trying skip whole "describe" section.

@boneskull mocha already has same [test](https://github.com/mochajs/mocha/blob/master/test/integration/fixtures/pending/skip.sync.before.js), and it is works for reporter "spec". Can you explain me why doesn't work for "html" reporter?

Looks like you're right. Avoiding async hooks/specs, I was able to replicate in the browser:

``` javascript
describe('foo', function () {
  before(function() {
    this.skip();
  });

  it('bar', function() {
    // test
  });
});
```

From the console:

```
  foo
    - bar


  0 passing (9ms)
  1 pending
```

But in the browser:

<img width="318" alt="screen shot 2015-07-15 at 2 39 57 am" src="https://cloud.githubusercontent.com/assets/817212/8695598/d4897ed6-2a9a-11e5-9c48-02eecff70d1c.png">

So seems like it's not being correctly labeled as pending.

@Kirill89 Are you able to get `this.skip()` to work from a spec, in the browser?

``` javascript
describe('foo', function () {
  it('bar', function() {
    this.skip();
  });
});
```

@danielstjules I am surprised, but it is also doesn't work. But with another error.

![](https://www.dropbox.com/s/1jfrf9dp13bvi4m/Screenshot%202015-07-15%2018.46.53.png?dl=1)

Thanks for confirming! Both errors are being propagated by the HTML reporter due to it trying to act on a nonexistent err property for the test, since it wasn't correctly marked as pending. Will look into where that regression may have been introduced. Appreciate your help!

really nice feature this.skip( what I was looking for ) in hooks(before, after, etc). My question: why is that not documented? it there more nice magic features what is not documented? :)

@danielstjules any chances to get it fixed soon?

Could you change a label? Needs-Feedback -> Bug?

Problem is with this check https://github.com/mochajs/mocha/blob/9b1e587b2a78c8f99248d2c4b0a48ea6620fd468/lib/reporters/html.js#L158

If i change this line to 

``` js
   } else if (test.pending || !test.err) {
```

...then code seems to work. But i'm not sure such hack is correct. Could anyone check why `.pending` flag is lost in `test end` event after `this.skip()`?

This will be fixed by https://github.com/mochajs/mocha/pull/1945#issuecomment-170399078

Thanks for info!

Still not fixed with 2.4.3. Test from the first message does not shows error on screen, but throw errors to console and mark test as failed in summary.

Something become better, but not all.

You're right - `this.skip()` is mostly broken in the browser. I haven't gotten around to it though.

It seems to be fixed for `it`, but not for `before` (most demanded)



Patch:
diff --git a/lib/interfaces/bdd.js b/lib/interfaces/bdd.js
--- a/lib/interfaces/bdd.js
+++ b/lib/interfaces/bdd.js
@@ -79,7 +79,7 @@ module.exports = function(suite) {
 
     var it = context.it = context.specify = function(title, fn) {
       var suite = suites[0];
-      if (suite.pending) {
+      if (suite.isPending()) {
         fn = null;
       }
       var test = new Test(title, fn);
diff --git a/lib/interfaces/tdd.js b/lib/interfaces/tdd.js
--- a/lib/interfaces/tdd.js
+++ b/lib/interfaces/tdd.js
@@ -81,7 +81,7 @@ module.exports = function(suite) {
      */
     context.test = function(title, fn) {
       var suite = suites[0];
-      if (suite.pending) {
+      if (suite.isPending()) {
         fn = null;
       }
       var test = new Test(title, fn);
diff --git a/lib/reporters/html.js b/lib/reporters/html.js
--- a/lib/reporters/html.js
+++ b/lib/reporters/html.js
@@ -155,7 +155,7 @@ function HTML(runner) {
     if (test.state === 'passed') {
       var url = self.testURL(test);
       el = fragment('<li class="test pass %e"><h2>%e<span class="duration">%ems</span> <a href="%s" class="replay">‣</a></h2></li>', test.speed, test.title, test.duration, url);
-    } else if (test.pending) {
+    } else if (test.isPending()) {
       el = fragment('<li class="test pass pending"><h2>%e</h2></li>', test.title);
     } else {
       el = fragment('<li class="test fail"><h2>%e <a href="%e" class="replay">‣</a></h2></li>', test.title, self.testURL(test));
@@ -193,7 +193,7 @@ function HTML(runner) {
 
     // toggle code
     // TODO: defer
-    if (!test.pending) {
+    if (!test.isPending()) {
       var h2 = el.getElementsByTagName('h2')[0];
 
       on(h2, 'click', function() {
diff --git a/lib/reporters/xunit.js b/lib/reporters/xunit.js
--- a/lib/reporters/xunit.js
+++ b/lib/reporters/xunit.js
@@ -131,7 +131,7 @@ XUnit.prototype.test = function(test) {
   if (test.state === 'failed') {
     var err = test.err;
     this.write(tag('testcase', attrs, false, tag('failure', {}, false, cdata(escape(err.message) + '\n' + err.stack))));
-  } else if (test.pending) {
+  } else if (test.isPending()) {
     this.write(tag('testcase', attrs, false, tag('skipped', {}, true)));
   } else {
     this.write(tag('testcase', attrs, true));
diff --git a/lib/runnable.js b/lib/runnable.js
--- a/lib/runnable.js
+++ b/lib/runnable.js
@@ -54,6 +54,7 @@ function Runnable(title, fn) {
   this._trace = new Error('done() called multiple times');
   this._retries = -1;
   this._currentRetry = 0;
+  this.pending = false;
 }
 
 /**
@@ -124,12 +125,21 @@ Runnable.prototype.enableTimeouts = function(enabled) {
 /**
  * Halt and mark as pending.
  *
- * @api private
+ * @api public
  */
 Runnable.prototype.skip = function() {
   throw new Pending();
 };
 
+/**
+ * Check if this runnable or its parent suite is marked as pending.
+ *
+ * @api private
+ */
+Runnable.prototype.isPending = function() {
+  return this.pending || (this.parent && this.parent.isPending());
+};
+
 /**
  * Set number of retries.
  *
@@ -302,7 +312,7 @@ Runnable.prototype.run = function(fn) {
 
   // sync or promise-returning
   try {
-    if (this.pending) {
+    if (this.isPending()) {
       done();
     } else {
       callFn(this.fn);
diff --git a/lib/runner.js b/lib/runner.js
--- a/lib/runner.js
+++ b/lib/runner.js
@@ -507,12 +507,7 @@ Runner.prototype.runTests = function(suite, fn) {
       return;
     }
 
-    function parentPending(suite) {
-      return suite.pending || (suite.parent && parentPending(suite.parent));
-    }
-
-    // pending
-    if (test.pending || parentPending(test.parent)) {
+    if (test.isPending()) {
       self.emit('pending', test);
       self.emit('test end', test);
       return next();
@@ -521,7 +516,7 @@ Runner.prototype.runTests = function(suite, fn) {
     // execute test and hook(s)
     self.emit('test', self.test = test);
     self.hookDown('beforeEach', function(err, errSuite) {
-      if (suite.pending) {
+      if (suite.isPending()) {
         self.emit('pending', test);
         self.emit('test end', test);
         return next();
diff --git a/lib/suite.js b/lib/suite.js
--- a/lib/suite.js
+++ b/lib/suite.js
@@ -28,9 +28,6 @@ exports = module.exports = Suite;
 exports.create = function(parent, title) {
   var suite = new Suite(title, parent.ctx);
   suite.parent = parent;
-  if (parent.pending) {
-    suite.pending = true;
-  }
   title = suite.fullTitle();
   parent.addSuite(suite);
   return suite;
@@ -176,6 +173,15 @@ Suite.prototype.bail = function(bail) {
   return this;
 };
 
+/**
+ * Check if this suite or its parent suite is marked as pending.
+ *
+ * @api private
+ */
+Suite.prototype.isPending = function() {
+  return this.pending || (this.parent && this.parent.isPending());
+};
+
 /**
  * Run `fn(test[, done])` before running tests.
  *
@@ -185,7 +191,7 @@ Suite.prototype.bail = function(bail) {
  * @return {Suite} for chaining
  */
 Suite.prototype.beforeAll = function(title, fn) {
-  if (this.pending) {
+  if (this.isPending()) {
     return this;
   }
   if (typeof title === 'function') {
@@ -215,7 +221,7 @@ Suite.prototype.beforeAll = function(title, fn) {
  * @return {Suite} for chaining
  */
 Suite.prototype.afterAll = function(title, fn) {
-  if (this.pending) {
+  if (this.isPending()) {
     return this;
   }
   if (typeof title === 'function') {
@@ -245,7 +251,7 @@ Suite.prototype.afterAll = function(title, fn) {
  * @return {Suite} for chaining
  */
 Suite.prototype.beforeEach = function(title, fn) {
-  if (this.pending) {
+  if (this.isPending()) {
     return this;
   }
   if (typeof title === 'function') {
@@ -275,7 +281,7 @@ Suite.prototype.beforeEach = function(title, fn) {
  * @return {Suite} for chaining
  */
 Suite.prototype.afterEach = function(title, fn) {
-  if (this.pending) {
+  if (this.isPending()) {
     return this;
   }
   if (typeof title === 'function') {


Test patch:
diff --git a/test/suite.js b/test/suite.js
--- a/test/suite.js
+++ b/test/suite.js
@@ -288,6 +288,11 @@ describe('Suite', function(){
       this.first.suites.should.have.length(1);
       this.first.suites[0].should.equal(this.second);
     });
+
+    it('treats suite as pending if its parent is pending', function(){
+      this.first.pending = true
+      this.second.isPending.should.be.true
+    });
   });
 
   // describe('.addTest()', function(){
diff --git a/test/test.js b/test/test.js
--- a/test/test.js
+++ b/test/test.js
@@ -1,4 +1,5 @@
 var mocha = require('../')
+  , should = require('should')
   , Context = mocha.Context
   , Test = mocha.Test;
 
@@ -52,4 +53,24 @@ describe('Test', function(){
       this._test.clone().file.should.equal('bar');
     });
   });
+
+  describe('.isPending()', function(){
+    beforeEach(function(){
+      this._test = new Test('Is it skipped', function () {});
+    });
+
+    it('should not be pending by default', function(){
+      should(this._test.isPending()).not.be.ok();
+    });
+
+    it('should be pending when marked as such', function(){
+      this._test.pending = true;
+      should(this._test.isPending()).be.ok();
+    });
+
+    it('should be pending when its parent is pending', function(){
+      this._test.parent = { isPending: function(){ return true } };
+      should(this._test.isPending()).be.ok();
+    });
+  });
 });