Bugfix/handle running parameterized tests

[TheSench]

This PR allows some forms of parameterized tests to be run directly in the test explorer (rather than having to run their non-parameterized parent). Specifically, the parameterized tests that use the form (it|describe).each(table)(name, fn) and use printf patterns in their names can now be run and matched in the results.

This is achieved by a two-fold change:

1. When generating the testNamePattern, printf placeholders will be replaced by regex expressions that will loosely match the range of possible values for that type of printf pattern (or the original pattern). We err on the side of matching too many so that we can ensure we always run the test selected.
2. When matching test results to the tree, we again use regex for the matches, as the resulting testIds may be different than the test initially chosen, due to placeholders being resolved at runtime.

Partially addresses #26.

[rossknudsen]

Hey @TheSench, I quite like your code style, especially where you create an array to project the ID using a fluent syntax.

I see what you are doing, adding a 'closing' tag to the end of the describe and test IDs. Which makes me consider something that I had been thinking about for a while and might alter your approach. the Test Explorer host expects an ID as a unique string but internally we can use anything we like for an identifier, including an object matching ID interface. Then we would only need to convert to/from a string when passing results to the adapter, or receiving IDs from the adapter. E.g.

public run(tests: string[]): Promise<void> {
  const ids: ID[] = []; // TODO convert the tests array into an array of ID objects
  return runInternal(ids);
}

private async runInternal(testIds: ID[]): Promise<void> {
   // actually run the requested tests.
}

Then we could just stringify the IDs instead of all the code that is currently being used for separators etc. We probably just need something that is slightly more deterministic than JSON.stringify since there is no guarantee on key order (and potentially array order, although, that seems strange to me). Something like this maybe. We'd still probably need some of the code you have currently above.

Alternatively, you could tackle the above in a subsequent PR if you wanted.

[rossknudsen]

PS, if you wouldn't mind having a look at #107 too. I don't have anyone to peer-review at the moment.

[TheSench]

@rossknudsen thank you for the compliment. I hadn't thought of that, but now that you mention it, it might simplify things if we just used ID's to match them up - I'd have to think on it. As much as the string-matching presented some problems, the someIdString.startsWith(someOtherIdString) pattern made some things easier too.

[TheSench]

PS, if you wouldn't mind having a look at #107 too. I don't have anyone to peer-review at the moment.

Sure. I don't have time this moment, but I'll carve out some time later this weekend to take a look.

[rossknudsen]

I hadn't thought of that, but now that you mention it, it might simplify things if we just used ID's to match them up - I'd have to think on it.

Sure, just let me know whether you want this merged as is. I'm happy for it to be merged like this and address the ID thing separately. Or if you would rather it be decided before merging this piece of work, let me know. I've reviewed it and am happy with it.

Thanks also for adding all the extra tests. It will make me feel more comfortable about taking it out of preview in the marketplace.

[TheSench]

I hadn't thought of that, but now that you mention it, it might simplify things if we just used ID's to match them up - I'd have to think on it.

Sure, just let me know whether you want this merged as is. I'm happy for it to be merged like this and address the ID thing separately. Or if you would rather it be decided before merging this piece of work, let me know. I've reviewed it and am happy with it.

Thanks also for adding all the extra tests. It will make me feel more comfortable about taking it out of preview in the marketplace.

I'm fine with leaving this PR as-is and taking a look at the JSONified-IDs in a separate PR. With the amount of time I currently have available for this type of stuff, it will probably be a week or more before I'd have these changes ready.

[botsman]

Hi guys,

In case you haven't seen yet, there is a new version of Jest editor support, which improves handling of parametrised tests.
Perhaps it will be useful for you

[rossknudsen]

Hi guys,

In case you haven't seen yet, there is a new version of Jest editor support, which improves handling of parametrised tests.
Perhaps it will be useful for you

@TheSench do you want to include the new version in this PR?

[TheSench]

Hi guys,
In case you haven't seen yet, there is a new version of Jest editor support, which improves handling of parametrised tests.
Perhaps it will be useful for you

@TheSench do you want to include the new version in this PR?

Sure, I think that makes sense.

[TheSench]

Hi guys,
In case you haven't seen yet, there is a new version of Jest editor support, which improves handling of parametrised tests.
Perhaps it will be useful for you

@TheSench do you want to include the new version in this PR?

Sure, I think that makes sense.

When I go to update the dependency, a whole bunch of other stuff gets added to package.lock because I'm using a later version of NPM that has lockfileVersion: 2. Do you mind adding that change to this PR?

[rossknudsen]

If it's a stable version of NPM then go for it. I'll update my local version.

[rossknudsen]

We'll see if the CI pipeline can handle it.

[TheSench]

If it's a stable version of NPM then go for it. I'll update my local version.

I ended up just pointing nvm at node 12 and it stopped using a new version of lockfiles.

[taschmidt]

@TheSench So if I'm running version 0.8.0 (which has this PR merged) should I not be seeing this anymore?

    test.each([
        ['video', 'mp4'],
        ['audio', 'mp3'],
        ['podcast', 'mp3'],
    ])('type %s has extension %s', async (type, extension) => {
    ...

[TheSench]

@TheSench So if I'm running version 0.8.0 (which has this PR merged) should I not be seeing this anymore?

    test.each([
        ['video', 'mp4'],
        ['audio', 'mp3'],
        ['podcast', 'mp3'],
    ])('type %s has extension %s', async (type, extension) => {
    ...

Unfortunately no - you'll still see the original entry that will run all of the variations, as that's what is discovered in the code. At the time that the extension identifies what tests exist, it doesn't know enough to populate the list of actual variations (it would have to execute the code in some cases).

What this PR added is the fact that the actual results now show up when you run that parameterized test. Before this change, the tests would run, but the extension would not identify them as being part of the tests it asked Jest to run, and it would throw them away, leaving you with only the type %s has extension %s entry and no status.

[taschmidt]

Too bad, i was always pleasantly surprised that the mocha test adapter did seem to dynamically evaluate these and accurately show all the iterations of the test in the sidebar. Which was really nice because vscode would then prompt you when you would click "Run" or "Debug" and ask you which values you wanted to use.

[TheSench]

I think that would require a large rewrite of this plugin, as it currently uses jest-editor-support to discover and run the tests, and that project does all its discovery via static analysis. There might be some ways we can improve the UI (such as possibly nesting parameterized tests underneath the generic version), although I haven't looked in a while to see how many hoops we'd have to jump through for that.

[taschmidt]

😢 I guess that's the only thing better about Mocha...