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

[Jan Lahoda]

On Tue, 19 Aug 2025 13:39:14 GMT, David Beaumont <duke 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.
>
> 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 :)

Here it is input and then expected values, not really actual value and the expected value. So the inconsistency is not that big.

I am afraid that if we move expected to front here, there will be a lot of readability lost. For me, at least. Currently (with TestNG), the code is consistent and readable. If, in order to convert to JUnit, I would have to sacrifice readability to keep consistency, then I guess I would start to question if the conversion has a point. I.e. the conversion would need to bring a benefit big enough to compensate for the loss of readability here.

> 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();

I think I am relatively opposed to mix different types of changes, esp. if there are biggish. Also, automatic `Arrays.asList()` -> `List.of()` is somewhat troublesome, as the semantics is different.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26841#discussion_r2285600560
PR Review Comment: https://git.openjdk.org/jdk/pull/26841#discussion_r2285569963