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

🚀 Feature: Throw on nested it call #4525

Open
dimadk24 opened this issue Dec 1, 2020 · 3 comments
Open

🚀 Feature: Throw on nested it call #4525

dimadk24 opened this issue Dec 1, 2020 · 3 comments
Labels
semver-major implementation requires increase of "major" version number; "breaking changes" status: accepting prs Mocha can use your help with this one! type: feature enhancement proposal

Comments

@dimadk24
Copy link

dimadk24 commented Dec 1, 2020

Is your feature request related to a problem or a nice-to-have?? Please describe.
It would be nice for Mocha to throw if it founds a nested it block. Currently, such blocks are silently ignored (code inside nested block is not called), but developers mistakenly can assume that they are called and test something.

I'm talking about such code:

describe('some function', () => {
  it('should work parent', () => {
    console.log('parent it block')

    // Accidentally created nested it block
    it('should work nested', () => {
      console.log('nested it block')
      assert.equal([1, 2, 3].indexOf(4), -1)
    })
  })
})

In such a simple example it's easy to spot the error but if you have a complex test structure, it's easy to accidentally create such a structure. And you would think that tests really test your code unless you add logs in nested it block and find out that none of your tests are really called.

Example of a more complex test structure:

const sumWithCondition = (condition, input) => {
  if (condition === 1 || condition === 2) {
    return input + 1
  }
  return input + 2
}

const runTest = (input, expected) => {
  testConditions = [1, 2]

  testConditions.forEach(condition => {
    it('should work with condition', () => {
      console.log('called')
      expect(someFunction(condition, input)).to.be.deep.equal(expected)
    })
  })
}

describe('some complex function', () => {
  it('should work with input 1', () => {
    runTest(1, 2)
  })

  it('should work with input 2', () => {
    runTest(2, 3);
  })
})

Describe the solution you'd like
Throw an error if find nested it block

Describe alternatives you've considered
You could also call nested it blocks, but it would create a strange test structure and it would not be consistent with other test libraries (e.g. jest)

Additional context
In jest such feature was implemented in jestjs/jest#9828
repo to check out with mocha and jest: https://github.com/DimaDK02/mocha-opportunity-to-improve/
Run yarn jest to run jest on this test and yarn mocha to run mocha

@dimadk24 dimadk24 added the type: feature enhancement proposal label Dec 1, 2020
@outsideris outsideris added the type: discussion debates, philosophy, navel-gazing, etc. label Jan 3, 2021
@JacobLey
Copy link
Contributor

I currently write tests with ui: 'exports' for this exact reason. I found I was often accidentally writing nested it, and not actually testing code.
The BDD interface seems much better for "dynamic" tests, like in the example above for iterating over a list of possible cases, and making individual tests for each.

Unfortunately it seems like this issue never got any love... Is there a good way to get maintainer attention?

I'd be happy to open a PR to fix as well. I suspect the naive solution is something like:

let isInTest = false;
it = (name, test) => {
    if (isInTest) {
       throw new Error('No nested it');
    }
    doTest(name, test, () => {
        isInTest = false;
    });
};

My understanding of "parallel" mode is that it would still work in that situation, but if really it is important to check if a "nested" it is invoked, something like Node.js async hooks might be a good place to start...

@binoche9
Copy link

+1 This seems like an easy win to help prevent developers from accidentally writing tests that never get invoked and was implemented in jasmine a long time ago

@JoshuaKGoldberg JoshuaKGoldberg removed the type: discussion debates, philosophy, navel-gazing, etc. label Dec 28, 2023
@JoshuaKGoldberg JoshuaKGoldberg changed the title Throw on nested it call 🚀 Feature: Throw on nested it call Dec 28, 2023
@JoshuaKGoldberg JoshuaKGoldberg added the semver-major implementation requires increase of "major" version number; "breaking changes" label Dec 28, 2023
@JoshuaKGoldberg JoshuaKGoldberg added the status: accepting prs Mocha can use your help with this one! label Feb 6, 2024
@JoshuaKGoldberg
Copy link
Member

Ha, I'm surprised this is allowed. Confirmed it's a no-op at the moment.

it("but inside", function () {
  it("doesn't matter", () => {});
});
$ npx mocha test.spec.js


  ✔ but inside

  1 passing (2ms)

This would be a semver-major breaking change as you never know who's relying on this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major implementation requires increase of "major" version number; "breaking changes" status: accepting prs Mocha can use your help with this one! type: feature enhancement proposal
Projects
None yet
Development

No branches or pull requests

5 participants