Support JUnit5 nested test classes

[ratcashdev]

Adds support for nested Junit5 test classes (@Nested) for the following operations:

1. [MAVEN] Navigate from the Test results to a method inside a nested class - for Gradle this has already been fixed in the past.
2. [MAVEN | GRADLE] Execute a single test method from a nested test class
3. [MAVEN] Distinguish method names inside embedded classes with their enclosing type
4. [MAVEN] Test file action now executes all test methods, including those in the embedded classes

Implements most of https://github.com/apache/netbeans/issues/3975.

IMO, the outstanding item is GRADLE Test Results showing all the test methods in the UI.

[ratcashdev]

@neilcsmith-net please route this to the appropriate person. Thanks.

[ratcashdev]

@lkishalmi can you please have a look on this - has some graddle related stuff too? Thanks.

[lkishalmi]

The Gradle stuff looks fine for me.

[matthiasblaesing]

I'm missing an evaluation why it is ok to change (break?) the SPI and a documentation of the change.

These are the modules, that import that single class SingleMethod:

apisupport/apisupport.ant enterprise/j2ee.clientproject enterprise/j2ee.ejbjarproject enterprise/web.project extide/gradle groovy/groovy.support groovy/maven.groovy ide/gsf.testrunner.ui ide/projectapi java/ant.freeform java/gradle.java java/gradle.test java/java.api.common java/java.j2seproject java/java.lsp.server java/java.mx.project java/junit.ant java/junit.ant.ui java/junit.ui java/maven java/maven.junit.ui java/testng.ant java/testng.ui php/php.project

Will they break? Why not?

[ratcashdev]

IMO SingleMethod was extended, not changed. It should be backward compatible. At least I tried to do it like that. The modules you listed are using the 2-arg constructor, in which case the behavior is as it was before.

[matthiasblaesing]

This still needs to be documented and you did not address my question. Third-party modules could rely on the original definition of SingleMethod why are they not affected by your change? If I see it correctly SingleMethod instances coming from the modules you modified can't be equal to SingleMethod instances by untouched modules. This effectively breaks interoperability of the modules and thus my expectations of an SPI.

[ratcashdev]

@matthiasblaesing I am probably not qualified to provide any explanation. I know practically zero about netbeans - just tried to scratch my own itch (and I've been using these changes daily, for 2 weeks now).

In any case it seemed to me, that the instance of SingleMethod class is not stored in some long-lived collections anyway, just used as a DTO. It's put into a TestMethod instance, which is add-ed to a List... and then the whole list is discarded. But maybe I am missing something here.

[matthiasblaesing]

I see several modules from the java area in the list of affected modules. If any of these creates SingleMethod instances and receives one from one of the modules you modified, they will not be equals. Consider a Method Dummy#test in the file Dummy.java, you create:

SingleMethod(file=Dummy.java, methodName=test, enclosingType=Dummy)

but the receiving module might create this:

SingleMethod(file=Dummy.java, methodName=test, enclosingType=null)

According to the implementation of equals these two are not equal anymore.

This becomes even harder because the relationship between the filename and the enclosing type is only trivial for java, but not for other languages. PHP and Javascript for example have no hard connection between filename and embedded type, so they can't be derived from each other.

For PHP for example the methodName is the combination of the type and the type. See here:

https://github.com/apache/netbeans/blob/030c2f302a8d67fedc4ccff789fcf1acfb977986/php/php.project/src/org/netbeans/modules/php/project/ui/actions/support/TestSingleMethodSupport.java#L101

and

https://github.com/apache/netbeans/blob/030c2f302a8d67fedc4ccff789fcf1acfb977986/php/php.project/src/org/netbeans/modules/php/project/ui/actions/support/CommandUtils.java#L81-L83

[ratcashdev]

As I see it, the only occasion, where the code is using the 3-arg constructor, is when executing a single test method, and this is not 'sent' or provided to other modules. All other cases (if any) will be using the 2-arg constructor, where there's no issue with the equals. Besides, for a case like Dummy#test enclosing type will be null and therefore not breaking equals(). https://github.com/apache/netbeans/pull/3995/files#diff-fa5324f0386490b6cfc96b5331e831a50db1dc5703bc1d7b0f47636f22a43f1aR186

It will only be non-null, if the method is in a class like Dummy$EmbeddedClass#test, in which case enclosingType will be EmbeddedClass (assuming that the name of the file is Dummy.java), and then again, this is only done for JAVA/JUnit sources. So I don't have to care about PHP here.

[matthiasblaesing]

The fact that the change only affects currently unsupported cases is a good argument, that this is a safe change.

I'm still uncomfortable with the fact, that the SingleMethod class changed, while the PHP module demonstrated a way to reference qualified names without changing the definition. In Javadoc there an established syntax to refer to members - a reference could be coded as package.class.innerclass#method(ParamType1,ParamType2).

An open question: is support for overloads required (implied with the parameter based syntax).

[ratcashdev]

a reference could be coded as package.class.innerclass#method(ParamType1,ParamType2).

This is an encoded, not a structured form and therefore places more responsibility on the modules using it.

As it can be seen from the changes, both maven as well as gradle have their own way of treating embedded classes when executing test methods. If indeed we were providing an encoded form, modules would need to invent/write their own ways to parse the encoded form, tokenize them and pass it on to the engine to run it. Since I provide the structured form, working with the data is much easier, no parsing/tokenization is necessary in the modules.

[matthiasblaesing]

I'm not convinced. Anyway, if another commiter wants to merge this, I won't stand in the way.

What must be changed before a merge, is that the author information is fixed. This needs a real name in it.

[ratcashdev]

What must be changed before a merge, is that the author information is fixed. This needs a real name in it.

Happy to sign a formal CLA.

[neilcsmith-net]

@sdedic @matthiasblaesing marking as stale and removing from NB16 milestone for now. Please take a look and re-add to milestone if you think it should be merged.

[ratcashdev]

Is there anything I can do to stop this from being stale? I thought I have answered all question, or at least reacted.

[mbien]

@ratcashdev would you like to fix the small conflict in the imports so that CI can do a fresh run?

[ratcashdev]

@mbien will have a look.

[ratcashdev]

@mbien pushed