Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8319668: Fixup of jar filename typo in BadFactoryTest.sh #16585

Closed
wants to merge 1 commit into from

Conversation

Deigue
Copy link
Contributor

@Deigue Deigue commented Nov 9, 2023

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.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8319668: Fixup of jar filename typo in BadFactoryTest.sh (Bug - P5)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16585/head:pull/16585
$ git checkout pull/16585

Update a local copy of the PR:
$ git checkout pull/16585
$ git pull https://git.openjdk.org/jdk.git pull/16585/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16585

View PR using the GUI difftool:
$ git pr show -t 16585

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16585.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 9, 2023

👋 Welcome back Deigue! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 9, 2023
@openjdk
Copy link

openjdk bot commented Nov 9, 2023

@Deigue The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Nov 9, 2023
@mlbridge
Copy link

mlbridge bot commented Nov 9, 2023

Webrevs

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@openjdk
Copy link

openjdk bot commented Nov 21, 2023

@Deigue This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8319668: Fixup of jar filename typo in BadFactoryTest.sh

Reviewed-by: alanb, jpai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 211 new commits pushed to the master branch:

  • 4bcda60: 8319713: Parallel: Remove PSAdaptiveSizePolicy::should_full_GC
  • 99f870c: 8320781: Fix whitespace in j.l.Double and j.u.z.ZipInputStream @snippets
  • a5ccd3b: 8267532: C2: Profile and prune untaken exception handlers
  • 464dc3d: 8319633: runtime/posixSig/TestPosixSig.java intermittent timeouts on UNIX
  • efc3922: 8319048: Monitor deflation unlink phase prolongs time to safepoint
  • debf0ec: 8313355: javax/management/remote/mandatory/notif/ListenerScaleTest.java failed with "Exception: Failed: ratio=792.2791601423487"
  • 20aae3c: 8320533: Adjust capstone integration for v6 changes
  • 0678253: 8320602: Lock contention in SchemaDVFactory.getInstance()
  • f1a24f6: 8318599: HttpURLConnection cache issues leading to crashes in JGSS w/ native GSS introduced by 8303809
  • 7848ed7: 8301856: Generated .spec file for RPM installers uninstalls desktop launcher on update
  • ... and 201 more: https://git.openjdk.org/jdk/compare/68110b7a82ae82e2485aec23aba5406d2a5c0327...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@AlanBateman, @jaikiran) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 21, 2023
@Deigue
Copy link
Contributor Author

Deigue commented Nov 21, 2023

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 ?

@eirbjo
Copy link
Contributor

eirbjo commented Nov 21, 2023

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

@eirbjo
Copy link
Contributor

eirbjo commented Nov 21, 2023

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.

@eirbjo
Copy link
Contributor

eirbjo commented Nov 22, 2023

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.

@eirbjo
Copy link
Contributor

eirbjo commented Nov 22, 2023

@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.

@openjdk
Copy link

openjdk bot commented Nov 22, 2023

@Deigue Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@Deigue
Copy link
Contributor Author

Deigue commented Nov 22, 2023

@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?

@eirbjo
Copy link
Contributor

eirbjo commented Nov 22, 2023

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?

@Deigue
Copy link
Contributor Author

Deigue commented Nov 22, 2023

@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.

@eirbjo
Copy link
Contributor

eirbjo commented Nov 22, 2023

@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.

@AlanBateman
Copy link
Contributor

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).

@eirbjo
Copy link
Contributor

eirbjo commented Nov 25, 2023

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.

@Deigue
Copy link
Contributor Author

Deigue commented Nov 27, 2023

Thanks @eirbjo, I dont directly have committer status for creating openjdk bugs myself. You can go ahead and contribute the PR in that case.

@Deigue
Copy link
Contributor Author

Deigue commented Nov 27, 2023

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 27, 2023
@openjdk
Copy link

openjdk bot commented Nov 27, 2023

@Deigue
Your change (at version 0780ad3) is now ready to be sponsored by a Committer.

@eirbjo
Copy link
Contributor

eirbjo commented Nov 28, 2023

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

Copy link
Member

@jaikiran jaikiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jaikiran
Copy link
Member

jaikiran commented Nov 28, 2023

/sponsor

@openjdk
Copy link

openjdk bot commented Nov 28, 2023

Going to push as commit 63ad868.
Since your change was applied there have been 211 commits pushed to the master branch:

  • 4bcda60: 8319713: Parallel: Remove PSAdaptiveSizePolicy::should_full_GC
  • 99f870c: 8320781: Fix whitespace in j.l.Double and j.u.z.ZipInputStream @snippets
  • a5ccd3b: 8267532: C2: Profile and prune untaken exception handlers
  • 464dc3d: 8319633: runtime/posixSig/TestPosixSig.java intermittent timeouts on UNIX
  • efc3922: 8319048: Monitor deflation unlink phase prolongs time to safepoint
  • debf0ec: 8313355: javax/management/remote/mandatory/notif/ListenerScaleTest.java failed with "Exception: Failed: ratio=792.2791601423487"
  • 20aae3c: 8320533: Adjust capstone integration for v6 changes
  • 0678253: 8320602: Lock contention in SchemaDVFactory.getInstance()
  • f1a24f6: 8318599: HttpURLConnection cache issues leading to crashes in JGSS w/ native GSS introduced by 8303809
  • 7848ed7: 8301856: Generated .spec file for RPM installers uninstalls desktop launcher on update
  • ... and 201 more: https://git.openjdk.org/jdk/compare/68110b7a82ae82e2485aec23aba5406d2a5c0327...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 28, 2023
@openjdk openjdk bot closed this Nov 28, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Nov 28, 2023
@openjdk
Copy link

openjdk bot commented Nov 28, 2023

@jaikiran @Deigue Pushed as commit 63ad868.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants