-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
8320712: Rewrite BadFactoryTest in pure Java #16830
Conversation
👋 Welcome back eirbjo! A progress list of the required criteria for merging this PR into |
@eirbjo this pull request can not be integrated into git checkout badfactory-java-rewritte
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
# Conflicts: # test/jdk/javax/script/JDK_8196959/BadFactoryTest.sh
@eirbjo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@eirbjo This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
Hello Eirik, what's being proposed here looks good to me. It's good that this is being migrated from a shell based test to a java based test. So if you are interested in pursuing this further, please reopen the PR. The only part that is unclear to me is the use of |
/open |
@eirbjo This pull request is now open |
Thanks! I must have thought this was required to put the service definition file on the classpath. But it isn't. I have removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@eirbjo 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:
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 237 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
@jaikiran We have not heard back from @sundararajana in the last week. It would be nice to see this low-risk change integrated. I'm sure we can respond to any post-integration feedback with a follow-up PR. Do you agree? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/integrate |
Hello Eirik @eirbjo, please give me a few more hours. I just now noticed that this test resides in tier2, so github actions job doesn't cover it. I'll trigger a CI run and once it's done successfully, you can go ahead and integrate this. |
Going to push as commit 6697160.
Your commit was automatically rebased without conflicts. |
Whoops, I just integrated after the approval from @sundararajana, seconds before this comment. What is the best action here, just have a rollback PR ready in case of failures? |
That's alright.
Typically when such failures happen, depending on the noise it creates in the CI, the change is either backed out https://openjdk.org/guide/#backing-out-a-change or a JBS issue is created to fix the issue, leaving the original change around. We don't need to do anything for now. I'll keep a watch on the test run. |
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:
BadFactoryTest.sh
has been retired and jtreg tags moved over toBadFactoryTest.java
@Test
method@library /javax/script/JDK_8196959
@summary
tag was updated since the existing summary was slightly confusing-Djava.security.manager=allow
instead of just-Djava.security.manager
ScriptEngineManager
can actually find and loadBadFactory
. Such a check would have detected the issue which inspired this rewrite, see 8319668: Fixup of jar filename typo in BadFactoryTest.sh #16585.Verified by running:
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.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16830/head:pull/16830
$ git checkout pull/16830
Update a local copy of the PR:
$ git checkout pull/16830
$ git pull https://git.openjdk.org/jdk.git pull/16830/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16830
View PR using the GUI difftool:
$ git pr show -t 16830
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16830.diff
Webrev
Link to Webrev Comment