On Wed, 24 Jan 2024 06:50:36 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Please review this PR which rewrites the BadFactoryTest to pure Java/JUnit. >> The test is currently implemented using a mix of shell script and a Java >> main method. >> >> Reviewers may notice the following changes: >> >> - The shell script file `BadFactoryTest.sh` has been retired and jtreg tags >> moved over to `BadFactoryTest.java` >> - The main method has been replaced with a JUnit `@Test` method >> - The service definition file used to be packaged into a jar file, now the >> source directory is instead directly added to the classpath using `@library >> /javax/script/JDK_8196959` >> - The `@summary` tag was updated since the existing summary was slightly >> confusing >> - To make jtreg happy, the SecurityManager-enabled invocation now runs with >> `-Djava.security.manager=allow` instead of just `-Djava.security.manager` >> - A sanity check was added to verify that `ScriptEngineManager` can actually >> find and load `BadFactory`. Such a check would have detected the issue which >> inspired this rewrite, see #16585. >> >> Verified by running: >> >> >> % make test TEST="test/jdk/javax/script/JDK_8196959" >> [..] >> TEST TOTAL PASS FAIL ERROR >> >> jtreg:test/jdk/javax/script/JDK_8196959 1 1 0 0 >> >> >> >> Note that the original issue JDK-8196959 is not available in JBS, so my >> understanding of what the original test does is based on code inspection. > > Eirik Bjørsnøs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains ten commits: > > - Remove the @library tag, the META-INF service definition file is already > on the classpath > - Merge branch 'master' into badfactory-java-rewritte > - Add the Java rewrite issue to the @bug list > - Merge branch 'master' into badfactory-java-rewritte > > # Conflicts: > # test/jdk/javax/script/JDK_8196959/BadFactoryTest.sh > - Move jtreg tags from before to after imports > - Merge branch 'master' into badfactory-java-rewritte > - Update the @summary to describe the purpose of the test, not the BadFactory > - Add comment describing the initialization of ScriptEngineManager > - Implement BadFactoryTest in pure Java Thank you for the update. This test-only change looks OK to me. Hello Sundar @sundararajana, given your past work on this test, do you have any thoughts on this change? ------------- Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16830#pullrequestreview-1840572685
