Merge modules with same name (or treat as error)

[trentmwillis]

Currently, if you create more than one module with the same name, then we will represent these modules with different data structures.

QUnit.module( "a" ); // creates a module
QUnit.module( "a" ); // creates a different module

This is odd because the moduleId is generated based on the name of the module. Thus, two different modules can wind up with the same moduleId which seems incorrect. So, I'd like to propose allowing subsequent declarations of modules with the same name, to be merged into the first module of that name.

I do not believe this would break any existing test setup, but it would enable better reporting for a certain set of use cases. In particular, it enables you to write abstractions that group tests by test type rather than module type, but still report based on module.

In other words, it enables this:

testAllTypes( "Feature A", function someTest() {} );
testAllTypes( "Feature B", function someTest() {} );

// Generates...

QUnit.module( "Type A", function() {
  QUnit.test( "Feature A" );
} );
QUnit.module( "Type B", function() {
  QUnit.test( "Feature A" );
} );

QUnit.module( "Type A", function() {
  QUnit.test( "Feature B" );
} );
QUnit.module( "Type B", function() {
  QUnit.test( "Feature B" );
} );

[Arkni]

What will happen when someone defines hooks on both modules ?
Should QUnit only invoke the first ones (in case they exist) or the second. Or just allow them all and queue them as QUnit already do with tests ?

[trentmwillis]

Good point. Will likely need to come up with a better API to handle that factor, as I don't think any of the approaches mentioned are very intuitive.

[Krinkle]

I'd prefer we avoid module merging as this would also complicate the concept of when a module is "done" (e.g. hooks like before/after, and logging events like suiteEnd).

In fact, I'd rather propose to consider two modules with the same name an error. Selecting one from the drop down menu should after all only run one module. Otherwise the filter can be used instead.

Alternatively, we could generate our own IDs, implement &moduleId=, and have the dropdown menu values reflect those IDs (which means multiple could have the same display name).

I imagine this dilemma might come from wanting to perfectly map "test modules" to some form of component concept in the tested project - instead of merely a group of unit tests. The solution could be to have them not have the same name (e.g. "Feature A - Type B"). The kind of grouping that maps to project component structure is probably better achieved by putting the two modules in the same file, with a meaningful file name, or by placing the two files in a subdirectory with a meaningful name - not by giving them the same QUnit module name.

Alternatively, one could make use of nested modules - which landed in 1.20 (see #858 and https://api.qunitjs.com/QUnit/module).

[leobalter]

In fact, I'd rather propose to consider two modules with the same name an error.

I agree with this or just leave as it is. Merging modules with the same names may cause more confusion and might be a mess with different hooks.

[gibson042]

+1 to erroring on duplicate module name.

[platinumazure]

👍 from me for error on duplicate module name, with the caveat that I think it would have to be semver-major.

[sechel]

I use different files to define nested-modules with a common parent module and it never occurred to me that the parent module is in fact several modules with the same name.
This is because all the reporters that I have used so far (Karma reporters verbose and alike) aggregate modules by their name. The only location where this issue pops up is the module selection dropdown of QUnit.
I'm not sure how to handle this use case when there will be an error raised on modules with the same name. Any suggestions?

[Krinkle]

@sechel If this is only about the name and/or the sorting of test results, perhaps writing that part of the name in full would work better?

For the general naming of the subject itself, I think a plain string is recommended for formulating the fully-qualified name of a package or namespace. If I understand correctly, you currently have something like this:

// thing.test.js
module('foo', hooks =>
  module('Thing', hooks => {
    test('construct', …);
    test('move', …);
  });
});

// walker.test.js
module('foo', hooks =>
  module('Walker', hooks => {
    test('construct', …);
    test('move', …);
    test('walk', …);
  });
});

Is that right? If so, would the following work equaly well?

// thing.test.js
module('foo.Thing', hooks => {
  test('construct', …);
  test('move', …);
});

// walker.test.js
module('foo.Walker', hooks => {
  test('construct', …);
  test('move', …);
  test('walk', …);
});

From what I've seen so far, module nesting is mainly seen as a way to share common test code, such as beforeEach/afterEach hooks being applied to both sets of tests.

When re-defining the module in separate test files, hooks are currently not shared, only the name is shared. Hence, it seems like prefixing foo. would meet that need. But I'm happy to hear otherwise if this is used for something else as well.

---

I'm tentatively adding this to the QUnit 3.0 milestone, with the idea that we would warn for this on a 2.x release, and treat it as error in QUnit 3.0. Feedback welcome, especially if the conversation so far has not addressed the way you use these today!

[Krinkle]

I'm removing this from the 2.x milestone, but keeping it open for future consideration. The proposal has shifted from the intial version (allow duplicates and merge into existing modules somehow) to the latest, where we instead consider it a mistake and disallow this.

This would be a breaking change with a potential benefit whereby modules have unique names. I can imagine how this would benefit reporters and such, in theory, however in practice I have not seen this come up. As such I'm hesitant to pull the trigger just yet. In particular, the biggest worry I can imagine here (hooks applying too little or too much) is currently working as expected. The name is only similar in textual output. The two live independently by other means, the same way as they would after this change.

In order to unblock QUnit 3.0, I'll pull this off the milestone. Feedback on how this would benefit specific integrations or enable new use cases, is welcome and would help expedite it for a future release.