'no-sibling-hooks': false-positives inside functions

[nhardy]

I have a rough example below:

describe('a thing', () => {

  before(() => {
    // do some global setup
  });

  function runSomeTests() {
    before(() => { // <-- Incorrectly triggers 'mocha/no-sibling-hooks'
      // stuff
    });
    it('should test a different thing', () => {
      // test
    });
  }

  function runSomeOtherTests() {
    before(() => { // <-- Incorrectly triggers 'mocha/no-sibling-hooks'
      // stuff
    });
    it('should test a thing', () => {
      // test
    });
  }

  context('one thing', () => {
    before(() => {
      // more setup
    });
    context('nested thing', () => {
      runSomeTests();
    });
    context('other nested thing', () => {
      runSomeOtherTests();
    });
  });

  context('another thing', () => {
    before(() => {
      // different setup
    });
    context('nested thing', () => {
      runSomeTests();
    });
    context('other nested thing', () => {
      runSomeOtherTests();
    });
  });
});

[lo1tuma]

Thanks for the issue.

The problem with the example you provided is that it depends on where runSomeTests is called, so it could create a sibling hook if it would be called directly within the top-level describe function. I don't think there is a reliable way to detect such edge-cases with static code analysis. So the question is should we always skip hook functions that are not direct children of a describe function? Then the rule would't be able to detect all possible sibling hooks but it is probably less of a problem than having false positives.

[jfmengels]

This looks like a very complex setup for your tests, and maybe it makes sense in your case, but this seems like an edge case to me.

I would rather advocate to disable the rule on this file, unless you have a reasonable way to handle non-direct children.

I'd prefer handling this case correctly

describe('foo', function() {
  before(function() {});
  if (FOO) {
    before(function() {}); // No sibling hook
  }
});

(and also in switch cases, etc... you never know) than to allow @nhardy's example. Sometimes eslint-disable are fine. Feel free to disagree, and I don't mind if you go ahead and skip hook functions that are not direct children, but it'd be nice if we don't get a lot of false positives due to this change.

[ljharb]

I just got this when upgrading from v5 to v8 - i have a function setup() {} defined in a describe, and i'm calling it inside three neighboring its, and no-sibling-hooks warns on them.

I do not think this is an edge case; I think the rule is broken, because it's not using eslint's AST information to determine whether the hook functions are in fact globals (as opposed to file-local variables).

[lo1tuma]

@ljharb I think this is not the same issue as described here, but I fully agree with you that rules in this plugin should track where the identifiers are coming from (either globals or from an import/require). I hope to find some time in the next days to work on this.