Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let the test.each callback access the current case key #1764

Open
vtintillier opened this issue Jun 13, 2024 · 3 comments
Open

Let the test.each callback access the current case key #1764

vtintillier opened this issue Jun 13, 2024 · 3 comments
Labels
Component: Core For module, test, hooks, and reporters. Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors.

Comments

@vtintillier
Copy link
Contributor

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
Copy link
Member

Krinkle commented Jun 13, 2024

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: #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
Copy link
Contributor Author

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 Krinkle added Type: Enhancement New idea or feature request. Component: Core For module, test, hooks, and reporters. labels Jun 15, 2024
@Krinkle
Copy link
Member

Krinkle commented Jun 19, 2024

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 as third callback argument
QUnit.test.each('example', {
    Running: true,
    Canceling: true,
    Canceled: false
}, function (assert, value, key) {
    assert.true(true);
});

// 2. Key as 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. labelKey parameter to each() method
//    Optional labelKey as second argument to each(), which produces
//    test case names as "example [Running]" instead of "example [0]"
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. New eachKeyed() method
//    Opt-in to expanding the object to key/value pairs
QUnit.test.eachKeyed('example', {
    Running: true, // translated to { key: 'Running', value: true }
    Canceling: true,
    Canceled: false
}, function (assert, { key, value }) {
    assert.true(true);
});

@Krinkle Krinkle added the Type: Meta Seek input from maintainers and contributors. label Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core For module, test, hooks, and reporters. Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors.
Development

No branches or pull requests

2 participants