Support JUnit5 nested test classes #3995
Conversation
|
@neilcsmith-net please route this to the appropriate person. Thanks. |
|
@lkishalmi can you please have a look on this - has some graddle related stuff too? Thanks. |
|
The Gradle stuff looks fine for me. |
|
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 apisupport/apisupport.ant Will they break? Why not? |
|
IMO |
|
This still needs to be documented and you did not address my question. Third-party modules could rely on the original definition of |
|
@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 |
|
I see several modules from the java area in the list of affected modules. If any of these creates
but the receiving module might create this:
According to the implementation of 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: and |
|
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. It will only be non-null, if the method is in a class like |
|
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 An open question: is support for overloads required (implied with the parameter based syntax). |
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. |
|
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. |
Happy to sign a formal CLA. |
java/gradle.java/src/org/netbeans/modules/gradle/java/GradleJavaTokenProvider.java
Outdated
Show resolved
Hide resolved
| public SingleMethod(FileObject file, String methodName, String enclosingType) { | ||
| super(); | ||
| if (file == null) { | ||
| throw new IllegalArgumentException("file is <null>"); |
There was a problem hiding this comment.
Consider to use org.openide.util.Parameters.notNull()
There was a problem hiding this comment.
just copied the existing code. Is NPE indeed better than the above? Or just a pure Objects.requireNotNull()...
| this.methodName = methodName; | ||
| } | ||
|
|
||
| public SingleMethod(FileObject file, String methodName, String enclosingType) { |
There was a problem hiding this comment.
Document the syntax of the enclosing type: should it be FQN, or just fragment starting from the simple name of the toplevel class in the file ... ? Note that from the other usages, it seems that the enclosingType must start with $ character. While possible, it's not a nice API ;)
There was a problem hiding this comment.
This class in just a placeholder, or container. There's no specific contract as of the format or even the usage of the enclosing type - hence no javadoc on it. I guess different languages will have different requirements.
You're right, however, that a kind of a convention is introduced by the TestClassInfoTask class in the JUnit Test UI, putting a leading $ into it (or null). Not sure if it's the right place to document a convention specific to a language (or just a test runner framework) in this classes constructor.
| if (classOnly.startsWith(fileName) && classOnly.length() > fileName.length()) { | ||
| return classOnly.substring(fileName.length()); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Note: there may be multiple toplevel classes in a file: how is the SingleMethod supposed to capture that ?
There was a problem hiding this comment.
Good question. How is it done today?
| Element element = trees.getElement(trees.getPath(compilationUnitTree, tree)); | ||
| if (element != null && element.getKind() == ElementKind.CLASS && element.getSimpleName().contentEquals(fo2open[0].getName())) { | ||
| List<? extends ExecutableElement> methodElements = ElementFilter.methodsIn(element.getEnclosedElements()); | ||
| Element classElement = getClassElement(element, desiredClassName); |
There was a problem hiding this comment.
this could eventually return another ClassElement within the compilation unit with the same simple name, i.e. in a case
class Foo {
class A {
class C {}
}
class B {
class C{}
}
}
the extractDeepestClass(B.C) seems to produce "C", and getClassElement("C") could find A.C instead of B.C ?
There was a problem hiding this comment.
Fix pushed. I guess the challenge of navigating to a specific class/method is quite common and should be solved once. Right now, a code like this is written at least 3 times (Gradle, TestNg, Maven/JUnit) so it would be great to have it unified, but I think it would be best to leave such a change it to a pure 'refactoring PR'
| // keep the embedded class in classnames and add to display name | ||
| int suiteNameLength = suite.getName().length(); | ||
| if (classname.length() > suiteNameLength && classname.startsWith(suite.getName())) { | ||
| displayName = classname.substring(suiteNameLength) + "." + methodName; |
There was a problem hiding this comment.
Q: when is this code path executed ? I have tried running the whole testsuite, and a single method within a nested class, but did not hit the branch. I am asking bcs it seems as there should be '.' or '$' at suiteNameLength-th character, so '.' would be duplicated.
There was a problem hiding this comment.
Have you tried using the @Nested JUnit5 annotation?
class MyClassTest {
@Nested
class NestedGroupTest {
@Test
public void mytest() {...}
}
}There was a problem hiding this comment.
it should look like this:
ratcashdev
requested review from
sdedic
and removed request for
dbalek,
entlicher and
matthiasblaesing
|
@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. |
|
Is there anything I can do to stop this from being stale? I thought I have answered all question, or at least reacted. |
mbien
added
Maven
|
@ratcashdev would you like to fix the small conflict in the imports so that CI can do a fresh run? |
|
@mbien will have a look. |
|
@mbien pushed |
|
Hi, this one could be a really useful addition. Any news on this? |
|
@mbien Realizing that this PR has been lying abandoned by the maintainers for 3 years, I feel uncertain to invest the time & energy to update it with regards to the conflicts (and alternative concept implemented) in If this functionality is something the maintainers do not want to have inside NetBeans, I accept it (and it will mean I will have to switch to IntelliJ). If the implementation is not something you like, feel free to suggest or come up with something better. I will also take no offense if anyone will say my implementation is a scratch-my-own itch. It may be so. After all, I have zero knowledge of the internals of NB and no confidence in refactoring major internals. Either way, either move this forward with some help & guidance, or be clear about the future of this functionality being dead. |
|
@ratcashdev I haven't encountered the nested tests yet but it would certainly be nice to have support for it. Other than that I can't promise anything. I do this in my freetime and as time permits. Once per release I try to go through some community contributed PRs which look ready and try to get some in if possible. There are only so many side quests I can accept at a time and there is typically always something else showing up. |
|
Does PR #7979 fix the nested class use-case completly or is there anything missing? |
|
Answering my own question, I tested the
@ratcashdev I raised concerns because you modified |
|
@matthiasblaesing PR updated. Smoke test on a local build works nicely. |
|
@ratcashdev lets simplify the history a bit please. A deep history with multiple merges from master makes it difficult so see what is really going on and if necessary to revert after merge if necessary. When doing that, please ensure, that all commits hold your full real name and email address (former is missing, latter is present). I will not have time to really look at this the next few days, but what I saw immediatly was the change in What is the idea here? As far as I can see this breaks the existing contract. |
ratcashdev
force-pushed
the
feature/support-nested-junit
branch
from
a3a565d to
b58c367
Compare
|
@matthiasblaesing I have rebased the history, it's clean now. I understand your reasons for asking for my real name, but I also have my own reasons (and they are not malicious) for not doing so. Please try to accept it. I am OK signing a formal cooperation agreement, if that helps, subject to that not being public. ad |
|
For the name problem: I read a bit and I think I would be ok with merging. For the code updates - I don't think this works correctly yet. I created a maven project with our sample code (adjusted method names to better see the differences): <?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<groupId>test</groupId>
<artifactId>TestNested</artifactId>
<version>1.0-SNAPSHOT</version>
<packaging>jar</packaging>
<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<maven.compiler.release>17</maven.compiler.release>
<exec.mainClass>test.testnested.TestNested</exec.mainClass>
</properties>
<dependencies>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
<version>5.11.3</version>
<scope>test</scope>
<type>jar</type>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-params</artifactId>
<version>5.11.3</version>
<scope>test</scope>
<type>jar</type>
</dependency>
</dependencies>
</project>import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
public class SampleTest {
@Test
public void testMyMethod1() {
System.out.println("write this");
}
@Nested
class NestedClass {
@Test
public void testMyMethod2() {
System.out.println("nested write 2");
}
}
@Nested
class NestedClass2 {
@Test
public void testMyMethod3() {
System.out.println("nested write 1");
}
@Test
public void testMyMethod4() {
System.out.println("nested write 3");
}
@Nested
class DoubleNestedClass3 {
@Test
public void testMyMethod5() {
System.out.println("double nested write 4");
}
@Test
public void testmyMethod6() throws Exception {
throw new Exception();
}
}
}
}When I run "Test" from the projects context menu I see:
What I would have expected:
When I choose "Test File" from the context menu in the editor window in
When I try to run a single method from the test file directly:
This fails for For Invocation: Result: For the change to |
|
@matthiasblaesing Something's off here. This is how it looks for me in a fresh build ( Executing individual tests fail though; The |
|
Pushed a fix to execute individual tests ("Run focused test method") with the correct maven arguments. However, "Run Again" and "Debug" does not work yet. |
|
The run single case works now. But the output problem is still there. Please ensure that you run the manuel test (
|
|
Did as suggested regarding the build procedure. The hash is as expected.
What exactly is the output problem? Please be more specific. as for the "Run Again" topic: |
|
I experimented a bit more today and this a race condition. Running the test setup multiple times, I sometimes get: 
and sometimes I get: 
|
|
Closing this as #8664 was finally merged. |
8 participants
Adds support for nested Junit5 test classes (
@Nested) for the following operations:Implements most of #3975.
IMO, the outstanding item is GRADLE Test Results showing all the test methods in the UI.