Let the test.each callback access the current case key

vtintillier commented 1 month ago

This is a feature request for QUnit.test.each. I can do a PR if this is something you think is desirable to add.

Tell us about your runtime:

QUnit version: 2.21.0
Which environment are you using? (e.g., browser, Node): browser
How are you running QUnit? (e.g., QUnit CLI, Grunt, Karma, manually in browser): Grunt and browser

What are you trying to do?

When using QUnit.test.each it would sometimes be nice to have access to the test case key.

Code that reproduces the problem: for example notice the duplication here where both the status and the test case keys are the same:

QUnit.test.each('Test jobs in progress status', {
    Running: {
        status: 'Running',
        expectedInProgress: true
    },
    Canceling: {
        status: 'Canceling',
        expectedInProgress: true
    },
    Canceled: {
        status: 'Canceled',
        expectedInProgress: false
    }
}, function(assert, { status, expectedInProgress }) {

    assert.equal(isInProgress(status), expectedInProgress);

});

What did you expect to happen?

I would expect to be able to write something like this:

QUnit.test.each('Test jobs in progress status', {
    Running: true,
    Canceling: true,
    Canceled: false
}, function(assert, expectedInProgress, status) {

    assert.equal(isInProgress(status), expectedInProgress);

});

where we get the test case key as an additional argument to the test callback.

What actually happened?

This is not possible yet, and we have to duplicate values to get nice reporting (instead of e.g. using an array where we just see integers as test case names).

Krinkle commented 1 month ago

we have to duplicate values to get nice reporting (instead of e.g. using an array where we just see integers as test case names).

I agree exposing the key would be interesting to persue. I will say there is also a separate proposal to bring nice reporting to array values: https://github.com/qunitjs/qunit/issues/1733. Perhaps you'd be interested to help with that as well?

In terms of ergonomics I imagine it'd be simpler not to have to introduce a new way to expose the current dataset key as another thing to learn, but I'm not opposed to it either if it can be done in a sufficiently simple/intuitive way. Adding more callback parameters is tricky and somewhat prone to mistakes, but that's a gut reaction. I will ponder on it :)

vtintillier commented 1 month ago

I will say there is also a separate proposal to bring nice reporting to array values: #1733. Perhaps you'd be interested to help with that as well?

sure 👍

In terms of ergonomics I imagine it'd be simpler not to have to introduce a new way to expose the current dataset key as another thing to learn

do you have something in mind?

Krinkle commented 4 weeks ago

The drawback of appending a callback parameter is that it locks us in. If we need an extra parameter for something else later, in test.each() or in test() more broadly, it creates confusion and ambiguity to reconcile. TypeScript support (and the humans using it!) also is more straight forward if the third parameter does not mean different things in different contexts. I also vaguely recall some plugins in the past appending extra parameters via a monkey-patch to QUnit.test(). That was a long time ago though, so maybe we don't need to worry about that.

Hence, I'm cautious. But, so far I've not found a compelling issue or potential thing we would need it for, so maybe we should just add this! 🙂

do you have something in mind?

I don't have a preference yet. Alternatives won't be as minimal as adding a third argument. But, here's a few ideas in case one of these jumps out as appealing.

// Status quo
QUnit.test.each('example', {
    Running: { status: 'Running', expected: true },
    Canceling: { status: 'Canceling', expected: true },
    Canceled: { status: 'Canceled', expected: false }
}, function (assert, { status, expected }) {
    assert.true(true);
});

// 1. Key in third callback argument
QUnit.test.each('example', {
    Running: true,
    Canceling: true,
    Canceled: false
}, function (assert, value, key) {
    assert.true(true);
});

// 2. Key in second callback argument
QUnit.test.each('example', {
    Running: true,
    Canceling: true,
    Canceled: false
}, function (assert, key, value) {
    assert.true(true);
});

// 3. Key in callback context
QUnit.test.each('example', {
    Running: true,
    Canceling: true,
    Canceled: false
}, function (assert, value) {
    // this.dataKey
    // this.$key
    // this.somethingElse?
    assert.true(true);
});

// 4. Request dataset naming with key as second argument to each()
// translates array data "example [0]" to object data "example [Running]"
QUnit.test.each('example', 'status', [ 
    { status: 'Running', expected: true },
    { status: 'Canceling', expected: true },
    { status: 'Canceled', expected: false }
], function (assert, { status, expected }) {
    assert.true(true);
});

// 5. Request object translation via method, eachKeyed()
QUnit.test.eachKeyed('example', {
    Running: true, // translated to { key: 'Running', value: true }
    Canceling: true,
    Canceled: false
}, function (assert, { key, value }) {
    assert.true(true);
});

qunitjs / qunit