On Fri, 25 Oct 2024 18:08:16 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Sean Mullan has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 150 commits: >> >> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 >> - Merge >> - Update @summary to replace "if the right permission is granted" can be >> replaced with "package java.lang is open to unnamed module". >> - Remove println about Security Manager. >> - Remove unused static variable NEW_PROXY_IN_PKG. >> - Remove static variable `DEFAULT_POLICY` and unused imports. >> - Remove hasSM() method and code that calls it, and remove comment about >> running test manually with SM. >> - clientlibs: import order >> - warning-string >> - java/net/httpclient/websocket/security/WSURLPermissionTest.java: >> integrated review feedback in renamed WSSanityTest.java >> - ... and 140 more: https://git.openjdk.org/jdk/compare/f7a61fce...cb50dfde > > test/jdk/javax/imageio/spi/AppletContextTest/IIOPluginTest.java line 42: > >> 40: } catch (ServiceConfigurationError sce) { >> 41: System.out.println("Expected ServiceConfigurationError \n" + >> sce); >> 42: System.out.println("Test Passed"); > > Previously, `ServiceConfigurationError` wasn't caught and, as far as I > understand, wasn't expected; if it were thrown, the test would fail. Why is > the logic different now? When SM is removed, test fails with java.util.ServiceConfigurationError: javax.imageio.spi.ImageReaderSpi:Provider BadReaderPluginSpi not found. This is the expected behavior without SM. When SM is present this error is swallowed because otherwise a erroneous plugin could cause a VM-wide problem. Now that SM is removed, the test (IIOPluginTest.java) has been updated to expect ServiceConfigurationError. AFAIK, there isn't another test which checks this code path hence it has been repurposed and retained. > test/jdk/javax/sound/midi/Soundbanks/GetSoundBankSecurityException/GetSoundBankSecurityException.java > line 1: > >> 1: /* > > I believe this test becomes irrelevant without `SecurityManager`. > > The summary of the test states, “`MidiSystem.getSoundbank()` throws > unexpected `SecurityException`” which couldn't happen if there's no security > manager. Also see [JDK-8312535](https://bugs.openjdk.org/browse/JDK-8312535). Right the JBS is about SM & SecurityException, but the test was repurposed to check if InvalidMidiDataException is thrown and to test this case for code coverage (when it was initially reviewed). I can update the test summary accordingly - **"Check if MidiSystem.getSoundbank() throws InvalidMidiDataException when provided with invalid soundbank data"** @azuev-java Your thoughts? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817371084 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817373529
