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

Skipped tests should have their beforeEach() skipped too? #337

Closed
dminkovsky opened this issue Apr 7, 2015 · 11 comments
Assignees
Labels
Milestone

Comments

@dminkovsky
Copy link

@dminkovsky dminkovsky commented Apr 7, 2015

When tests are skipped, their beforeEach() runs anyway. This doesn't make sense to me. Thank you!

@dminkovsky dminkovsky changed the title Skipped tests should have beforeEach Skipped tests should have their beforeEach() skipped too? Apr 7, 2015
@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Apr 14, 2015

Can you provide a short example?

@bkendall

This comment has been minimized.

Copy link
Contributor

@bkendall bkendall commented Apr 17, 2015

I've hit this issue in my testing before, so I hope I can shed some light on it for you, @hueniverse.

Say I have a test file as follows (the important parts):

describe('one test', function () {
  before(function (done) { console.log('one before'); done(); });
  after(function (done) { console.log('one after'); done(); });

  it('should add all the things', function (done) {
    expect(true).to.equal(true);
    done();
  });
});

describe('two test', function () {
  before(function (done) { console.log('two before'); done(); });
  after(function (done) { console.log('two after'); done(); });

  it('should subtract all the things', function (done) {
    expect(true).to.equal(true);
    done();
  });
});

Running lab --verbose gives the following output:

one before
one test
  ✔ 1) should add all the things (3 ms)
one after
two before
two test
  ✔ 2) should subtract all the things (0 ms)
two after


2 tests complete

Now, if I want to run the second test alone, I expect that the before and after functions for the first would not run. I find that's not the case, however:
edit: running lab --verbose -i 2

one before
one after
two before
two test
  ✔ 2) should subtract all the things (2 ms)
two after


1 tests complete

It still only runs one test, but it's running the before/after statements of the first test as well. Hopefully that helps shed some light on this! (and @dminkovsky I hope this is accurate to what you were seeing)

@tjmehta

This comment has been minimized.

Copy link
Contributor

@tjmehta tjmehta commented Apr 17, 2015

@bkendall hit it spot on

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Apr 18, 2015

@bkendall This is not the same issue as the one reported above. Your issue is that lab doesn't check if a describe() section is empty before executing before/after actions. The issue reported here originally is about beforeEach() which has different logic and should not run.

Solving the empty describe() section is a bit more involved because there can be many nested sections. To solve it cleanly, we probably have to do a pre-iteration phase to figure out the real list of tests and then execute that.

Either way, since I never use any of the before/after features, I don't have a strong opinion as to how this should be resolved. I will leave it up to @geek to sort it out.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Apr 18, 2015

@hueniverse is there a reason why you don't use the before/after features?

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Apr 18, 2015

@AdriVanHoudt because it makes tests too reliant on external conditions and over time the setup and teardown adds up and gets abused. I like each test to be completely atomic and separate without any dependencies. It allows me to move tests around without having to worry about any dependencies. I just much rather do a bit more work setting up common test utils or just a bit of repeating setup than get myself a much more complex environment.

It is not obvious when you have small test files but after a couple of years these things always came back to bite me in the ass.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Apr 18, 2015

@hueniverse seems reasonable, thanks for the thorough explanation!

@geek geek added the request label Apr 20, 2015
@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Apr 20, 2015

I am open to this change, it will require some rework of how we calculate empty describe sections as Eran mentioned. Like him, I too avoid before/after completely... but since we have them we might as well get them right :)

@garthk

This comment has been minimized.

Copy link

@garthk garthk commented Apr 29, 2015

Either that, or it's time for lab 6.0.0. <dive-for-cover />

More seriously: please don't remove before et al. Just fix them. You'll need to bump major regardless, though, as strictly speaking it'll be a breaking change.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Apr 29, 2015

@garthk we won't remove them, no worries about that. I agree, we need to get it working correctly. As we don't use them, the priority for fixing it is a little lower. #264 is one of the next enhancements I want to tackle.

@geek geek added feature and removed request labels May 17, 2016
@geek geek added this to the 10.5.2 milestone May 17, 2016
@geek geek self-assigned this May 17, 2016
@cjihrig cjihrig closed this in #584 May 18, 2016
@bkendall

This comment has been minimized.

Copy link
Contributor

@bkendall bkendall commented May 18, 2016

Woo! 🎉 👍 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.