RFR: 8375231: Refactor util/ServiceLoader tests to use JUnit [v2]

Justin Lu jlu at openjdk.org
Wed Jan 14 18:03:55 UTC 2026


On Wed, 14 Jan 2026 07:32:42 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Justin Lu has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Retain the removed whitespace in BadProvidersTest.java
>
> test/jdk/java/util/ServiceLoader/BadProvidersTest.java line 142:
> 
>> 140: 
>> 141: 
>> 142:     public static Object[][] createBadFactories() {
> 
> I assume the method source doesn't need to be public. Also it could be changed to return `Stream<List>` and that would allow the "ignore" parameter to be dropped.

Both true. Instead, I replaced the `MethodSource` with a `ValueSource` to simplify it further.

> test/jdk/java/util/ServiceLoader/BadProvidersTest.java line 173:
> 
>> 171:             // load providers and instantiate each one
>> 172:             loadProviders(mods, TEST1_MODULE).forEach(Provider::get);
>> 173:         });
> 
> The scope of the function passed to assertThrows is way too large. The code to compile the factory and copy the compiled class into the module should not be in this block. Was this tool or manual edit? Asking because we should only assert that the SL use throws ServiceConfigurationError.

This is done by the tool, presumably because when testNG uses the `expectedExceptions` attribute, it permits the exception to be thrown _anywhere_ in the test, so the conversion tool is guranteeing that the JUnit replacement has the _exact_ same behavior. I agree we should narrow down the scope though.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/29210#discussion_r2691483763
PR Review Comment: https://git.openjdk.org/jdk/pull/29210#discussion_r2691482346


More information about the core-libs-dev mailing list