8340326: Remove references to Applet in core-libs/security tests #21096

justin-curtis-lu · 2024-09-19T22:25:25Z

Please review this PR which removes usages of Applet within the corelibs tests.

Most changes are removed comments/updated var names. The JBS issue lists more files than the ones included in this pull request, please see the comment on the JBS issue for the reason why they were not included.

The following files were removed,

test/jdk/java/util/TimeZone/DefaultTimeZoneTest.html
- Test runner is no longer an Applet, so not needed
test/jdk/java/net/Socket/SocketImplTest.java
- An old Applet test missing Jtreg tags.

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-8340326: Remove references to Applet in core-libs/security tests (Bug - P4)

Reviewers

Naoto Sato (@naotoj - Reviewer) Review applies to c90218e9
Daniel Fuchs (@dfuch - Reviewer) Review applies to 5b6bd624
Phil Race (@prrace - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21096

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-19T22:25:55Z

👋 Welcome back jlu! 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 · 2024-09-19T22:26:31Z

@justin-curtis-lu 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:

8340326: Remove references to Applet in core-libs/security tests

Reviewed-by: prr, naoto, dfuchs

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 119 new commits pushed to the master branch:

6f459af: 8340077: Open source few Checkbox tests - Set2
b6e72ff: 8339403: sun.security.ssl.StatusResponseManager.get swallows interrupt status
013250e: 8340432: Open source some MenuBar tests - Set2
de12fc7: 8339684: ResizeObserver callback interrupts smooth scrolling on Chrome
ebb4759: 8340625: Open source additional Component tests (part 3)
3ee94e0: 8341282: (fs) Move creation time fallback logic to Java layer (Linux)
f1ea57f: 8340229: Improve opening sentence of FileInputStream constructor specification
1202800: 8341006: Optimize StackMapGenerator detect frames
eb93e69: 8339979: VirtualThreadSchedulerMXBeanTest.testReduceParallelism fails intermittently
21f8ccf: 8341310: Test TestJcmdWithSideCar.java should skip ACCESS_TMP_VIA_PROC_ROOT (after JDK-8327114)
... and 109 more: https://git.openjdk.org/jdk/compare/f7bc9ba552cf913eef2131b964c48f1b4b55131c...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-09-19T22:27:05Z

@justin-curtis-lu The following labels will be automatically applied to this pull request:

core-libs
hotspot-runtime
i18n
net

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

mlbridge · 2024-09-19T22:29:57Z

Webrevs

naotoj

Removing test/jdk/java/util/TimeZone/DefaultTimeZoneTest.* looks good to me. It's a manual test that involves underlying OS settings manipulation which cannot be automated.

justin-curtis-lu · 2024-09-25T22:03:59Z

Removing test/jdk/java/util/TimeZone/DefaultTimeZoneTest.* looks good to me. It's a manual test that involves underlying OS settings manipulation which cannot be automated.

As we previously spoke offline, I did not think there was much value in preserving the test.
However, I realize it is a unique test that does validate that the runtime detects the "Automatically adjust clock for daylight saving changes" setting correctly on Windows. I preserved it and replaced the Applet overhead with PassFailJFrame in the most recent commit. Not sure if it will be ran by anyone as it is manual, but at least we can verify the setting if needed and the test history is preserved.

prrace · 2024-09-25T22:17:40Z

Not sure if it will be ran by anyone as it is manual,

I don't know about other vendors, but for Oracle, manual tests are run at least once on every release some time after RDP1 and before RDP2.

naotoj · 2024-09-25T22:35:19Z

test/jdk/java/util/TimeZone/DefaultTimeZoneTest.java

+ * @bug 4296930 5033603 7092679 8340326
+ * @summary Ensure that Java detects the platform time zone correctly, even
+ * if changed during runtime. Also ensure that the system time zone detection code
+ * detects the "Automatically adjust clock for daylight saving changes" setting


Probably taking this opportunity, changing the wording/instructions aligned with "Settings app", instead of deprecated "Control Panel" would be good.

naotoj

LGTM

naotoj · 2024-09-26T16:25:21Z

test/jdk/java/util/TimeZone/DefaultTimeZoneTest.java

-            // TimeZone.getDefault will detect the platform time zone
-            TimeZone.setDefault(null);
-            System.setProperty("user.timezone", "");
+            If the local time in the control panel and test window are always the same,


nit: "control panel" -> "Settings app"

FWIW on my WIndows 11 system both Settings and Control Panel provide this.

Yes, but they are sunsetting Control Panel.

dfuch · 2024-09-26T17:06:38Z

test/jdk/sun/net/www/ParseUtil_6380332.java: shouldn't we actually keep this test? I see that the code in ParseUtil which is tested here is still there, and I don't think we'd want to touch ParseUtil given potential compatibility issues that might arise.

justin-curtis-lu · 2024-09-26T17:20:12Z

test/jdk/sun/net/www/ParseUtil_6380332.java: shouldn't we actually keep this test? I see that the code in ParseUtil which is tested here is still there, and I don't think we'd want to touch ParseUtil given potential compatibility issues that might arise.

Hi Daniel,

You're right, I looked at that test too fast. While there are similar tests testing ParseUtil.toURI, ParseUtil_6380332.java tests the specific -1 port number case.

Restored the test, and instead updated the summary in c90218e. Thanks.

dfuch

Logging and sun/net changes look reasonable to me.

dfuch · 2024-09-30T13:10:37Z

The changes that restore the original @bug in tests look good to me

dfuch

Since Naoto previously approved before the simple @bug updates I am going to approve now. @prrace no objection?

prrace · 2024-10-03T17:56:30Z

test/jdk/java/util/TimeZone/DefaultTimeZoneTest.java

+ * detects the "Automatically adjust clock for daylight saving changes" setting
+ * correctly on Windows. This is a manual test dependent on making changes to
+ * the platform setting of the machine and thus cannot be automated.
+ * @library /java/awt/regtesthelpers


You are missing "@build PassFailJFrame".
Per https://openjdk.org/jtreg/tag-spec.html -

In general, classes in library directories are not automatically compiled as part of a compilation command explicitly naming the source files containing those classes.
A test that relies upon library classes should contain appropriate @build directives to ensure that the classes will be compiled.
It is strongly recommended that tests do not rely on the use of implicit compilation by the Java compiler. Such an approach is generally fragile,
and may lead to incomplete recompilation when a test or library code has been modified.

justin-curtis-lu · 2024-10-04T17:32:54Z

Thanks for the reviews.
/integrate

openjdk · 2024-10-04T17:34:09Z

Going to push as commit 2e5b420.
Since your change was applied there have been 137 commits pushed to the master branch:

f5f0852: 8341379: Shenandoah: Improve lock contention during cleanup
04c9c5f: 8341111: open source several AWT tests including menu shortcut tests
0dd4997: 8340555: Open source DnD tests - Set4
beb2a51: 8341377: Update VMProps.isCDSRuntimeOptionsCompatible to include Parallel and Serial GC
42f3255: 8341053: Two CDS tests fail again with -UseCompressedOops and UseSerialGC/UseParallelGC
feb6a83: 8340945: Ubsan: oopStorage.cpp:374:8: runtime error: applying non-zero offset 18446744073709551168 to null pointer
db61458: 8341298: Open source more AWT window tests
72ac72f: 8341413: Stop including osThread_os.hpp in the middle of the OSThread class
7fa2f22: 8341127: Extra call to MethodHandle::asType from memory segment var handles fails to inline
1bdd79e: 8341261: Tests assume UnlockExperimentalVMOptions is disabled by default
... and 127 more: https://git.openjdk.org/jdk/compare/f7bc9ba552cf913eef2131b964c48f1b4b55131c...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-04T17:34:16Z

@justin-curtis-lu Pushed as commit 2e5b420.

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

init

a50c197

openjdk bot changed the title ~~JDK-8340326: Remove references to Applet in core-libs/security tests~~ 8340326: Remove references to Applet in core-libs/security tests Sep 19, 2024

openjdk bot added the rfr Pull request is ready for review label Sep 19, 2024

openjdk bot added hotspot-runtime [email protected] core-libs [email protected] net [email protected] i18n [email protected] labels Sep 19, 2024

justin-curtis-lu added 2 commits September 25, 2024 09:48

Merge branch 'master' into JDK-8340326-tests-appletRemoval

ec14144

update TEST.groups with DefaultTimeZoneTest.java removal

cc86071

naotoj reviewed Sep 25, 2024

View reviewed changes

restore DefaultTimeZoneTest.java as a manual test using PassFailJFrame

6b2c060

naotoj reviewed Sep 25, 2024

View reviewed changes

review: update wording of instructions

603e40f

naotoj reviewed Sep 26, 2024

View reviewed changes

review: replace last control panel instance with Settings app

d1822f4

restore ParseUtil_6380332.java test

c90218e

dfuch reviewed Sep 26, 2024

View reviewed changes

naotoj approved these changes Sep 27, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 27, 2024

revert bugID update to tests

5b6bd62

openjdk bot removed the ready Pull request is ready to be integrated label Sep 27, 2024

dfuch approved these changes Oct 1, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 1, 2024

prrace reviewed Oct 3, 2024

View reviewed changes

reflect Phil's review: build tag for PassFailJFrame

8a06843

openjdk bot removed the ready Pull request is ready to be integrated label Oct 3, 2024

prrace approved these changes Oct 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 3, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 4, 2024

openjdk bot closed this Oct 4, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8340326: Remove references to Applet in core-libs/security tests #21096

8340326: Remove references to Applet in core-libs/security tests #21096

justin-curtis-lu commented Sep 19, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 19, 2024

openjdk bot commented Sep 19, 2024 •

edited

Loading

openjdk bot commented Sep 19, 2024

mlbridge bot commented Sep 19, 2024 •

edited

Loading

naotoj left a comment

justin-curtis-lu commented Sep 25, 2024

prrace commented Sep 25, 2024

naotoj Sep 25, 2024

naotoj left a comment

naotoj Sep 26, 2024

prrace Sep 26, 2024

naotoj Sep 26, 2024

dfuch commented Sep 26, 2024

justin-curtis-lu commented Sep 26, 2024

dfuch left a comment

dfuch commented Sep 30, 2024

dfuch left a comment

prrace Oct 3, 2024

justin-curtis-lu commented Oct 4, 2024

openjdk bot commented Oct 4, 2024

openjdk bot commented Oct 4, 2024

8340326: Remove references to Applet in core-libs/security tests #21096

8340326: Remove references to Applet in core-libs/security tests #21096

Conversation

justin-curtis-lu commented Sep 19, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 19, 2024

openjdk bot commented Sep 19, 2024 • edited Loading

openjdk bot commented Sep 19, 2024

mlbridge bot commented Sep 19, 2024 • edited Loading

Webrevs

naotoj left a comment

Choose a reason for hiding this comment

justin-curtis-lu commented Sep 25, 2024

prrace commented Sep 25, 2024

naotoj Sep 25, 2024

Choose a reason for hiding this comment

naotoj left a comment

Choose a reason for hiding this comment

naotoj Sep 26, 2024

Choose a reason for hiding this comment

prrace Sep 26, 2024

Choose a reason for hiding this comment

naotoj Sep 26, 2024

Choose a reason for hiding this comment

dfuch commented Sep 26, 2024

justin-curtis-lu commented Sep 26, 2024

dfuch left a comment

Choose a reason for hiding this comment

dfuch commented Sep 30, 2024

dfuch left a comment

Choose a reason for hiding this comment

prrace Oct 3, 2024

Choose a reason for hiding this comment

justin-curtis-lu commented Oct 4, 2024

openjdk bot commented Oct 4, 2024

openjdk bot commented Oct 4, 2024

justin-curtis-lu commented Sep 19, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 19, 2024 •

edited

Loading

mlbridge bot commented Sep 19, 2024 •

edited

Loading