RFR: JDK-8262899: TestRedirectLinks fails [v4]

[Jonathan Gibbons]

On Mon, 29 Mar 2021 15:03:03 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Disable problem test case by default, unless enabled by system property
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Extern.java line 388:
> 
>> 386: 
>> 387:         Fault(String msg, Exception cause) {
>> 388:             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.

> test/langtools/jdk/javadoc/doclet/testLinkOption/TestRedirectLinks.java line 91:
> 
>> 89:     public void testRedirects() throws Exception {
>> 90:         // This test relies on access to an external resource, which may or may not be
>> 91:         // reliably available, depending on the shost system configuration and other
> 
> Typo: shost

Will fix.

> test/langtools/jdk/javadoc/doclet/testLinkOption/TestRedirectLinks.java line 110:
> 
>> 108:         } catch (UnknownHostException e) {
>> 109:             out.println("Setup failed (" + testURLHost + " not found); this test skipped");
>> 110:             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.

> test/langtools/jdk/javadoc/doclet/testLinkOption/TestRedirectLinks.java line 196:
> 
>> 194:                 "--module-source-path", libSrc.toString(),
>> 195:                 "--module", "mA,mB",
>> 196:                 "-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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3137