no-identical-names rule false positives with modules inside separate scopes/functions

[bmish]

This should be an valid case (but is currently invalid):

function sharedBehavior() {
    module('moduleName', function () {});
}
module('moduleName', function () {}); // This is erroneously considered a violation.

This should be an valid case (but is currently invalid):

export default {
    desktop() {
        module('hourly chart', function () {});
    },

    mobile() {
        module('hourly chart', function () {});
    },
};

This should be an valid case (but is currently invalid):

if (foo) {
    module('module1', function () {});
} else {
    module('module1', function () {});
}

Additional test cases that should be added:

module("name1", function() {
    function sharedBehavior() {
        module("name1", function() {}); // This is fine since it's in its own scope.
    };
    module("name1", function() {}); // This is the violation.
});

function sharedBehavior() {
    module("name1", function() {});
    module("name1", function() {}); // This is the violation.
};

[platinumazure]

I'm not sure I agree about separate scopes protecting duplicate module names. The intent of no-invalid-names is to make it so that every test failure can be easily located to the correct module chain and test name. Scopes are just a way to add confusion in the file: Yes, the tests may run in a separated manner, but they are still duplicated in the file itself and make tracing of test errors harder.

If you think this is a compelling case, I think you should either not use the rule, or we should consider enhancing the rule to take scopes into account as an opt-in behavior.

What do you think?

[Krinkle]

(Note to myself) The rule does support and allows duplicate names that are nested inside different modules, but only when there is no indirection or dynamicity in how the modules are defined (i.e. lexically in-place).

https://github.com/platinumazure/eslint-plugin-qunit/blob/v8.1.1/tests/lib/rules/no-identical-names.js#L117

I personally favour little to no indirection in test suites as this tends to decrease test confidence for me. If someone does want to enable and satisfy the rule as-is, there are two approaches that can work:

• Same as you have, but move one line from function to caller, the module() statement.
• Or, invert relationship and centralise use case by leveraging QUnit.test.each() with a data provider.

For example:

function generateSomethingTests(Klass) {
  QUnit.test('x', …);
  QUnit.test('y', …);
  QUnit.test('z', …);
}

QUnit.module('Foo', function () {
  QUnit.module('something');
  generateSomethingTests(MyApp.Foo);
});
QUnit.module('Bar', function () {
  QUnit.module('something');
  generateSomethingTests(MyApp.Bar);
});

// Foo > something > x
// Foo > something > y
// Foo > something > z
// Bar > something > x
// Bar > something > y
// Bar > something > z

QUnit.module('Something', function () {
  var dataset = { 'Foo': MyApp.Foo, 'Bar': MyApp.Bar };

  QUnit.test.each('x', dataset, function (assert, Klass) {
    …
  });

  QUnit.test.each('y', dataset, function (assert, Klass) {
    …
  });

  QUnit.test.each('z', dataset, function (assert, Klass) {
    …
  });
});
// Something > x [Foo]
// Something > x [Bar]
// Something > y [Foo]
// Something > y [Bar]
// Something > z [Foo]
// Something > z [Bar]

Both produce the same number of test cases, and give you e.g. a "Rerun" link for each one.