On Wed, 14 Jan 2026 07:32:42 GMT, Alan Bateman <[email protected]> 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
