RFR: 8310277: jdk/javadoc/doclet/testMethodCommentAlgorithm/TestMethodCommentsAlgorithm.java fails with IllegalStateException

[Pavel Rappo]

On Fri, 5 Jan 2024 21:45:41 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>> This PR improves diagnostic output and behaviour of the `TestMethodCommentsAlgorithm` test in environments that do not meet that test's expectations.
>> 
>> The test assumes that a correct source file for `java.lang.Object` is available nearby. While the test verified that assumption, it didn't verify it deep enough. The issue described in the linked bug report seems to be that a directory that looks like a source directory does not contain that source file.
>> 
>> The solution is to check the assumptions more thoroughly. Note that if the assumptions aren't met, the test will be skipped, but it will not fail. Some tools display a skipped test as **passed**, which could be misleading. If I were the original bug reporter, I'd investigate why the source file for `java.lang.Object` is missing.
>> 
>> As for exception messages, I tried my best to make them helpful. That said, test exception messages are not user-level error messages. The stacktrace of an exception is supposed to be analysed in conjunction with the source that threw that exception.
>
> test/langtools/jdk/javadoc/doclet/testMethodCommentAlgorithm/TestMethodCommentsAlgorithm.java line 402:
> 
>> 400:             start = Path.of(testSrc).toAbsolutePath();
>> 401:         } catch (InvalidPathException | IOError e) {
>> 402:             throw new SkippedException("Couldn't make sense of '" + testSrc + "'", e);
> 
> As a general stylistic convention, if you don't know much about the exception being caught or how the "rethrown" exception will be handled, I like to include the nested-exception message inr the rethrown message, as well as giving it in the cause.
> 
> So, for example,
> 
> throw new SkippedException("Couldn't make sense of '" + testSrc + "': " + e, e);
> 
> 
> (note the `: " + e`)

Firstly, this is not an API but a test. So we know how that exception is going to be handled. Secondly, I thought that when one translates an exception, one does at most one of these:

 - includes the exception's message in the rethrown exception message
 - sets the exception as a cause for the rethrown exception

That is, one does either of those, maybe none, but never both. Duplication makes an exception harder to read and defeats the purpose of chaining.

That said, I skimmed through the source and noticed that "including a message and chaining" is used; for whatever reason, in java.base it is used in the security area.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17280#discussion_r1444515689