On Wed, 22 Jan 2025 15:04:24 GMT, Jaikiran Pai <[email protected]> wrote:
>> Added tests against `ofInputStream()` in
>> 70387718fabbd63ec94bb2b538e29931dce1fea2 for both happy and unhappy paths.
>> The latter is interesting. The `HttpResponseInputStream::current` method
>> (used by `read()`) starts as follows:
>>
>>
>> if (closed || failed != null) {
>> throw new IOException("closed", failed);
>> }
>>
>>
>> That is, I cannot verify the failure using
>>
>>
>> assertEquals(exception.getMessage(), "body exceeds capacity: " +
>> insufficientCapacity)
>>
>>
>> anymore. Instead, I need to verify against _the cause_:
>>
>>
>> assertNotNull(exception.getCause());
>> assertEquals(exception.getCause().getMessage(), "body exceeds capacity: " +
>> insufficientCapacity);
>>
>>
>> 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.
>>
>> @jaikiran, if the current resolution (i.e.,
>> 70387718fabbd63ec94bb2b538e29931dce1fea2) addresses your concern, would you
>> mind resolving this conversation, please?
>
>> 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).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1925487415
