RFR: 8365776: Convert JShell tests to use JUnit instead of TestNG

[David Beaumont]

On Tue, 19 Aug 2025 10:26:14 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

> This is a conversion of JShell tests from TestNG to JUnit 5.
> 
> To make the changes more transparent, there are 4 commit in this PR:
> - https://github.com/openjdk/jdk/pull/26841/commits/c6b5d8441ad532551e626b3542431aa3f479ffc0: this one changes a name of a custom interface representing test cases, so that subsequent commits can use a simple name for `org.junit.jupiter.api.Test` (otherwise `Test` would resolve to the custom interface is subclasses of `UITesting`, and FQN would need to be used to refer to the annotation).
> - https://github.com/openjdk/jdk/pull/26841/commits/2afee7d258f646f822d0b1bc3465d5595058e3df: the bulk of changes, done automatically by: 
>  The bulk of the changes is done automatically, using this tool/script:
> https://github.com/lahodaj/netbeans/tree/19515c3e2dbc07d977ca227d933197f856121ae5/java/java.openjdk.project/junit-convert
> - https://github.com/openjdk/jdk/pull/26841/commits/8ca03af1b4d06e479cdfe7cc66186222feeaaa76: manual adjustments for things the tool does not handle (at least currently):
>     - one indent problem
>     - two uses of injected `java.lang.reflect.Method`, which needs to be replaced with `TestInfo`
>     - a different way to skip a test (normally, the tool detects and converts uses of `SkipException`, but that exception was not used on this place).
> - https://github.com/openjdk/jdk/pull/26841/commits/639da0171a593031dae6a73830bb1665b7097e73: scripted update to copyright headers, done using:
> 
> bash ./make/scripts/update_copyright_year.sh -b `git merge-base master jshell-upgrade-testng-to-junit`
> 
> 
> As part of validation of this change, this script:
> [CompareTestResults.java](https://github.com/user-attachments/files/22169483/CompareTestResults.java)
> was used to compare the results in `junit.txt` and `testng.txt` in test results before and after the change, and it didn't find a difference in test counts.

Just leaving some thoughts in passing - not a review in any meaningful sense.
No signs of any issues so far either (about 50 files examined).

test/langtools/jdk/jshell/AnalyzeSnippetTest.java line 87:

> 85:         ImportSnippet sn = (ImportSnippet) assertSnippet("import java.util.List;",
> 86:                 SubKind.SINGLE_TYPE_IMPORT_SUBKIND);
> 87:         assertEquals("List", sn.name());

Nice that this is handling the order of expected/actual properly.

test/langtools/jdk/jshell/ClassesTest.java line 70:

> 68:         assertTypeDeclSnippet(c1, "A", RECOVERABLE_NOT_DEFINED, CLASS_SUBKIND, 1, 0);
> 69:         TypeDeclSnippet c2 = classKey(assertEval("@interface A { Class<B> f() default B.class; }",
> 70:                 ste(MAIN_SNIPPET, RECOVERABLE_NOT_DEFINED, RECOVERABLE_NOT_DEFINED, false, null),

Might be worth making sure that any helper asserts are also refactored to be conceptually "(<expected>, <actual>)" just to be consistent. Obviously asserts shared with other tests not being converted (yet) could wait until everything is done, but ideally it would happen eventually.

test/langtools/jdk/jshell/CommandCompletionTest.java line 94:

> 92:     }
> 93: 
> 94:     public void assertCompletion(String code, boolean isSmart, String... expected) {

Of course this is where <expected>, <actual> hits the snag of varargs being better for a list/set of expected things. It's one of the reasons I personally prefer (<actual>, <expected>), but this code needs consistency more than personal preference :)

test/langtools/jdk/jshell/CompletionSuggestionTest.java line 759:

> 757:     }
> 758: 
> 759:     @Test(enabled = false) //TODO 8171829

Sadly the TODO got lost here. Maybe worth looking through diffs with grep as a 2nd pass for inline comments like this to see what was removed.

test/langtools/jdk/jshell/ComputeFQNsTest.java line 132:

> 130:         QualifiedNames candidates = getAnalysis().listQualifiedNames(code, code.length());
> 131: 
> 132:         assertEquals(Arrays.asList(), candidates.getNames(), "Input: " + code + ", candidates=" + candidates.getNames());

This is where my itch for test readability wants to see `List.of()` etc, I know it's not much, but every little helps.
Of course, in Truth this is even nicer as:

assertThat(candidates.getNames()).withMessage("Input: " + code).isEmpty();

-------------

PR Review: https://git.openjdk.org/jdk/pull/26841#pullrequestreview-3132392319
PR Review Comment: https://git.openjdk.org/jdk/pull/26841#discussion_r2285296296
PR Review Comment: https://git.openjdk.org/jdk/pull/26841#discussion_r2285305488
PR Review Comment: https://git.openjdk.org/jdk/pull/26841#discussion_r2285310088
PR Review Comment: https://git.openjdk.org/jdk/pull/26841#discussion_r2285313918
PR Review Comment: https://git.openjdk.org/jdk/pull/26841#discussion_r2285321182