JDK-8262899: TestRedirectLinks fails #3137

jonathan-gibbons · 2021-03-22T22:26:08Z

Please review a trivial change to fix this test when run behind a firewall with a proxy set.

Previously, the test used InetAddress.getLocalHost which returns an IP address for the current host. It runs a temporary local web server on that address, but since the address is typically not on the nonProxyHosts list, the attempted access to the web server goes to the proxy, and fails.

The fix is to explicitly use 127.0.0.1, which should never go to a proxy, if one is set.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8262899: TestRedirectLinks fails

Reviewers

Pavel Rappo (@pavelrappo - Reviewer) ⚠️ Review applies to 6901c1c

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3137/head:pull/3137
$ git checkout pull/3137

Update a local copy of the PR:
$ git checkout pull/3137
$ git pull https://git.openjdk.java.net/jdk pull/3137/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3137

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3137.diff

bridgekeeper · 2021-03-22T22:26:24Z

👋 Welcome back jjg! 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 · 2021-03-22T22:27:27Z

@jonathan-gibbons The following label will be automatically applied to this pull request:

javadoc

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.

mlbridge · 2021-03-22T22:30:45Z

Webrevs

pavelrappo · 2021-03-22T22:59:20Z

A good place to look for network idioms would be test/jdk/java/net. To reliably set up both client and server, consider using InetAddress.getLoopbackAddress().

* Use InetAddress.getLoopbackAddress * Suppress irrelevant doclint warnings * Check if testURL host is resolveable * Report duration if test connection fails (Duration is ~75 seconds on local machine.)

pavelrappo · 2021-03-29T15:03:03Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Extern.java

@@ -385,7 +385,7 @@ private boolean link(String url, String elemlisturl, Reporter reporter, boolean
        private static final long serialVersionUID = 0;

        Fault(String msg, Exception cause) {
-            super(msg, cause);
+            super(msg + (cause == null ? "" : " (" + cause + ")"), cause);


Although this message concatenation captures the immediate cause of the Fault, it doesn't capture those deeper exceptions that might be present in the chain. Moreover, such concatenation is required in all places where the cause is to be preserved.

A better way to do it would be to derive diagnostic information from all the exceptions in the chain on the handling site (i.e. wherever this exception is caught). Later we should consider improving jdk.javadoc.doclet.Reporter.print instead of introducing this concatenation.

Maybe. There's a tension between investing effort in an increasingly rare occurrence, and the pain of investigating the occurrence when it does arise.

pavelrappo · 2021-03-29T15:04:11Z

test/langtools/jdk/javadoc/doclet/testLinkOption/TestRedirectLinks.java

@@ -84,9 +87,31 @@ public static void main(String... args) throws Exception {
     */
    @Test
    public void testRedirects() throws Exception {
-        // first, test to see if access to external URLs is available
+        // This test relies on access to an external resource, which may or may not be
+        // reliably available, depending on the shost system configuration and other


Typo: shost

pavelrappo · 2021-03-29T15:05:14Z

test/langtools/jdk/javadoc/doclet/testLinkOption/TestRedirectLinks.java

-                        "--module", "mA,mB")
+                        "--module", "mA,mB",
+                        "-Xdoclint:none")
                .run()
                .writeAll();

        javadoc("-d", libApi.toString(),
                "--module-source-path", libSrc.toString(),
-                "--module", "mA,mB" );
+                "--module", "mA,mB",
+                "-Xdoclint:none" );


Why did you add -Xdoclint:none?

Because doclint generated spurious irrelevant warnings in the output that were an unnecessary distraction. The alternative would have been to sprinkle dummy comments throughout the code.

pavelrappo · 2021-03-29T15:51:51Z

test/langtools/jdk/javadoc/doclet/testLinkOption/TestRedirectLinks.java

+            return;
+        }
+
+        // test to see if access to external URLs is available, and that the URL uses a redirect
+
        URL testURL = new URL("http://docs.oracle.com/en/java/javase/11/docs/api/element-list");
+        String testURLHost = testURL.getHost();
+        try {
+            InetAddress testAddr = InetAddress.getByName(testURLHost);
+            out.println("Found " + testURLHost + ": " + testAddr);
+        } catch (UnknownHostException e) {
+            out.println("Setup failed (" + testURLHost + " not found); this test skipped");
+            return;


L98 and L110: can we use jtreg.SkippedException instead? That way the results of the test test run might look more clearly.

No, because that means the entire text was skipped; here we just want to skip one @Test method.

It's rather unfortunate that a setup that uses @javadoc.tester.JavadocTester.Test works differently than that of @org.testng.annotations.Test.

openjdk · 2021-03-29T16:03:37Z

@jonathan-gibbons 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:

8262899: TestRedirectLinks fails

Reviewed-by: prappo

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:

963f1fc: 8264309: JFR: Improve .jfc parser
364cce1: 8264332: Use the blessed modifier order in jdk.charsets
fbbd98b: 8264029: Replace uses of StringBuffer with StringBuilder in java.base
019080e: 8264268: Don't use oop types for derived pointers
3516c26: 8263591: Two C2 compiler phases with the name "after matching"
3caea47: 8262739: String inflation C2 intrinsic prevents insertion of anti-dependencies
19a6ac4: 8264142: Remove TRAPS/THREAD parameters for verifier related functions
aefc156: 8264179: [TESTBUG] Some compiler tests fail when running without C2
30b4b17: 8264334: Use the blessed modifier order in jdk.jpackage
8ee9a05: 8259623: JfrTypeSet::_subsystem_callback is left dangling after use
... and 109 more: https://git.openjdk.java.net/jdk/compare/f62b1008ed136dcbe7ee17191cc9b77ca2b70334...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.

jonathan-gibbons · 2021-03-29T16:12:03Z

/integrate

openjdk · 2021-03-29T16:13:29Z

@jonathan-gibbons Since your change was applied there have been 119 commits pushed to the master branch:

963f1fc: 8264309: JFR: Improve .jfc parser
364cce1: 8264332: Use the blessed modifier order in jdk.charsets
fbbd98b: 8264029: Replace uses of StringBuffer with StringBuilder in java.base
019080e: 8264268: Don't use oop types for derived pointers
3516c26: 8263591: Two C2 compiler phases with the name "after matching"
3caea47: 8262739: String inflation C2 intrinsic prevents insertion of anti-dependencies
19a6ac4: 8264142: Remove TRAPS/THREAD parameters for verifier related functions
aefc156: 8264179: [TESTBUG] Some compiler tests fail when running without C2
30b4b17: 8264334: Use the blessed modifier order in jdk.jpackage
8ee9a05: 8259623: JfrTypeSet::_subsystem_callback is left dangling after use
... and 109 more: https://git.openjdk.java.net/jdk/compare/f62b1008ed136dcbe7ee17191cc9b77ca2b70334...master

Your commit was automatically rebased without conflicts.

Pushed as commit f17ea9e.

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

JDK-8262899: TestRedirectLinks fails

dd006db

openjdk bot added the rfr Pull request is ready for review label Mar 22, 2021

openjdk bot added the javadoc javadoc-dev@openjdk.org label Mar 22, 2021

jonathan-gibbons added 3 commits March 23, 2021 09:04

Address review comments:

19d7168

* Use InetAddress.getLoopbackAddress * Suppress irrelevant doclint warnings * Check if testURL host is resolveable * Report duration if test connection fails (Duration is ~75 seconds on local machine.)

Extern: Include details of specific exception when access to URL fails.

89f5442

Disable problem test case by default, unless enabled by system property

6901c1c

pavelrappo approved these changes Mar 29, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 29, 2021

Fix typo in comment

8502ced

openjdk bot closed this Mar 29, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 29, 2021

jonathan-gibbons deleted the redirect-links branch March 29, 2021 16:13

lahodaj mentioned this pull request Apr 1, 2021

8262900: ToolBasicTest fails to access HTTP server it starts #3301

Closed

3 tasks

JDK-8262899: TestRedirectLinks fails #3137

JDK-8262899: TestRedirectLinks fails #3137

Uh oh!

Conversation

jonathan-gibbons commented Mar 22, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 22, 2021

Uh oh!

openjdk bot commented Mar 22, 2021

Uh oh!

mlbridge bot commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

pavelrappo commented Mar 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathan-gibbons commented Mar 29, 2021

Uh oh!

openjdk bot commented Mar 29, 2021

Uh oh!

Uh oh!

jonathan-gibbons commented Mar 22, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 22, 2021 •

edited

Loading

openjdk bot commented Mar 29, 2021 •

edited

Loading