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

Allow skip from test context for #332 #946

Merged
merged 1 commit into from
Feb 3, 2015
Merged

Allow skip from test context for #332 #946

merged 1 commit into from
Feb 3, 2015

Conversation

cmbuckley
Copy link
Contributor

Opening a PR to get some feedback for this implementation.

@cmbuckley
Copy link
Contributor Author

Copied from my comment on the referenced issue regarding concerns with this PR as a first draft:

One part that feels a little ugly at the moment is that calling this.skip() in afterEach will skip all subsequent tests. It's asked for as part of the original comment, but it feels too counter-intuitive to leave as-is for me.

I'm also concerned about just using a pending property on the error, but it works as proof-of-concept and could be improved upon. See below.


function Pending(message) {
this.constructor.prototype.__proto__ = Error.prototype;
Error.captureStackTrace(this, this.constructor);
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be able to get away with leaving all this stuff out since we're catching anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, perhaps this was gold-plating on my part. I’ll add a commit to remove it. What are your thoughts on how it behaves in afterEach (and in general)?

@travisjeffery
Copy link
Contributor

we already have a way to skip test with the xit apis so this would be inconsistent with those. and inside the test you could just use a return statement.

@cmbuckley
Copy link
Contributor Author

I'm sorry, but I'm not quite sure how the existing xit APIs address the problems described in #332 and #591.

At best, they seem to be a temporary measure — akin to commenting out a test while trying something out.

In #332 you mention the need for use cases, as there are a lot of +1's on the thread. In my mind, @grampajoe gives the best example.

Simply adding a return inside a test is not helpful at all — it will appear to the user as if the test has passed, which is not true.

So currently, users need to do one of the following:

// Option 1: just don't run the tests.
// Not as bad as reporting a pass, but doesn't give the full picture.
if (frobulation_supported()) {
    describe('tests dependent on frobulation', function () {
    });
}

// Option 2: Come up with their own way to mark these tests as skipped.
// A little clunky, not very readable or portable.
var describe_frobulation = (frobulation_supported() ? describe : describe.skip);

describe_frobulation('tests dependent on frobulation', function () {
});

The proposed change would mean users could do this:

describe('tests dependent on frobulation', function () {
    before(function () {
        if (frobulation_not_supported()) this.skip();
    });
});

Or even:

this.skipIf(frobulation_not_supported());

Unless you would suggest a different way to address these concerns with the existing API?

@travisjeffery
Copy link
Contributor

alright, i see. seems strange but lots of people want it.

i'm not sure if this is the best way, but something will happen shortly.

@cmbuckley
Copy link
Contributor Author

I wasn't sure either, but I'd welcome any feedback on the PR.

@aklt
Copy link

aklt commented Feb 3, 2014

This would be a nice feature to add.

We ran into the need to skip a test if a condition in one of the environments the test suite is run in
is not met. An asynchronous function is used to check if the environment meets the conditions and it would be nice if the test was marked pending in the resulting test report.

Sounds like this patch, so +1 from me.

@sukima
Copy link
Contributor

sukima commented Feb 10, 2014

Easy, already implemented:

describe("conditional suite", function() {
  this.pending = shouldThisBePending(); // true or false
  it("is a pending test conditionally", function() {
    throw "This test failed";
  });
});

@aklt
Copy link

aklt commented Feb 10, 2014

@sukima thanks for the suggestion, but this does not seem to work when this.pending needs to be set in an asynchronous function.

@pwaller
Copy link

pwaller commented May 17, 2014

I have an immediate need for this too. I wish to skip tests if a particular transient network service isn't available, I don't want it to fail everything, but just run if they're available.

@aharpervc
Copy link

👍 great feature, it's irritating that this isn't part of the base library

@jqdometita
Copy link

@travisjeffery probably concerns with inconsistencies. Do you suggest an edit on the implementation?

@boneskull
Copy link
Contributor

@travisjeffery I'm going to reopen this one to get it back on the table, please close again if you have a different solution.

@boneskull boneskull reopened this Aug 26, 2014
@Natim
Copy link

Natim commented Aug 26, 2014

@starsquare could you please rebase on top of master?

@cmbuckley
Copy link
Contributor Author

Rebased, and all looks good.

@Natim
Copy link

Natim commented Aug 26, 2014

Great thx.

@boneskull boneskull added the status: waiting for author waiting on response from OP - more information needed label Sep 13, 2014
@boneskull
Copy link
Contributor

@travisjeffery Please let me know if this is OK to merge; it wasn't clear if you had something else in the works.

@travisjeffery
Copy link
Contributor

alright sounds good, just need to update the pr so we've got a clean merge. and we're g2g

@cmbuckley
Copy link
Contributor Author

Rebased off current master.

@dasilvacontin
Copy link
Contributor

👍

@dasilvacontin
Copy link
Contributor

Needs rebasing again. I will merge if you rebase. Sorry for the hassle.

@cmbuckley
Copy link
Contributor Author

Rebased again, no issues or conflicts 😄

@dasilvacontin
Copy link
Contributor

Thanks!

dasilvacontin added a commit that referenced this pull request Feb 3, 2015
Allow skip from test context for #332
@dasilvacontin dasilvacontin merged commit 6402fff into mochajs:master Feb 3, 2015
@artkoshelev
Copy link

any chances to see new version with this feature included soon?

@maggiesavovska
Copy link

maggiesavovska commented Jun 22, 2016

Hi! I can't get this to work with webdriver.io (see below example). Also, was this reverted? This feature doesn't seem to be present in the current version of mocha??

 `it('should be skipped async', function(done){
  var self = this;
  //setTimeout(function(){
    //self.skip(); //works -- test prints as pending in terminal
  //},1000);

  driver.waitForExist('body').then(function(){
    console.log(self); //correct
    self.skip.bind(self)(); //doesn't work -- test prints as error in terminal
  });
 })`

@cmbuckley
Copy link
Contributor Author

This was merged in 6402fff so is present in 2.2.0+. Any more information on the error? May be worth filing a separate issue.

@dasilvacontin
Copy link
Contributor

dasilvacontin commented Jun 22, 2016

@maggiesavovska what's the actual error? Is the test timing out, or is it something else?

@dasilvacontin
Copy link
Contributor

I just experienced this:

async.js

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


  1) should be skipped async

  0 passing (1s)
  1 failing

  1)  should be skipped async:
     Error: the object {
  "message": [undefined]
  "uncaught": true
} was thrown, throw an Error :)

@maggiesavovska
Copy link

@dasilvacontin + @cmbuckley: Please forget what I said about webdriver.io. I think it's a general issue. I get the same as @dasilvacontin (uncaught error with undefined message). I actually was only able to get this to work at all by modifying the current version of mocha so I'm guessing this feature is somehow broken due to recent changes (regardless of whether its used with webdriver.io promise or not).

@maggiesavovska
Copy link

Hi, sorry to be a bug, but were you able to confirm that this feature isn't currently working? I really need this for the project I'm working on now. Thanks!

@boneskull
Copy link
Contributor

Well, I can confirm @dasilvacontin 's code is throwing that error

@boneskull
Copy link
Contributor

Is this a regression? Did we ever have this support for async tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.