Skip to content

pagination: add coverage for the untested response-close branches #206

Description

@OmarAlJarrah

Context

#202 makes each Page hold a live Response that must be closed exactly once on every path.
A few of those close branches are not exercised by the suite, so a regression that dropped the
close — or surfaced the wrong cause — would not fail the build. The aggregate coverage floor is
unlikely to catch a single lost close() line.

Sync Paginator parse-failure close has no test

Paginator.walker() closes the just-fetched response when the strategy throws
(Paginator.kt:118-123). The async mirror is tested twice (AsyncPaginatorTest: "strategy
parse failure surfaces and still closes the response", plus the subsequent-page variant), but
no sync test drives a throwing strategy — every sync test uses a real, non-throwing one.
Deleting the sync response.close() would strand a connection on every sync parse failure and
leave the build green. closeRecordingResponse already exists; add a sync test asserting the
exception propagates and the close counter is 1.

drainPage "swallow close error when the sink already failed" branch is untested

AsyncPaginator.drainPage (around AsyncPaginator.kt:436-438) keeps the sink's cause and
swallows a throwing close() when result.isDone. The two relevant tests exercise the inputs
only in isolation — a throwing sink over a clean-close client, and a clean sink over a
throwing-close response. The intersection (throwing sink and throwing close) is never
tested, so the branch that preserves the sink cause over the close IOException has no
coverage. Add a test with a throwing consumer over a throwing-close page asserting the sink
failure is the surfaced cause.

closeStagedPageQuietly swallow branch is untested on both staged-page paths

closeStagedPageQuietly (around AsyncPaginator.kt:302-312) swallows a throwing close() so
the propagating failure is not masked. Its two callers — the rejected re-dispatch path and the
external-settle drop path — are both tested only with a non-throwing closeRecordingResponse,
so the swallow branch never runs. Add staged-page tests using a throwing-close response and
assert the original cause survives: RejectedExecutionException on the rejected re-dispatch
path, and a clean cancellation on the external-settle drop path.

(lower priority) Sync Paginator.byPage early-break release isn't asserted end-to-end

PaginatorByPageTest only fully consumes the walk. The early-break-then-close release is
covered for PagedIterable.byPage() (over an in-memory fetcher) but not for
Paginator.byPage() over a live-Response client. This largely duplicates the shared
CloseablePages logic, so it is the lowest-value of the four — a small test mirroring the
PagedIterable early-break case over the stub client would close the gap.

References

#202 (#30).

Metadata

Metadata

Assignees

No one assigned

    Labels

    sdk-coresdk-core toolkittech-debtsimplification / cleanup

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions