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

Nested describes don't run if parent describe has .only (mocha 3) #2378

Closed
jhartikainen opened this issue Jul 18, 2016 · 7 comments
Closed
Assignees
Labels
type: bug a defect, confirmed by a maintainer
Milestone

Comments

@jhartikainen
Copy link

jhartikainen commented Jul 18, 2016

Previously, if you had a nested describe and tests inside a parent describe with .only, the child-describe blocks would also run.

It seems #1481 may have broken this behavior. While I think the actual fix in 1481 makes sense, this side-effect of it seems counter-intuitive, and at least goes against my expectations of how this should work. It's also different from how this works in some other testing tools I've used in the past with similar features.

Steps to reproduce

  1. Create a test file with a root describe using .only.
  2. Put another describe inside it, with an it inside
  3. Run test

Expected result

The tests inside the nested describe run

Actual result

Only tests immediately inside the parent describe block run, at least with mocha@3.0.0-1

Sample code

describe.only('this should run', function() {
  it('an it should also run', function() { });

  describe('this nested describe should run', function() {
    it('and this nested it should run as well', function() { });
  });
});

Output:

  this should run
    ✓ an it should also run


  1 passing (14ms)

Expected output:


  this should run
    ✓ an it should also run
    this nested describe should run
      ✓ and this nested it should run as well


  2 passing (17ms)
@boneskull boneskull self-assigned this Jul 20, 2016
@boneskull boneskull added type: bug a defect, confirmed by a maintainer unconfirmed labels Jul 20, 2016
@boneskull
Copy link
Contributor

boneskull commented Jul 20, 2016

Yeah, OK, this was clearly the intent of #1807. See this line.

I'll need to re-read the discussion around it, because I wouldn't expect this behavior.

UPDATE: Fixed the PR number twice

@boneskull
Copy link
Contributor

Hmm. Actually I'm not sure this particular case got coverage.

@boneskull
Copy link
Contributor

This is a bit tricky, so I'm not going to finish it tonight; could use some assistance. Basically what needs to happen is if a suite is marked "only", then unless a test within that suite is marked "only", all child suites' tests should run as well. Or, that's what I'd expect, anyhow.

describe.only('foo', function () {
  describe('bar', function () {
    it('should run', function () {});
  });
});
// do we agree?
describe.only('foo', function () {
  it.only('should run', function () {});
  describe('bar', function () {
    it('should not run', function () {});
  });
});
// careful with the order...
describe.only('foo', function () {
  describe('bar', function () {
    it('should not run', function () {});
  });
  it.only('should run', function () {});
});

also deeply-nested suites may complicate matters, as you can't simply check the most immediate parent to see if it has tests marked "only"; when child suites are created, they should be given a false isOnly prop (regardless of whether describe.only() was used!) when the parent has a truthy onlyTests prop OR when the parent has a false isOnly prop. Given that the default value of isOnly is undefined, one could think of this as a ternary flag...

@boneskull boneskull added this to the v3.0.0 milestone Jul 20, 2016
@boneskull
Copy link
Contributor

cc @mochajs/core

this will delay the final v3 release until it's resolved or we can agree that this is what should be happening, which is unlikely...

boneskull added a commit that referenced this issue Jul 21, 2016
boneskull added a commit that referenced this issue Jul 21, 2016
- includes some more refactoring of interface code into `lib/interfaces/common.js` for DRY (still more work to be done here)
- unfortunately said refactoring contains hellish logic which addresses this issue (someone should make a flowchart of this for lolz)
- original PR did not address `exports` interface; this doesn't either
- may need more coverage against `qunit` interface
@boneskull
Copy link
Contributor

@jhartikainen If you're feeling generous, I'd encourage you to pull down the issue/2378 branch and see if the behavior matches what you expect.

@jhartikainen
Copy link
Author

@boneskull yep appears to function as expected on that branch

boneskull added a commit that referenced this issue Jul 26, 2016
- includes some more refactoring of interface code into `lib/interfaces/common.js` for DRY (still more work to be done here)
- unfortunately said refactoring contains hellish logic which addresses this issue (someone should make a flowchart of this for lolz)
- original PR did not address `exports` interface; this doesn't either
- may need more coverage against `qunit` interface
@boneskull
Copy link
Contributor

closed by 9ea2011, but I'm not 100% convinced, so we'll see if this stays closed.

boneskull added a commit that referenced this issue Aug 1, 2016
boneskull added a commit that referenced this issue Aug 1, 2016
- includes some more refactoring of interface code into `lib/interfaces/common.js` for DRY (still more work to be done here)
- unfortunately said refactoring contains hellish logic which addresses this issue (someone should make a flowchart of this for lolz)
- original PR did not address `exports` interface; this doesn't either
- may need more coverage against `qunit` interface
helderroem added a commit to helderroem/mocha that referenced this issue Aug 7, 2016
* 'master' of github.com:mochajs/mocha: (79 commits)
  Release v3.0.1
  update CHANGELOG.md; rebuild [ci skip]
  fix nested describe.only suites; closes mochajs#2406
  update date in CHANGELOG.md [ci skip]
  Release v3.0.0
  rebuild mocha.js
  fix bad merge of karma.conf.js
  add note about spec reporter to CHANGELOG.md [ci skip]
  fixed typo in mocha.css introduced by 185c0d9 [ci skip]
  Remove carriage return before each test line in spec reporter. Served no purpose
  add "logo" field to package.json [ci skip]
  fix incorrect executable name with new version of commander
  add bower.json to published package for npmcdn support [ci skip]
  fix broken/wrong URLs in CHANGELOG.md [ci skip]
  Release v3.0.0-2
  rebuild mocha.js
  add browser-stdout to dependencies
  update CHANGELOG [ci skip]
  let child suites run if parent is exclusive; closes mochajs#2378 (mochajs#2387)
  Upgrade eslint package to 2.13 version (mochajs#2389)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

No branches or pull requests

2 participants