Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Wed, 22 Nov 2023 17:57:22 GMT, Gaurav Chaudhari wrote: >> The file test/jdk/javax/script/JDK_8196959/BadFactoryTest.sh contains a >> typo. When running without security manager, the test references >> 'badfactoty.jar' instead of 'badfactory.jar'. This change addresses this by >> correcting the jar name. > > Gaurav Chaudhari has refreshed the contents of this pull request, and > previous commits have been removed. The incremental views will show > differences compared to the previous content of the PR. The pull request > contains one new commit since the last revision: > > 8319668: Fixup of jar filename typo in BadFactoryTest.sh Hello Gaurav, the change looks good to me. I ran the test/jdk/javax/script tests with this change in our CI and they continue to pass. I'll go ahead and sponsor this. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16585#pullrequestreview-1752668321
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Mon, 27 Nov 2023 16:45:12 GMT, Gaurav Chaudhari wrote: > You can go ahead and contribute the PR in that case. Thanks! You'll need a sponsor to integrate this PR, then I'll go ahead and follow up with #16830 - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1829551620
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Sat, 25 Nov 2023 16:45:07 GMT, Eirik Bjorsnos wrote: >>> Reviewer time is a scarce resource. It would be wasteful to spend review >>> cycles on getting a fix of this `.sh` test integrated now and then >>> immediately follow up with a delete in the rewrite PR. >>> >>> I think we should handle this change in one PR, not two. If you prefer to >>> keep the history of your `.sh` fix documented, we can repurpose this PR, >>> otherwise we start fresh with a new PR for the rewrite. >> >> The change here is trivial, it's okay to integrate and use a separate >> issue/PR to replace the shell test. I can't tell from the bug report if this >> was just noticed in passing or it actually failed (maybe on an IBM port). > >> The change here is trivial, it's okay to integrate and use a separate >> issue/PR to replace the shell test. > > Fair point, I filed https://bugs.openjdk.org/browse/JDK-8320712 to track the > rewrite. > > @Deigue, would you like to contribute a PR for the rewrite as well? If not, > I'll be happy to take on this task. Thanks @eirbjo, I dont directly have committer status for creating openjdk bugs myself. You can go ahead and contribute the PR in that case. - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1828215999
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Fri, 24 Nov 2023 16:46:05 GMT, Alan Bateman wrote: > The change here is trivial, it's okay to integrate and use a separate > issue/PR to replace the shell test. Fair point, I filed https://bugs.openjdk.org/browse/JDK-8320712 to track the rewrite. @Deigue, would you like to contribute a PR for the rewrite as well? If not, I'll be happy to take on this task. - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1826374488
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Wed, 22 Nov 2023 18:28:30 GMT, Eirik Bjorsnos wrote: > Reviewer time is a scarce resource. It would be wasteful to spend review > cycles on getting a fix of this `.sh` test integrated now and then > immediately follow up with a delete in the rewrite PR. > > I think we should handle this change in one PR, not two. If you prefer to > keep the history of your `.sh` fix documented, we can repurpose this PR, > otherwise we start fresh with a new PR for the rewrite. The change here is trivial, it's okay to integrate and use a separate issue/PR to replace the shell test. I can't tell from the bug report if this was just noticed in passing or it actually failed (maybe on an IBM port). - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1825910051
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Wed, 22 Nov 2023 18:01:21 GMT, Eirik Bjorsnos wrote: >> @eirbjo Yes, as you noticed, the jar file does matter. And the reason I >> suspected it wasn't noticed was because it was in the scenario of running >> without security manager, So may that part of the code wasn't being actively >> executed. >> >> As for the rewrite, it does look good. But would it make more sense to bring >> this change as a separate PR having a own openjdk bug issue # designated to >> reworking of BadFactoryTest.sh for tracking purposes? > >> As for the rewrite, it does look good. But would it make more sense to bring >> this change as a separate PR having a own openjdk bug issue # designated to >> reworking of BadFactoryTest.sh for tracking purposes? > > We have two options: > > - Withdraw this PR, submit a new PR for the rewrite > - Repurpose this PR for the rewrite, updating JBS and PR titles accordingly > > In either case, I can help updating/creating JBS isssues as required. > > I have a slight preference for repurposing, but it's up to you. What do you > prefer? > @eirbjo If it makes sense from perspective of commiters/reviewers, I'll be > happy to integrate the changes here and tweak so that it looks good. Reviewer time is a scarce resource. It would be wasteful to spend review cycles on getting a fix of this `.sh` test integrated now and then immediately follow up with a delete in the rewrite PR. I think we should handle this change in one PR, not two. If you prefer to keep the history of your `.sh` fix documented, we can repurpose this PR, otherwise we start fresh with a new PR for the rewrite. > I also wonder if we have BadFactoryTest.java instead of BadFactoryTest.sh , > will certain references elsewhere need to be adjusted accordingly, in case > these jtreg tests are being referenced in a certain way somewhere. jtreg will pick this up automatically. I did a search of `BadFactoryTest` across the code base, and it is not referenced outside the file itself. - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1823271462
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Wed, 22 Nov 2023 18:01:21 GMT, Eirik Bjorsnos wrote: >> @eirbjo Yes, as you noticed, the jar file does matter. And the reason I >> suspected it wasn't noticed was because it was in the scenario of running >> without security manager, So may that part of the code wasn't being actively >> executed. >> >> As for the rewrite, it does look good. But would it make more sense to bring >> this change as a separate PR having a own openjdk bug issue # designated to >> reworking of BadFactoryTest.sh for tracking purposes? > >> As for the rewrite, it does look good. But would it make more sense to bring >> this change as a separate PR having a own openjdk bug issue # designated to >> reworking of BadFactoryTest.sh for tracking purposes? > > We have two options: > > - Withdraw this PR, submit a new PR for the rewrite > - Repurpose this PR for the rewrite, updating JBS and PR titles accordingly > > In either case, I can help updating/creating JBS isssues as required. > > I have a slight preference for repurposing, but it's up to you. What do you > prefer? @eirbjo If it makes sense from perspective of commiters/reviewers, I'll be happy to integrate the changes here and tweak so that it looks good. I also wonder if we have BadFactoryTest.java instead of BadFactoryTest.sh , will certain references elsewhere need to be adjusted accordingly, in case these jtreg tests are being referenced in a certain way somewhere. - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1823259262
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Wed, 22 Nov 2023 17:53:33 GMT, Gaurav Chaudhari wrote: > As for the rewrite, it does look good. But would it make more sense to bring > this change as a separate PR having a own openjdk bug issue # designated to > reworking of BadFactoryTest.sh for tracking purposes? We have two options: - Withdraw this PR, submit a new PR for the rewrite - Repurpose this PR for the rewrite, updating JBS and PR titles accordingly In either case, I can help updating/creating JBS isssues as required. I have a slight preference for repurposing, but it's up to you. What do you prefer? - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1823238308
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
> The file test/jdk/javax/script/JDK_8196959/BadFactoryTest.sh contains a typo. > When running without security manager, the test references 'badfactoty.jar' > instead of 'badfactory.jar'. This change addresses this by correcting the jar > name. Gaurav Chaudhari has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8319668: Fixup of jar filename typo in BadFactoryTest.sh - Changes: - all: https://git.openjdk.org/jdk/pull/16585/files - new: https://git.openjdk.org/jdk/pull/16585/files/5cd537cb..0780ad32 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16585=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16585=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16585.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16585/head:pull/16585 PR: https://git.openjdk.org/jdk/pull/16585
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Wed, 22 Nov 2023 11:32:13 GMT, Eirik Bjorsnos wrote:
>>> Looks okay. This test is begging to be re-written in Java, maybe some day.
>>>
>>> I assume the copyright header will be updated before this change is
>>> integrated.
>>
>> Hi @AlanBateman, do I have to update the copyright year to 2023 manually and
>> amend the commit before `/integrate` ?
>
> @Deigue
>
> I was able to update `BadFactoryTest.java` to work as a pure Java test by
> adding the following jtreg header before the imports:
>
>
> /*
> * @test
> * @bug 8196959
> * @summary BadFactory that results in NPE being thrown from
> ScriptEngineManager
> * @library /javax/script/JDK_8196959
> * @build BadFactory BadFactoryTest
> * @run main/othervm BadFactoryTest
> * @run main/othervm -Djava.security.manager=allow BadFactoryTest
> */
>
>
> I think we should also add a sanity check that the BadFactory
> ScriptEngineFactory is actually loaded. (This validation would have caught
> the jar file typo):
>
>
> ScriptEngineManager m = new ScriptEngineManager();
> m.getEngineFactories().stream()
> .filter(c -> c.getClass() == BadFactory.class)
> .findAny()
> .orElseThrow(() -> new IllegalStateException("Expected to find
> BadFactory"));
>
>
> Would you like to include these changes in your PR? If not, I'm happy to
> create a separate PR to convert this test to Java.
@eirbjo Yes, as you noticed, the jar file does matter. And the reason I
suspected it wasn't noticed was because it was in the scenario of running
without security manager, So may that part of the code wasn't being actively
executed.
As for the rewrite, it does look good. But would it make more sense to bring
this change as a separate PR having a own openjdk bug issue # designated to
reworking of BadFactoryTest.sh for tracking purposes?
-
PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1823228953
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
On Tue, 21 Nov 2023 19:59:16 GMT, Gaurav Chaudhari wrote:
>> Looks okay. This test is begging to be re-written in Java, maybe some day.
>>
>> I assume the copyright header will be updated before this change is
>> integrated.
>
>> Looks okay. This test is begging to be re-written in Java, maybe some day.
>>
>> I assume the copyright header will be updated before this change is
>> integrated.
>
> Hi @AlanBateman, do I have to update the copyright year to 2023 manually and
> amend the commit before `/integrate` ?
@Deigue
I was able to update `BadFactoryTest.java` to work as a pure Java test by
adding the following jtreg header before the imports:
/*
* @test
* @bug 8196959
* @summary BadFactory that results in NPE being thrown from ScriptEngineManager
* @library /javax/script/JDK_8196959
* @build BadFactory BadFactoryTest
* @run main/othervm BadFactoryTest
* @run main/othervm -Djava.security.manager=allow BadFactoryTest
*/
I think we should add some code in the main method which validates that the
BadFactory ScriptEngineFactory is loaded. (This validation would have caught
the jar file typo):
ScriptEngineManager m = new ScriptEngineManager();
m.getEngineFactories().stream()
.filter(c -> c.getClass() == BadFactory.class)
.findAny()
.orElseThrow(() -> new IllegalStateException("Expected to find
BadFactory"));
Would you like to include these changes in your PR? If not, I'm happy to create
a separate PR to convert this test to Java.
-
PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1822599642
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
On Tue, 21 Nov 2023 21:16:41 GMT, Eirik Bjorsnos wrote:
> So it's not clear to me why this test uses a jar file at all, it does not
> seem necessary.
On a closer look, `${TESTCLASSES}` contains the compiled java classes, but not
the service definition file
`META-INF/services/javax.script.ScriptEngineFactory`. That probably explains
the jar file.
Adding `${TESTSRC}` to the class paths would incude the service definition file:
-classpath "${TESTCLASSES}${PS}${TESTSRC}"
But perhaps not a big improvement. Might be better to spend efforts re-writing
this in Java, as suggested by Alan.
-
PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1822463159
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
On Thu, 9 Nov 2023 16:49:41 GMT, Gaurav Chaudhari wrote:
> The file test/jdk/javax/script/JDK_8196959/BadFactoryTest.sh contains a typo.
> When running without security manager, the test references 'badfactoty.jar'
> instead of 'badfactory.jar'. This change addresses this by correcting the jar
> name.
The typo you found here reveals that the test runs fine without having the jar
file on the classpath. All the necessary class files already seem to be in
`${TESTCLASSES}`
So it's not clear to me why this test uses a jar file at all, it does not seem
necessary.
Perhaps you could clean this up in this PR? That would include:
- Removing the code which creates the JAR
- Updating the two `-classpath` options to just `"${TESTCLASSES}"`
If you feel this is out of scope for this PR, I'm fine with that as well.
-
PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1821701137
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
On Tue, 21 Nov 2023 16:36:26 GMT, Alan Bateman wrote: >> The file test/jdk/javax/script/JDK_8196959/BadFactoryTest.sh contains a >> typo. When running without security manager, the test references >> 'badfactoty.jar' instead of 'badfactory.jar'. This change addresses this by >> correcting the jar name. > > Looks okay. This test is begging to be re-written in Java, maybe some day. > > I assume the copyright header will be updated before this change is > integrated. > Hi @AlanBateman, do I have to update the copyright year to 2023 manually and > amend the commit before `/integrate` ? @Deigue Not sure if this was a question on the formatting of the copyright header, but the year should not simply be updated to `2023`, but instead to the range `2018, 2023`. So in your case, the first line should read: # Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved. Once you push that commit, a reviewer can approve the PR and you can `/integrate` - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1821612336
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
On Tue, 21 Nov 2023 16:36:26 GMT, Alan Bateman wrote: > Looks okay. This test is begging to be re-written in Java, maybe some day. > > I assume the copyright header will be updated before this change is > integrated. Hi @AlanBateman, do I have to update the copyright year to 2023 manually and amend the commit before `/integrate` ? - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1821594845
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
On Thu, 9 Nov 2023 16:49:41 GMT, Gaurav Chaudhari wrote: > The file test/jdk/javax/script/JDK_8196959/BadFactoryTest.sh contains a typo. > When running without security manager, the test references 'badfactoty.jar' > instead of 'badfactory.jar'. This change addresses this by correcting the jar > name. Looks okay. This test is begging to be re-written in Java, maybe some day. I assume the copyright header will be updated before this change is integrated. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16585#pullrequestreview-1742482324
