Fix test filtering for test parameterized using TestParameterInjector.

[alexjski]

Intellij junit plugin allows filtering tests by the method name. In case of parameterized tests, the name includes parameters as well (e.g. test[parameter]). By default, attempt to run a test with a parameter in the name would fail since a method like that cannot be found. We already special case junit.Parameterized to work around the problem.

Extend logic handling parameterized runners to support TestParameterInjector. Do not fail on tests with arguments (allowed by TestParameterInjector).

Please note that similar problem exists for other parameterization libraries. There are workarounds in some libraries (e.g. Burst), however one cannot properly filter on parameter values without change on the IDE side.

For reference: TestParameterInjector issue.

[akozlova]

Could you please create e.g. service provider and the extension in the TestParameterInjector project, so IJ doesn't depend on custom junit runner? Thanks

[alexjski]

Could you please create e.g. service provider and the extension in the TestParameterInjector project, so IJ doesn't depend on custom junit runner? Thanks

Thank you for the suggestion, but I think I may need to ask for some clarification. The dependency is there for tests only, not IJ itself. What this change is trying to achieve is to make the JUnit runner plugin recognize TestParameterInjector as one supporting parameterization. Do you have a specific idea in mind? If so, can you elaborate?

[akozlova]

Ok, I don't want IJ tests to depend on specific runner and I also don't want to have code in the listener which depends on specific runner. I suggest to have an extension in junit plugin (rt part) which would allow to recognise additional parameterised tests. Then simple plugin or even internal part of library (if it's applicable) could make the whole thing work.

Yes, far more complicated than just put this code into junit plugin but this way we would allow people to customise runners and not to put everything in the platform

[alexjski]

Ok, I don't want IJ tests to depend on specific runner and I also don't want to have code in the listener which depends on specific runner.

I am happy to delete the tests. That would remove the library dependency. I presume by the listener depending on the runner you mean the mention of it's name there?

I suggest to have an extension in junit plugin (rt part) which would allow to recognise additional parameterised tests. Then simple plugin or even internal part of library (if it's applicable) could make the whole thing work.

How would you envision that being recognized by the plugin? Do you want to introduce a new marker interface/annotation to be applied on the JUnit runners? If that's the idea, where should that new interface/annotation live? I would think it is not ideal for the test runner libraries to depend on the junit plugin of Intellij. Should we be adding that to something like https://github.com/JetBrains/java-annotations?

If you have a different idea of how to recognize the custom runners, can you please elaborate on your idea (including how they would depend on it)?

[akozlova]

I suggest to introduce service provider and load extensions in runtime if user has additional dependency on the classpath. This means that that jar may be loaded by the user explicitly, be a part of IJ plugin or be a part of the library.

When you define derive providers you need api which you would like to load. And then the provider must recognise runner classes or directly create a request for IJ runner.

[nymanjens]

Question @ Anna: Would you accept this PR with only the change to plugins/junit_rt/src/com/intellij/junit4/JUnit4TestRunnerUtil.java and none of the other changes? If I understand this correctly, the other files are just for testing the change that is mostly already tested by the existing Parameterized tests?

[akozlova]

I'd like to avoid any dependencies on custom runners. If you can ensure me that suggested plan won't work, I can reconsider but junit 5 works this way and I would rather ask you to move in that direction instead

[nymanjens]

I'd like to avoid any dependencies on custom runners.

Are you talking about this change?

runnerClass.getCanonicalName().equals("com.google.testing.junit.testparameterinjector.TestParameterInjector")

--> You consider a hard-coded string that is the canonical name of TestParameterInjector a dependency?

[akozlova]

Yes, I consider the change which refers to some custom runner as third-party dependency.

Would you include dependency on IJ internals as strings as part of your library? It would be easy to write an extension on IJ side and users won't need additional plugin or another dependency.

[nymanjens]

I think I'm starting to understand what you're saying.

So instead of this:

private static boolean isParameterizedRunner(Class<? extends Runner> runnerClass) {
  return runnerClass.getCanonicalName().equals("com.google.testing.junit.testparameterinjector.TestParameterInjector")
}

you want something like this:

// ******** Defined in new file in IntelliJ codebase ******** //
package com.intellij.junit4;

interface ParameterizedTestMetadataProvider {
  boolean isParameterizedRunner(Class<? extends Runner> runnerClass);
}

// ******** In JUnit4TestRunnerUtil.java ******** //
private static boolean isParameterizedRunner(Class<? extends Runner> runnerClass) {
  for(clazz in classpath) {
    if(clazz extends ParameterizedTestMetadataProvider) {
      if(clazz.getConstructor().newInstance().isParameterizedRunner(runnerClass)) {
        return true;
      }
    }
  }
}

// ******** In TestParameterInjector codebase ******** //
class TestParameterInjectorIntellijMetadataProvider implements ParameterizedTestMetadataProvider {
  // ... implement isParameterizedRunner ...
}

?

[alexjski]

I think I'm starting to understand what you're saying.

So instead of this:

private static boolean isParameterizedRunner(Class<? extends Runner> runnerClass) {
  return runnerClass.getCanonicalName().equals("com.google.testing.junit.testparameterinjector.TestParameterInjector")
}

you want something like this:

// ******** Defined in new file in IntelliJ codebase ******** //
package com.intellij.junit4;

interface ParameterizedTestMetadataProvider {
  boolean isParameterizedRunner(Class<? extends Runner> runnerClass);
}

// ******** In JUnit4TestRunnerUtil.java ******** //
private static boolean isParameterizedRunner(Class<? extends Runner> runnerClass) {
  for(clazz in classpath) {
    if(clazz extends ParameterizedTestMetadataProvider) {
      if(clazz.getConstructor().newInstance().isParameterizedRunner(runnerClass)) {
        return true;
      }
    }
  }
}

// ******** In TestParameterInjector codebase ******** //
class TestParameterInjectorIntellijMetadataProvider implements ParameterizedTestMetadataProvider {
  // ... implement isParameterizedRunner ...
}

?

This is kind of one of the proposals I had in my comment above -- that's a more complicated marker interface. Is that something you would accept? Another one would be annotate the runner with something:

// ******** Somewhere within the Intellij codebase -- Anna, please share where it would be ******** //
public @interface ParameterizedTestRunner {
}

// ******** In JUnit4TestRunnerUtil.java ******** //
private static boolean isParameterizedRunner(Class<? extends Runner> runnerClass) {
  return Parameterized.class.isAssignableFrom(runnerClass) || ArrayUtils.contains(runnerClass.getDeclaredAnnotations(), ParameterizedTestRunner.class);
}

// ******** In TestParameterInjector codebase ******** //
@ParameterizedTestRunner
class TestParameterInjectorIntellijMetadataProvider {
  // ... implement isParameterizedRunner ...
}

If any of these are acceptable, can you please share a location where you would accept adding such an an interface/annotation?

The reason we are asking for details is because I am more than happy to do the work which you would deem up to quality, however I would like us to agree on how that would look like before I do that to avoid any misunderstandings down the line. If there is more details you would like from the proposals, please let me know -- I will be more than happy to elaborate.

[akozlova]

Yes, exactly! I am totally fine with the "current" location:

package com.intellij.junit4;

interface ParameterizedTestMetadataProvider {
  boolean isParameterizedRunner(Class<? extends Runner> runnerClass);
}

[alexjski]

Yes, exactly! I am totally fine with the "current" location:

package com.intellij.junit4;

interface ParameterizedTestMetadataProvider {
  boolean isParameterizedRunner(Class<? extends Runner> runnerClass);
}

Great! Can you help me understand what Maven artifact that would live in (test runners will need to be able to import that)?

[nymanjens]

Great! Can you help me understand what Maven artifact that would live in (test runners will need to be able to import that)?

Friendly ping @ Anna on this question?

Also, I'd like to propose an alternative that avoids dependencies between our systems in both directions:

// ******** In JUnit4TestRunnerUtil.java ******** //
private static boolean isParameterizedRunner(Class<? extends Runner> runnerClass) {
  for (Annotation annotation : runnerClass.getAnnotations()) {
    if (annotation.annotationType().getSimpleName().equals("IntroducesTestNamesWithSquareBrackets")) {
      return true;
    }
  }
  return false;
}

// ******** In TestParameterInjector codebase ******** //
package com.google.testing.junit.testparameterinjector;

@IntroducesTestNamesWithSquareBrackets
public final class TestParameterInjector extends PluggableTestRunner {
  // ...
}

@Retention(RUNTIME)
@interface IntroducesTestNamesWithSquareBrackets {}

This has all the advantages that the provider interface has, but saves IntellIJ from exposing an interface and saves the TestParameterInjector of taking a binary dependency on an IntelliJ artifact.

What do you think?

[akozlova]

I don't know what do you want to get from me: I suggest to create IJ plugin, then you don't need a maven artefact for that but you just use ij gradle plugin.

I am also ok with the common annotation though you would need to put it in junit artefact or publish an artefact and pursue junit to use it. Otherwise it would be exactly the same as now: dependency on one custom runner which I would like to avoid.

[nymanjens]

Otherwise it would be exactly the same as now: dependency on one custom runner which I would like to avoid.

I don't think this is true. Any parameterized test framework can define their own @IntroducesTestNamesWithSquareBrackets and thus signal to IntelliJ (and potentially other IDEs) that the IDE needs to do the extra square bracket filtering logic.

There is nothing about this solution that is specific to either IntelliJ or TestParameterInjector

The other solutions you're proposing don't sound particularly appealing:

• I suggest to create IJ plugin

  This is a simple test runner, and we really don't want to ask our users to install a plugin just for filtering tests. We also don't want to maintain a plugin. I also think many users would still be annoyed by their experience because they e.g. don't know that a plugin exists.

• you would need to put it in junit artefact or publish an artefact and pursue junit to use it

  Why would JUnit be interested in this bug that only affects IntelliJ when used with test runners that use square brackets in the test names? I don't think they would accept this annotation.

[akozlova]

Anybody can use some maven artefact = nobody would ever find it and would never use it. Moreover as far as I know, JUnit 4 is not developed anymore. Why not junit 5?

[nymanjens]

Moreover as far as I know, JUnit 4 is not developed anymore. Why not junit 5?

Because TestParameterInjector is a JUnit4 runner. Almost all Google tests use JUnit4 so this isn't going to change anytime soon.

Anybody can use some maven artefact = nobody would ever find it and would never use it.

I don't understand this argument because:

• I'm not proposing to changing a Maven artefact. I'm proposing to use the String value of the simple name of an annotation as a system-agnostic API between IntelliJ and TestParameterInjector
• There are ~10 people in the world who are maintaining parameterized test libraries who could possibly be interested in this. How is it a problem that nobody would ever find this? If you want, I can send a message to all of them to let them know this exists...

To recap

Your original point was:

I don't want IJ tests to depend on specific runner and I also don't want to have code in the listener which depends on specific runner. [...] Yes, far more complicated than just put this code into junit plugin but this way we would allow people to customise runners and not to put everything in the platform

My proposal that uses annotation.annotationType().getSimpleName().equals("IntroducesTestNamesWithSquareBrackets") check all these boxes.

[akozlova]

My proposal that uses annotation.annotationType().getSimpleName().equals("IntroducesTestNamesWithSquareBrackets") check all these boxes.

You assume that all that 10 people would go and fix their libraries. I have an experience that it doesn't happen at all or at least not fast.

Ok, suppose it's done. Let's talk about the name: it should reflect that it's about junit 4 runner and parameterised tests and then about brackets. Normally, annotation names are not verbs. Naming is hard...

[nymanjens]

You assume that all that 10 people would go and fix their libraries.

They may not need to fix anything at all, right. The status quo might be working for them.

Let's talk about the name: it should reflect that it's about junit 4 runner and parameterised tests and then about brackets. Normally, annotation names are not verbs.

How about @Junit4ParameterizedRunnerIntroducingTestNamesWithSquareBrackets ?

[alexjski]

Anybody can use some maven artefact = nobody would ever find it and would never use it. Moreover as far as I know, JUnit 4 is not developed anymore. Why not junit 5?

Not to detract from choosing the best annotation name, but I wanted to point out that in a prior comment suggested adding an annotation to a Jetbrains-owned library which contains annotations to communicate facts about the code to the IDE: https://github.com/JetBrains/java-annotations -- do you think you could state your opinion about that?

The upside of that is if the annotation was imported, we could properly document what it does. If java-annotations is not a right place, please let me know if another Jetbrains-owned library would work (Jetbrains-owned since we are adding it only because of Intellij)? If none, then hard-coded names seem like an only option.

[akozlova]

I have 3 similar issues, I'll try to come up with the common solution and will write here the results

[akozlova]

The fix will be available in the first 2021.3 EAP. My commit should be available on github later today/tomorrow. Thanks

[alexjski]

The fix will be available in the first 2021.3 EAP. My commit should be available on github later today/tomorrow. Thanks

Thank you for doing that. I looked at your change but it looks like it leaves out an important aspect of the parameterized runners, namely:

@RunWith(TestParameterInjector.class)
public class Test {
  @TestParameter boolean flag;
  
  @Test
  public void test() {
    System.out.println("Hello " + flag);
  }
}

Same follows for Burst.

[akozlova]

Same follows for Burst.
?

You are welcome to create a PR with check that class has constructor with parameters. I don't know how to detect boolean fields which are supposed to be injected by the test framework though.