fixes Rerun failed test with maven and junit5 creates the wrong command line.

[homberghp]

This fix reformats the test property passed to the MavenCommandLineExecutor by replacing occurrences of f.q.c.n.TestClass#f.q.c.n.TestClass.testMethod with f.q.c.n.TestClass#testMethod.

The location where this fix is applied might not be ideal, because the wrongly formatted test property is composed elsewhere, however inside the MavenCommandLineExecutor we are sure that it is used for maven.

Click to collapse/expand PR instructions

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

• are all your own work, and you have the right to contribute them.
• are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

PR approval and merge checklist:

1. [ ] Was this PR correctly labeled, did the right tests run? When did they run?
2. [ ] Is this PR squashed?
3. [ ] Are author name / email address correct? Are co-authors correctly listed? Do the commit messages need updates?
4. [ ] Does the PR title and description still fit after the Nth iteration? Is the description sufficient to appear in the release notes?

If this PR targets the delivery branch: don't merge. (full wiki article)

[matthiasblaesing]

I had a look at this and I think this is the wrong location to fix this. Try these two runs:

1. run the full unittests and try to invoke "Run again" from the failing test t01Max
2. open RangeTest.java and run that single test: now try "Run again" in the test results window for that run.

The first run shows the problem you are observing, while the second does not. The difference between the two can be found in the JUnit Integration for maven:

https://github.com/apache/netbeans/blob/1947efd398f35c19c524c93749b42bf63813fd94/java/maven.junit.ui/src/org/netbeans/modules/maven/junit/ui/MavenJUnitTestMethodNode.java#L72-L82

The critical section is line 73. For the first run returns io.github.jristretto.ranges.RangeTest.t01Max for testcase.getName() while the second run yields t01Max. The difference comes from here:

https://github.com/apache/netbeans/blob/1947efd398f35c19c524c93749b42bf63813fd94/java/junit/src/org/netbeans/modules/junit/api/JUnitTestcase.java#L55-L68

For the full run you get a suiteName and thus you get the method name prefixed with the class. Case 1 goes through line 67, while case 2 goes through line 64.

Looking at the NB history (Link goes to a git clone of the original mercurial history):

https://github.com/emilianbold/netbeans-releases/commit/3364af50e2d9714524b6873125dc5ab1f4c36b38

this strange behavior was introduces to handle nested tests and that does not even work correctly.

I get io.github.jristretto.ranges.SampleTest$NestedClass2.testMyMethod1 for the getName() call and io.github.jristretto.ranges.SampleTest$NestedClass2 for the getClassName() call. So you could just check in MavenJUnitTestMethodNode.java if class name is a prefix of name and if so strip it away. For the nested case more is required (check for $ and split them, but that is a different case and is already broken, so does not need to be fixed here).

[homberghp]

Thank you for your attention, and time and effort spent. I will modify the PR accordingly.

Done 20225-06-10 13:56

[homberghp]

However, this solution does not cut it. For the time being, I would keep the previous 'hacky fix'

[homberghp]

I had a look at this and I think this is the wrong location to fix this. Try these two runs:

1. run the full unittests and try to invoke "Run again" from the failing test t01Max
2. open RangeTest.java and run that single test: now try "Run again" in the test results window for that run.

The first run shows the problem you are observing, while the second does not. The difference between the two can be found in the JUnit Integration for maven:

https://github.com/apache/netbeans/blob/1947efd398f35c19c524c93749b42bf63813fd94/java/maven.junit.ui/src/org/netbeans/modules/maven/junit/ui/MavenJUnitTestMethodNode.java#L72-L82

The critical section is line 73. For the first run returns io.github.jristretto.ranges.RangeTest.t01Max for testcase.getName() while the second run yields t01Max. The difference comes from here:

https://github.com/apache/netbeans/blob/1947efd398f35c19c524c93749b42bf63813fd94/java/junit/src/org/netbeans/modules/junit/api/JUnitTestcase.java#L55-L68

For the full run you get a suiteName and thus you get the method name prefixed with the class. Case 1 goes through line 67, while case 2 goes through line 64.

Looking at the NB history (Link goes to a git clone of the original mercurial history):

emilianbold/netbeans-releases@3364af5

this strange behavior was introduces to handle nested tests and that does not even work correctly.

I get io.github.jristretto.ranges.SampleTest$NestedClass2.testMyMethod1 for the getName() call and io.github.jristretto.ranges.SampleTest$NestedClass2 for the getClassName() call. So you could just check in MavenJUnitTestMethodNode.java if class name is a prefix of name and if so strip it away. For the nested case more is required (check for $ and split them, but that is a different case and is already broken, so does not need to be fixed here).

I have tried what you suggested, but that appears not to cut it. Reverted the commits in the PR back to my 'hacky solution'

Will create a better example project.

[homberghp]

Here an improved example project. Note that the tests are intentionally broken. The correct project can be found at https://github.com/jristretto/statewalker

statewalker.zip

Run test project, then rerun failed tests. With this project the correctly built command line is: mvn -Dtest=io.github.jristretto.statewalker.ContextBaseTest#testRawContext+testSomeMethod,io.github.jristretto.statewalkerpile.ContextBaseTest#testMethod+testMethod+testMethod+testMethod+testMethod+testMethod --no-transfer-progress test

[homberghp]

In the latets version I applied matthiasblaesing to a different file. This does the trick.

[mbien]

@homberghp please don't mention issue numbers in titles or commit messages. To link a PR with an issue, all you have to do is to add:

closes #8533

to the PR message. github doc

This will link it properly, and won't spam additional links during branching etc. (also: the merge commit will already contain the PR ID - so this is already covered)

[homberghp]

will do.

[matthiasblaesing]

@homberghp a second comment: when you force push into a branch, please also add a comment. Github does not notify about force pushes and so I missed your update.