Support when clause for test launch configurations

[ReubenFrankel]

• Implement feature
• Add tests

---

Closes #1627

[ReubenFrankel]

@microsoft-github-policy-service agree

[jdneo]

Hi @ReubenFrankel,

Thank you for your contribution!

I have one concern about comparing with ID. Since that a kind of internal property and might be changed in the future.

It would be better to compare with something that users are aware of and cleared defined in some specs. Maybe like, a fully qualified name?

[ReubenFrankel]

Hi @ReubenFrankel,

Thank you for your contribution!

I have one concern about comparing with ID. Since that a kind of internal property and might be changed in the future.

It would be better to compare with something that users are aware of and cleared defined in some specs. Maybe like, a fully qualified name?

As far as I can see TestItem.id does currently contain the fully-qualified name of the test item, but if that's subject to change then I agree that it would need to be resolved elsewhere.

[ReubenFrankel]

@jdneo How's this? ac7888a#diff-f624d2810b538e481c3a1cd552f8808c287f2ca8c481c00b9b2f19219681b125

[jdneo]

Thank you @ReubenFrankel for quick response! I will take a look soon.

[jdneo]

Some questions:

1. Looks like the current implementation focus on include. While in the original issue, there are some use cases like !xxx. I guess you want to use ! to represent exclusion. Is that request still valid?
2. About the setting description. Since fully qualified name is a clearly defined concept for package and /type. then what about other level? Like project and method?

It will become complex for methods because JUnit 5 and TestNG allows methods to override. Below is how the extension constructs the fullName for methods:

vscode-java-test/java-extension/com.microsoft.java.test.plugin/src/main/java/com/microsoft/java/test/plugin/util/TestItemUtils.java

Line 43 in 22a11be

public static String parseFullName(IJavaElement element, TestLevel level, TestKind kind) {

For JUnit5 and TestNG, the name of the test method contains the parameter list, while for JUnit 4, it's only the name of the method.

I checked the document of Surefire Plugin, looks like it does not support methods filtering?

[ReubenFrankel]

1. Looks like the current implementation focus on include. While in the original issue, there are some use cases like !xxx. I guess you want to use ! to represent exclusion. Is that request still valid?

My personal use-case for this feature only really requires inclusion (i.e. I only want a specific configuration to be available when I'm running tests from a specific class). I'm sure there is a case for exclusion, in which case it might make more sense to implement something like:

{
    "filters": {
        "pattern": {
            "include": ...
            "exclude": ...
         }
    }
}

The reason I chose regex in the end here is:

1. I'm not sure of the full Surefire plugin spec for test filtering, beyond what I use currently (hence why I also don't know about it not supporting method filtering)
2. It seemed like a heavy lift to support the full Surefire spec or design a custom filtering expression spec
3. Regex is flexible and more widely known
4. You don't need a deep understanding of regex to use pattern and can write an expression that matches a class, method or method of a class without regex syntax (as long as the structure of fully-qualfiied names for test items is known):

{
    "filters": {
        "pattern": "TestClass"
    }
}

{
    "filters": {
        "pattern": "testMethod"
    }
}

{
    "filters": {
        "pattern": "TestClass#testMethod"
    }
}

2. About the setting description. Since fully qualified name is a clearly defined concept for package and /type. then what about other level? Like project and method?

A fully-qualified method name takes the pattern com.package.test.TestClass#testMethod, as defined here. I'm not sure about project... To me, it feels like an edge case but I don't know how many people have configurations that they want to use across multiple projects in the same workspace.

It will become complex for methods because JUnit 5 and TestNG allows methods to override.

I don't think this changes anything? Overridden methods will have the same name and signature, so just the class name changes, which is easy enough to match:

{
    "filters": {
        "pattern": "(TestClass|AnotherTestClass)#testMethod"
    }
}

For JUnit5 and TestNG, the name of the test method contains the parameter list, while for JUnit 4, it's only the name of the method.

JUnit4

{
    "filters": {
        "pattern": "TestClass#testMethod"
    }
}

JUnit5/TestNG

{
    "filters": {
        "pattern": "TestClass#testMethod\(String testArgument\)"
    }
}

[jdneo]

Thank you @ReubenFrankel. All of your comments make sense to me.

Then let's discuss some details about the implementation.

1. Should we go with

"pattern": {
   "include": [...]
   "exclude": [...]
}

or

"pattern": ["...", "!..."]

Considering the tag filtering already adopts the second style, we can adopt it for the pattern filter to make all filter settings align. WDYT?

2. For project filtering: After thinking it again, I now think filtering on project level may not be a real requirement in the real world. And user can just simply click the run test button at the project level in the testing explorer. So we can just simply declare project filtering is not supported.

3. For method filtering, for now things like (TestClass|AnotherTestClass)#testMethod should be good enough. Let's not mention the parameter list usage like TestClass#testMethod\(String testArgument\) in the description, since that list might be changed because of the bug: JUnit 5 ParameterizedTest results not properly flagged for complex arguments #1603

4. One last concern is about the name pattern. Actually, I quite like your input that this name hides the implementation detail. One thing I'm imagine is that, what if in the future, a user comes and says let's support glob pattern filtering. If that happens, what kind of change should be made to the current schema?

[ReubenFrankel]

"pattern": ["...", "!..."]

I'm not sure this can work with regex, since it will just treat the ! as a literal character. To work with that, it would have to be a simple string includes/excludes substring operation, which having just thought about it might be fine (regex could very well be overkill here):

pattern.startsWith('!') ? !fullName.includes(pattern.slice(1)) : fullName.includes(pattern)

I take it this means you want to support multiple patterns for a single configuration, which I hadn't considered but also makes sense.

4. One last concern is about the name pattern. Actually, I quite like your input that this name hides the implementation detail. One thing I'm imagine is that, what if in the future, a user comes and says let's support glob pattern filtering. If that happens, what kind of change should be made to the current schema?

pattern doesn't really make sense any more if this becomes a substring match. What about name (i.e. filters.name)?

How do you imagine a glob pattern working for a fully-qualified name? I'm not sure that makes sense since its not a filename or path. If you mean like a wildcard character, then it's sort of heading towards the Surefire plugin or custom implementation.

[jdneo]

I'm not sure this can work with regex, since it will just treat the ! as a literal character.

So, if the first character of the pattern is !, we slice it and treat the left part as the pattern for exclusion.

pattern doesn't really make sense any more if this becomes a substring match.

Personally, I think using regular expression is a good idea. Because IDEA also support that, and the name of it is exactly pattrn: https://www.jetbrains.com/help/idea/run-debug-configuration-junit.html

How do you imagine a glob pattern working for a fully-qualified name?

It's not for fully qualified name, but for the file names. Because in the Surefire Plugin's document, I see usages like:

<configuration>
    <excludes>
        <exclude>**/TestCircle.java</exclude>
        <exclude>**/TestSquare.java</exclude>
    </excludes>
</configuration>

[ReubenFrankel]

So, if the first character of the pattern is !, we slice it and treat the left part as the pattern for exclusion.

Ok, that can work. Regardless, it just needs documenting well.

It's not for fully qualified name, but for the file names.

That to me sounds like a separate feature to filter by file path then, in which case glob patterns could be supported of course. Not sure it's strictly related to this PR, although it does raise the question of when/why would a user need to filter by file path if they can filter by pattern already? Something to think about...

[jdneo]

although it does raise the question of when/why would a user need to filter by file path if they can filter by pattern already?

Good ask. To me it looks like pattern is enough. (Guess that's why IDEA only supports that)

I don't know why the Surefire plugin supports both. But since there is a reference from IDEA, it makes me feel safer to move forward.

To summary:

• let's use pattern as the setting name
• It's a regular expression match. If the value starts with !, it means exclusion.
• The target to match is the fully qualified name. Project level is not taken into consideration.

Sorry for having such a long discussion back and forth. Thanks for the patience. 👍

[ReubenFrankel]

• It's a regular expression match. If the value starts with !, it means exclusion.

To clarify terminology here, is this more like inversion since we are dealing with a pattern, which can both include and exclude (without a ! prefix) test items depending on the match result? For example, it would be possible for a user to specify some identifier that doesn't exist in a pattern prefixed with ! to have it match all test items (just demonstrating behaviour - not sure why you would want to do this):

 {
    "filters": {
        "pattern": "!thisdoesntexist"
    }
}

Sorry for having such a long discussion back and forth. Thanks for the patience. 👍

No worries! 🙂 I find this type of design discussion interesting!

[ReubenFrankel]

By the way, did you want to support multiple patterns for this feature? If so, I have a couple more implementation questions:

• Rename pattern to patterns?
• Support both string and string[] in config?
• Every pattern of a configuration must match every test item to be considered a candidate (current implementation is pattern must match every test item)?

I don't personally have a use-case for multiple patterns at the moment, especially since a you can achieve simple conditional matching in a single regex pattern already:

{
    "filters": {
        "pattern": "(TestClass|AnotherTestClass)"
    }
}

I do see it being useful in a couple of situations - for example, match every test in a class apart from one (although this might be possible still in a single pattern with a negative lookahead, albeit not the easiest expression to write):

{
    "filters": {
        "patterns": [
            "TestClass",
            "!TestClass#testMethod"
        ]
    }
}

[jdneo]

Rename pattern to patterns?
Support both string and string[] in config?

Let use patterns and make it a string[]. If user only has one pattern, just simply use an array with one element: [".*FooTest"].

Every pattern of a configuration must match every test item to be considered a candidate (current implementation is pattern must match every test item)?

Instead of every, let use any. I checked the IDEA's behavior, for example, if user writes .*Validator.*||.*PetController.*, IDEA will run tests from ValidatorTests and PetControllerTests.

is this more like inversion since we are dealing with a pattern, which can both include and exclude (without a ! prefix) test items depending on the match result?
this might be possible still in a single pattern with a negative lookahead

Thank you for mentioning this! Negative lookahead can used for exclusion. And I noticed that this is exactly what IDEA does. So let's not bother using ! for exclusion. It's just a regular expression that user can define inclusion or exclusion (Negative lookahead) on their own.

[ReubenFrankel]

Instead of every, let use any. I checked the IDEA's behavior, for example, if user writes .*Validator.*||.*PetController.*, IDEA will run tests from ValidatorTests and PetControllerTests.

If a user runs tests for a package from the test explorer view, doesn't it make sense to make the configuration available only if the pattern satisfies all test items (when running a package, each test item is a class)? That's what I meant by "every" - you could still define a single pattern that matches multiple items by itself (like in your example).

[jdneo]

I think things should work in the way that:

User selects the configuration, the configuration filter the tests to execute.

Not the selected tests determine which configurations should appear.

[ReubenFrankel]

I think things should work in the way that:

User selects the configuration, the configuration filter the tests to execute.

Not the selected tests determine which configurations should appear.

That's not what I was intending this PR to address. From the original issue #1627:

Is there a way to control what configurations are available to select when running tests in a specific package, class or method?

For the class CustomRunner, I only want the "Custom runner" configurations to show.

[jdneo]

Ah, sorry I totally misunderstood the intension of the feature request.

It's to filter the configurations to be displayed, not the tests to be executed.

Since both IDEA and Surefire plugin has the capability to configure the pattern filter for test execution, how can we make sure users are aware the purpose of this setting? (At least, it confused me 😢)

What I'm thinking right now is to put it into a new key, not the key filter. For example:

java.test.config: {
    ...
    when: {
        matchPatterns: [...]
    }
}

Or just simply when: "..." without children.

One question is: Currently there is only one condition - pattern match to determine if the configuration will be displayed or not. In the future, if one more condition appear, how to specify if it's an and or or combination among multiple conditions.

An answer could be referencing how VS Code handles such case: https://code.visualstudio.com/api/references/when-clause-contexts#match-operator. We can use something like when: "testItems =~ /MyTest/":

• testItems is a context key representing the selected test items.
• =~ means it's a pattern match.
• /MyTest/ is the regular expression.

[ReubenFrankel]

Ah, sorry I totally misunderstood the intension of the feature request.

It's to filter the configurations to be displayed, not the tests to be executed.

Yeah, I probably could have explained better, sorry - I thought we were on the same page! 😅

Since both IDEA and Surefire plugin has the capability to configure the pattern filter for test execution, how can we make sure users are aware the purpose of this setting? (At least, it confused me 😢)

What I'm thinking right now is to put it into a new key, not the key filter. For example:

Yeah, I think you're right here. It is confusing that filters would be filtering both configurations and tests. It should be one or the other.

Or just simply when: "..." without children.

I like that this could follow the VS Code when clause convention. We would have to parse the expression ourselves though, unless there's an API for that already...

java.test.config: {
    ...
    when: {
        matchPatterns: [...]
    }
}

I do like how explicit this is vs the when clause, but agreed that it could get complicated as soon as another condition is added.

[jdneo]

I do like how explicit this is vs the when clause, but agreed that it could get complicated as soon as another condition is added.

Let's go for the vscode when clause approach. WDYT?

[ReubenFrankel]

Yep, let's do it. 😄

[jdneo]

Thank you @ReubenFrankel, some feedbacks added.

[ReubenFrankel]

@jdneo I have been working on a utility class to evaluate a when-clause, since there is no equivalent public VS Code API (that I could find). Are you interested in that as part of this feature? Obviously, it will need some unit tests (which I did start writing) and a little more manual testing.

[jdneo]

@jdneo I have been working on a utility class to evaluate a when-clause, since there is no equivalent public VS Code API (that I could find). Are you interested in that as part of this feature? Obviously, it will need some unit tests (which I did start writing) and a little more manual testing.

Yeah, I like that! Thank you for the effort!

[jdneo]

Overall looks good, added some new comments.

[ReubenFrankel]

I guess we can link off to the VS Code reference for the when-clause syntax now? The only features not supported by this implementation are the in and not in operators. These would be easy enough to add but they're probably not necessary at the moment, since the only context key available for configurations currently is testItem.

[jdneo]

Appended a commit to add chinese description and did some simplification.

I prefer to progressively adding more description on demand (if users are confused).

So Let wait and see if we need to link to the VS Code reference for the when-clause syntax.

[jdneo]

LGTM!

I'll wait if you have more comments on the appended commit. If not, I'll merge it.

Merry Christmas! 🎉

[ReubenFrankel]

Last thing would maybe be to remove the bit of the description about the only supported clause (=~) and just mention the only available context key (testItem)? Other than that, I'm happy if you're happy!

Merry Christmas! 😁🎄

[jdneo]

Last thing would maybe be to remove the bit of the description about the only supported clause (=~) and just mention the only available context key (testItem)?

I tend to keep as what it is since testItem =~ xxx is the only pattern we support to users (though the implementation can do further)

[jdneo]

Merged. Thank you @ReubenFrankel for your contribution.