Core: Add test.each.

[ventuno]

WIP implementation of #1568.

[smcclure15]

I'm really liking this; both with the client-facing API design and it's clean implementation, IMHO. Thanks for your patience in iterating.

I agree the exact naming can be left as a follow up; this looks like a solid foundational piece to build up the other support that was discussed. Next, allow objects with field names? Then support "flat" (or idk, "primitive") arrays for convenience? I feel like those should be very near-term though separate follow ups, just to round out the feature.

[ventuno]

Thanks @smcclure15, I just added support of 1D arrays. Let's tackle "objects with field names" as a fast follow up as I would like to discuss a few examples in #1568 first before working on it. @Krinkle FYI.

[smcclure15]

I am in favor of this change; still want to bash it interactively for edge cases, but the design/implementation looks solid.
I defer to @Krinkle as lead for the go/no-go on this.

[ventuno]

@Krinkle any chance you might take a look at this?

[Krinkle]

Thanks @ventuno and @smcclure15, this is looking good.

My main concerns at this point are about making sure the ergonomics hold up in relation to these two things:

1. Use of only, skip and todo.
2. (In the near future) use of module context parameter (ref Add module-contextual 'test' function #978), e.g. the "q" object or otherwise.

By adding each as a property of test I think these become a bit less intuitive, so I'd like us to explore alternatives explicitly as well. This isn't a hard-no, I'd just like to make sure we think about the options a bit more.

It could be a sibling of test (e.g. testEach, testProvide, etc.). We did something like this in the past with QUnit.asyncTest. But, we also have since phased that out in favour of Promise support directly in QUnit.test, and otherwise via assert.async(). This has the benefit of being in the right place in the hierarchy, I think.

Another way would be to try to squish it into the test function's signature directly. This might not be so infeasible, and is also something we've done before. There used to be a middle parameter of QUnit.test. This was removed in 2.0 in favour of assert.expect(). This is sufficiently far in the past that we could very well make use of this again, I think. I don't know whether "less is more" holds up here, though. Some amount of explicitness could be good, to have at least one word or name to identify the feature with, and to discover it prominently in the API.

So the options would be:

1. QUnit.test.each( 'name', { data }, ( assert, data ) => {

This feels a bit out of place in that it isn't a mode of "test" like the other three properties. On the other hand, it is the least disruptive to the API, but placing it in the only "test"-related corner of the API that has precedent for expansion.

2. QUnit.test( 'name', { data }, ( assert, data ) => {

This is the leanest option and is also least disruptive. It retains the logic that the two main methods are "module" and "test" which together do everything. The two methods both have various aliases of themselves as properties which invoke them in special modes.

The downside I see with this is that it might be a bit too lean for less experienced developers, and leaves any form of name for the feature to documentation and social constructs, which also make it harder to search for help. There's also potential for difficult linting / typing when the callback accepts variadic parameters in some cases but not others. For ESLint, I'm not worried about this as it should be trivial to detect the presence of a middle parameter and allow extra arguments based on that. For TypeScript, it has support for overloads, so that seems fine.

3. QUnit.each( 'name', { data }, ( assert, data ) => {

This one feels structurally correct to me, in that it's not a method that defines a test. Rather, it's a new primitive between "module" and "test" that defines tests on your behalf. The downside is that it breaks the single-object model we have currently, which would make nesting difficult (ref #978).

It basically has to be either squished into the "test" method, or be a property of it.

4. QUnit.testEach( 'name', { data }, ( assert, data ) => {

Same as "3" but more verbosely named. I was pondering with this, thinking it might sort or look better in the API for discovery and in IDEs. But, would probably be worse long-term, because testFoo and test.foo look alike, and we would have both in our API. It's the sort of thing that might make a library feel complicated.

---

So, yeah, I think test.each and test are good both options. After I reminding myself to think of the test function as not just merely having three aliases under it, but actually being "the" primary function object, it feels less bad to put each under there. But, I could also see us going with test if others convince me it's not a bad choice. What do y'all think? How would it be if we made it part of the test signature?

[ventuno]

@Krinkle: updated the name to append the index prefixed by # (see screenshot). I personally like Java's DataProviders style that use []. In this case it would be particularly fitting because [] would fit both array indexes and property names when the input is an object.

For example:

test.each('my test', [1, 2], function(assert, v) {
  assert.true(v > 0);
});

Current output with array indexes:

my test #0
my test #1

Suggested output:

my test [0]
my test [1]

Suggested output with property names:

test.each('my test', { foo: 1, bar: 2 }, function(assert, v) {
  assert.true(v > 0);
});
...
my test [foo]
my test [bar]

[Krinkle]

@ventuno Sounds good, let's go with [key] then. I've not seen that before so it does feel a little odd, but it's as good as anything I think.

Actually, using # is troublesome for us since we're moving toward embracing TAP more (both for cross-browser testing, and for CLI) where # is generally used for adding additional metadata to a test result line, so using # could mess that up.

[ventuno]

Thanks @Krinkle, just updated it. See attached screenshot.

[Krinkle]

Thanks!

I might do a follow-up to add some spacing in the default theme between [1] and (1). And perhaps in QUnit 3 we can come up with something further integrated that might be a bigger visual difference. But, as a new feature adding to what we have, I think this makes sense.

[ventuno]

@Krinkle that sounds good to me. Or we can also address this as part of my next PR to support objects as inputs. I also wanted to get your comments on #1568 (comment) so that I know in which direction to start.

[Krinkle]

Set this to "unreleased" for now.

[ventuno]

Done. Is this going to be updated automatically with a new release?

[Krinkle]

Not at the moment. I try to remember them, or grep for it, during a release and fill them in as part of the release prep commit. While our release cadence is increasing through past and this year, I don't expect features to accumulate. So, for now, my view of the Is It Worth The Time? table says that it's not worth automating.

Having said that, it seems like a simple thing to automate and so I wouldn't be worried about it complicating our maintenance work. If you feel compelled to give it a try (perhaps as a way to familiarize with the release process), it would likely take the form of an additional step in build/prep-release.js.

[ventuno]

Thanks. Let me look into that after #1614.

[Krinkle]

I'll land this tomorrow after one final closer look. From glancing it over now, I think maybe the added tests aren't actually being run by either test-on-node, nor the grunt/browser test. I may've missed it, but it seems like it's not wired up. If that's indeed the case, I suggest the main each test be added to the main list of tests (via test/index.html and the list of test-on-node tests), and then the special one that has to be its own file (each-only) kept as-is but make sure it is in the list of tests that the grunt/browser task iterates over, if it isn't already included by other means.

[ventuno]

Thanks @Krinkle, take your time. I added test/each.html and test/each-only.html to Gruntfile.js and when I run npm test I see both tests in the output (see attached screenshot).
I also see the same output in CI test run. I may be missing something, just let me know and I'll make the necessary updates.

[Krinkle]

I like this abstraction a lot, did you mean to make more use of it? I went ahead and did so as part of #1620.

[ventuno]

This actually came from @smcclure15's recommendation. Glad it's useful!