On Wed, 22 Jan 2025 15:07:44 GMT, Jaikiran Pai <[email protected]> wrote:
>>> Instead, I need to verify against the cause
>>
>> Verifying against the cause is OK. Some of these tests, including this one
>> are crafted based on what the implementation does. As long as we don't make
>> the tests way too tied into the implementation (I believe this one doesn't).
>>
>>> This made me think, maybe HttpResponseInputStream::current should have
>>> started as follows:
>>>
>>> if (failed) throw new IOException(failed);
>>> if (closed) throw new IOException("closed");
>>>
>>> So that the exception message of failed will be preserved.
>>
>> In its current form in mainline, the exception message is still preserved in
>> the underlying cause. Having said that, I think what you suggest for the
>> `HttpResponseInputStream::current` implementation make sense since the
>> current message says "closed" even if it may not be. If we do considering
>> doing that change, we should do it in a separate issue.
>
>> @jaikiran, if the current resolution (i.e.,
>> https://github.com/openjdk/jdk/commit/70387718fabbd63ec94bb2b538e29931dce1fea2)
>> addresses your concern, would you mind resolving this conversation, please?
>
> The change you did does address my review. I however don't see a way to
> resolve this conversation in the GitHub UI (I anyway don't resolve
> conversations because I find it hard to follow the discussion in these PRs
> since GitHub hides resolved conversations).
Got it, resolving the conversation – will discuss my
`HttpResponseInputStream::current` proposal with the rest of the crew first.
<hr/>
I generally navigate through unresolved conversations using the `Conversations`
drop down:
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1925567770
