mocha icon indicating copy to clipboard operation
mocha copied to clipboard

🚀 Feature: Throw on nested it call

Open dimadk24 opened this issue 4 years ago • 3 comments

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 https://github.com/facebook/jest/pull/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 avatar Dec 01 '20 09:12 dimadk24

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...

JacobLey avatar Aug 12 '22 15:08 JacobLey

+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

binoche9 avatar Jan 10 '23 23:01 binoche9

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.

JoshuaKGoldberg avatar Feb 06 '24 22:02 JoshuaKGoldberg