NETBEANS-1744: Support Run/Debug Focused Test Method for @Nested JUni… #1069
Conversation
…t 5 test classes in Maven projects
|
Thanks for this PR. I'm not familiar enough with this to be able to review it, how confident are you of this, what problems do you anticipate? |
|
I'm reasonably confident in this. I've checked that test single works for There shouldn't be a change in behavior for non-Maven projects, since the changes outside the Maven module should be backwards compatible. I think the only slight oddity I would expect from the new handling is that running single nested tests will work even if you don't mark the class as |
|
OK. If we merge this and issues occur or further problems arise, will you be around to work on that? |
|
Most likely yes, but I'd rather not commit to anything. If the patch turns out to cause trouble, you can always revert it. Waiting for more reviewers is also an option, I'm not under time pressure to get this in. |
| * @exception java.lang.IllegalArgumentException | ||
| * if the file or method name is {@code null} | ||
| */ | ||
| public SingleMethod(FileObject file, String binaryClassName, String methodName) { |
There was a problem hiding this comment.
Changes in API should be reflected in apichanges.xml, version of the module should be incremented in manifest.mf or project properties. New elements should have @since tag. Plus: all modules that depend on the new elements should depend on the new version of the API module.
This is a request.
|
|
||
| * @return Class name held by this object | ||
| */ | ||
| public Optional<String> getBinaryClassName() { |
There was a problem hiding this comment.
I prefer plain String that can be null. There is no Optional in this API yet, and there is no real reason to start with it as far as I can tell.
This is an opinion.
There was a problem hiding this comment.
Plain nullable String makes it more likely that users of this method will forget to null check the return value. Using Optional means that users don't have to read and remember the Javadoc (or code) of each method, in order to tell whether they need to null check.
I would prefer not to change it, but if you feel strongly that Optional is not useful, I will change this to plain String.
| @@ -91,6 +92,23 @@ public void run(CompilationController controller) throws Exception { | |||
| } | |||
| } | |||
| } | |||
| if (methodName != null) { | |||
There was a problem hiding this comment.
This code change should be covered by unit test. Especially when you cannot commit to future maintenance of the code, do others a favour and cover your code with tests.
This is an opinion.
There was a problem hiding this comment.
I agree. Are there any tests of similar code in the codebase I could use for reference? I think deep stubbing CompilationController in order to test this would make for a brittle test.
There was a problem hiding this comment.
@JaroslavTulach are there examples of similar code being tested in the codebase, without deep stubbing the CompilationController?
| @@ -144,6 +159,24 @@ public void run(CompilationController compilationController) throws Exception { | |||
| UIJavaUtils.openFile(fo2open[0], (int) line[0]); | |||
| } | |||
| } | |||
|
|
|||
| private Element findInnermostTypeElement(Element containingTypeElement, List<? extends String> targetInnerClassNames) { | |||
There was a problem hiding this comment.
This code also deserves a test. See above for justification...
This is an opinion.
There was a problem hiding this comment.
Yes. I'm not sure how to test this, since this class is glued to some static methods. I'll take a look, but if you could point to a test of similar code elsewhere, it would be helpful.
There was a problem hiding this comment.
@JaroslavTulach are there tests of similar code that could be used as a starting point for testing this code?
There was a problem hiding this comment.
No need to stub CompilationController: refactor code to have the tested part callable and save make accessors so the test can collect the computed data. I.e. make the UserTask a package-private member class, so the test can instantiate it. Create sources in test data, then process it using JavaSource.parse(). Call the tested code with (real) CompilationController instance and observe the outcome.
|
Hi, Is there any plan to integrate this patch? It's still not working at the moment and Nested test classes are a big part of our testing plateform. It would really be amazing if it could be supported properly in Netbeans! Thanks! |
|
@ixM I'm not working on this, but you are welcome to take this over. You could probably fairly easily address the comments that were made on this PR. I think it might be integrated then? |
|
Yes, the PR can be integrated, if somebody polishes it. Closing for now when @srdo gave up. Make a change and reopen whoever takes over. |
…t 5 test classes in Maven projects
The reason this doesn't work currently is that Netbeans assumes all test methods are in the top level class in a file. For
@Nestedtests, the tests are in an inner class.The changes here make Netbeans use the immediately enclosing class instead of the top level class when doing run/debug for Maven projects.
Note that this only fixes run/debug for single test methods. Running
@Nestedclasses is broken in Surefire junit-team/junit5#1343.