On Mon, 29 Mar 2021 15:03:03 GMT, Pavel Rappo <[email protected]> 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
