Skip to content

Stub every page in the listing resume test - #871

Merged
laughingman7743 merged 5 commits into
masterfrom
fix/842-listing-resume-test-stub
Oct 1, 2026
Merged

laughingman7743 merged 5 commits into
masterfrom
fix/842-listing-resume-test-stub

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

WHAT

Rewrite TestCursor::test_listing_resumes_after_a_failed_glue_request so every ListTableMetadata request is served from three synthetic pages (tokens "1", "2") instead of the real Athena API.
The second request still raises an injected ThrottlingException, and Glue is still the unreachable client, so the test exercises the same path: throttle, Glue fallback fails, the Athena listing resumes with the throttled page.
The assertions are now exact: the names in page order, the request tokens [None, "1", "1", "2"], and that Glue was tried (not glue.reachable).

Add TestCursor::test_listing_reads_every_page, which lists the session schema from the real Athena API four entries per page and checks that each table and view the test session creates (TABLES and VIEWS in tests/pyathena/tables.py) appears exactly once.
Without it, no test would follow a real NextToken: the session's tables fit in one page of the default 50, and the resume test is now stubbed.
The comparison ignores tables that other tests create and drop concurrently.
The test turns the Glue fallback off, so a throttled page is retried by the default policy (8 attempts with backoff) instead of being answered by Glue, and it requires at least three distinct NextToken values in the requests, which repeated retries cannot fake.
If throttling outlasts the retry policy, the test fails, as any live metadata request would.
It assumes that Athena's NextToken is not an offset that concurrent table changes in the schema could shift; one probe on a leftover schema showed no shift when a table was added or removed before the current page.

No production code changes.

WHY

Closes #842.

The test passed every request except the injected one, and the expected listing, to the real Athena API with RetryConfig(attempt=1).
A real account-level metadata throttle (#780) under concurrent CI runs then changed the request sequence the test asserts, producing both failure shapes in the issue: a real throttle on a request sent after the Glue fallback failed (run 36218921080, Python 3.13), and a real throttle on page 1 that made its retry the second request, which received the injected error (run 36219249264, Python 3.12; the first of its two "reading it from Glue" warnings came from the expected listing).
With stubbed pages, real throttling cannot reach the asserted requests.

The sibling metadata-throttling tests in tests/pyathena/test_cursor.py, tests/pyathena/aio/test_cursor.py, and both SQLAlchemy test_base.py files stub the Athena metadata operations they call through throttle_metadata_api before the asserted requests, so none of those requests reaches Athena.
Those that read expected first compare Athena's answer with Glue's; if that read is throttled, Glue answers it and the comparison still holds.
They still send real Glue requests, which this PR does not change.

TEST

Tested at 5041b76, rebased onto master 49481d9 (local runs; AWS CI pending while the PR is Draft).

  • just format, just lint: pass.
  • uv run --env-file .env pytest -n 1 tests/pyathena/test_cursor.py -k "test_listing_reads_every_page or test_listing_resumes_after_a_failed_glue_request" -p no:randomly: 2 passed.
  • uv run --env-file .env pytest -n 1 tests/pyathena/test_cursor.py -k "TestCursor and (throttl or glue or metadata_exception or listing)" -p no:randomly: 11 passed.
  • Regression checks (not committed):
    • Resetting next_token to None at the start of each Athena attempt in BaseCursor.list_table_metadata fails the resume test (['t0', 't1', ...2', 't3', ...] == ['t0', 't1', 't2', 't3', 't4']).
    • Returning after the first page fails the listing test (Right contains 5 more items).
    • Ignoring max_results (always 50) fails the listing test (assert 1 >= 3); the version before 5041b76 passed in that case.
  • Measured on a leftover test schema with boto3: ListTableMetadata applies Expression after paging, so pages can be empty yet carry a NextToken, and the order is not by name. This is why the listing test filters on the client instead of using Expression.

🤖 Generated with Claude Code

assert requests[0] is None
assert requests[1] == requests[2]
assert len(requests) == len(set(requests)) + 1
assert requests == [None, "1", "1", "2"]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round 1 (behavior and implementation): CLEAN

  • Base 659676c07e2c09397b7cbc5740cc10bfe6fe41cb, head 8b8686bcfaca1a9a3ec53376c2412de1cfdb61ff; inventory: tests/pyathena/test_cursor.py (one test, no production code).
  • Failure path: BaseCursor.list_table_metadata → _with_glue_fallback resolves the cursor's catalog to AwsDataCatalog, so the injected throttle goes to the unreachable Glue client; not glue.reachable proves that path ran. Without the fallback, RetryConfig(attempt=1) would raise the injected error, so the test cannot pass by skipping Glue.
  • Resume contract: the exact requests list and the ordered names fail if the listing restarts from page 1 (checked locally by resetting next_token per attempt: At index 2 diff: 't0' != 't2') or if the throttled page is skipped.
  • Fidelity: the stub ignores MaxResults; max_results=2 is kept so the request matches the two-name pages. The real NextToken format is irrelevant because the throttled request never reaches Athena, in the old test as well.
  • Out of scope (pre-existing): the aio cursor has no listing-resume counterpart for _async_with_glue_fallback; not added here.

expected = sorted(m.name for m in cursor.list_table_metadata(max_results=2))
client = cursor.connection.client
list_table_metadata = client.list_table_metadata
# The pages are stubbed so that real throttling cannot change the requests.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round 2 (claims, callers, AWS operation): FINDINGS, repaired in the PR description

  • Base 659676c07e2c09397b7cbc5740cc10bfe6fe41cb, head 8b8686bcfaca1a9a3ec53376c2412de1cfdb61ff; claims checked: commit message, PR body, and this code comment.
  • Failure-shape claims checked against the CI logs. Run 36219249264 (3.12): two "reading it from Glue" warnings, one Glue failure, then the injected error from throttle_second_page_once on its first policy call. The second request was therefore sent after Glue failed, so request 1 of the asserted listing was really throttled; the other warning came from the expected listing, which real Glue answered. Run 36218921080 (3.13): one warning, one Glue failure, then a real ThrottlingException ... (reached max retries: 2). The log does not say whether that was the retried page 2 or a later page, so the PR body's "on the retried page" was narrowed to "a request sent after the Glue fallback failed".
  • Sibling claim: test_throttled_reflection_reads_glue (aio SQLAlchemy) stubs only two operations through operations=, so "stub every Athena metadata operation" was reworded to "the operations they call". The PR body now also states that those tests still send real Glue requests.
  • This comment's claim holds: every ListTableMetadata call goes to pages, and Glue goes to the closed local port, so the test sends no Athena or Glue request after the fixture is set up.
  • Callers: test-only change; no production code, fixtures, or helpers are modified.
  • Evidence: local runs were on the working tree that was committed as the head; the AWS matrix has not run yet (Draft).

Comment thread tests/pyathena/test_cursor.py Outdated
assert sorted(m.name for m in cursor.list_table_metadata(max_results=2)) == expected
names = [m.name for m in cursor.list_table_metadata(max_results=2)]

assert names == ["t0", "t1", "t2", "t3", "t4"]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review (relayed): CLEAN

  • Reviewer: Codex CLI 0.157.1 (codex exec, model gpt-6-astra, model_reasoning_effort=high, --sandbox read-only), session 01a0e709-e0dd-7ea3-bc49-216424a2acf7. Static review only: no edits, builds, tests, or network/GitHub access; the prompt omitted the PR number, description, commit message, and self-review findings.
  • Base 659676c07e2c09397b7cbc5740cc10bfe6fe41cb, head 8b8686bcfaca1a9a3ec53376c2412de1cfdb61ff, reviewed in a detached snapshot at the head; the snapshot and PR worktree were unchanged afterwards.
  • Covered: the diff and neighbouring metadata tests, test helpers, cursor fixture/session setup, pagination and Glue fallback, Glue reachability tracking, retry policy, metadata decoding, repository conventions.
  • Reviewer's conclusions: ordered names catch lost or duplicated results; [None, "1", "1", "2"] catches restarting, skipping, or re-reading pages; RetryConfig(attempt=1) stops a retry from hiding the injected throttle; not glue.reachable catches bypassing Glue; {"Name": ...} entries and an absent NextToken match what _list_table_metadata consumes.
  • Non-actionable observations, all existing dependencies shared with sibling tests: unreachable_glue (tests/pyathena/util.py:54) assumes nothing listens on 127.0.0.1:9; the session fixture (tests/pyathena/conftest.py:16) still uses real S3/Athena; the stub does not check MaxResults/catalog/database, and retry-suppression removal is covered by neighbouring tests rather than this one. No changes made.

@laughingman7743
laughingman7743 marked this pull request as ready for review September 28, 2026 08:04
@laughingman7743
laughingman7743 marked this pull request as draft September 28, 2026 10:47
laughingman7743 and others added 4 commits October 2, 2026 01:00
test_listing_resumes_after_a_failed_glue_request passed all but the
injected ListTableMetadata request to Athena, so a real account-level
metadata throttle changed the request sequence it asserts. Serve
synthetic pages instead so the test only exercises the resume logic.

Closes #842

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drop the max_results argument the stubbed pages ignore, read the
request token once, and inline the listing assertion.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With the resume test stubbed, no test followed a real NextToken: the
session's 16 tables fit in one page of the default 50. List them five
per page and check that every session table appears once. The filter
keeps tables other tests create and drop out of the comparison, and a
throttled page answered by Glue gives the same result.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…bles

The session now creates its tables and views from TABLES and VIEWS, and
the executemany tables are created per test, so the hardcoded list was
stale. Read the names from the shared definitions, include the views
that ListTableMetadata also returns, and use four per page so that they
span several pages.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 force-pushed the fix/842-listing-resume-test-stub branch from 17391fa to 6cef032 Compare October 1, 2026 16:04
Comment thread tests/pyathena/test_cursor.py Outdated
cursor.get_table_metadata("one_row")
assert calls == ["get_table_metadata"] * 2

def test_listing_reads_every_page(self, cursor):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round 1 after the rebase (behavior and implementation): CLEAN

  • Base 49481d900e9e0098e3edd562f1a858ce91ec0573 (merge-base with master after rebasing onto Replace name-mangled helpers with single-underscore methods #881 and the shared test tables), head 6cef032c4f5c1ee5a0809cc18346e4f14a3d88e0. Full pass over the whole diff, not only the new commits, because upstream changed tests/pyathena/conftest.py, tests/pyathena/util.py and pyathena/common.py.
  • Upstream check: BaseCursor.list_table_metadata and _with_glue_fallback are unchanged in the paging/resume path. The session now creates TABLES and VIEWS from tests/pyathena/tables.py, and executemany tables are per test, so the earlier hardcoded 16-table list was stale. Replaced it with the shared definitions.
  • test_listing_reads_every_page: the default cursor (default retry policy, Glue fallback on) lists ENV.schema four per page. Filtering to the session's names makes tables created and dropped by other tests irrelevant. A missing or duplicated session entry fails the sorted comparison. Locally, returning after the first page fails it (Right contains 5 more items).
  • test_listing_resumes_after_a_failed_glue_request: still sends no Athena request. Locally, resetting next_token per attempt fails it.
  • Limitation: a run whose listing is throttled is answered by Glue and passes without exercising the pages; it does not fail.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review of the repair in 5041b76 (both perspectives): CLEAN

  • Scope: 6cef032c..5041b763 (git range-diff: the first four commits are unchanged, and one commit was added). The change is test_listing_reads_every_page only.
  • Behavior: glue_metadata_fallback=False goes through the indirect cursor fixture, the same way the disabled case of test_throttled_metadata_without_glue does. A throttled page is then retried by RetryConfig() (8 attempts, ThrottlingException included) and resumes at the same token. The recorder passes every call through and only lower-bounds the number of distinct tokens, so real throttling adds repeated tokens and cannot break the assertion. This is unlike the pass-through in Flaky test: test_listing_resumes_after_a_failed_glue_request under real metadata throttling #842, which asserted an exact sequence.
  • Regressions caught locally: first page only (Right contains 5 more items), and MaxResults forced to 50 (assert 1 >= 3).
  • Claims: the PR description no longer says Glue answers a throttled page. It now states the retry-exhaustion failure mode and the NextToken offset assumption. The commit message's "Repeated tokens from retries are counted once" matches set(tokens).
  • Cost: unchanged, about ceil(entries in schema / 4) ListTableMetadata calls plus any retries, and no queries.
  • Validation at 5041b76: just lint passes; both tests passed; the TestCursor and (throttl or glue or metadata_exception or listing) selection gave 11 passed.

assert requests[1] == requests[2]
assert len(requests) == len(set(requests)) + 1
assert [m.name for m in cursor.list_table_metadata()] == ["t0", "t1", "t2", "t3", "t4"]
assert requests == [None, "1", "1", "2"]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round 2 after the rebase (claims, callers, AWS operation): CLEAN

  • Base 49481d900e9e0098e3edd562f1a858ce91ec0573, head 6cef032c4f5c1ee5a0809cc18346e4f14a3d88e0; claims checked: PR body, commit messages, test comments.
  • "No test would follow a real NextToken": other max_results/NextToken uses in tests/pyathena (test_cursor.py:300-319, aio/test_cursor.py:261-282) mock list_query_executions. The session has 9 tables and views, which fit in one default page of 50.
  • "Glue answers the whole listing with the same tables": GlueMetadataClient.list_tables pages GetTables, which returns views too, so the filtered names match.
  • Views are included because ListTableMetadata returns them (seen on a leftover schema: v_one_row, view_one_row).
  • The comment avoids a table count, so adding a table to TABLES keeps it true. Four per page gives at least three pages for the current nine.
  • AWS operation: the new test adds about ceil(objects in schema / 4) ListTableMetadata calls per run, no queries. The resume test no longer makes any Athena request.
  • CI: tests/** changes select the PyAthena suite (.github/workflows/test.yaml), so marking Ready runs the AWS job. Local evidence is from head 6cef032c4f5c1ee5a0809cc18346e4f14a3d88e0; AWS CI has not run on it yet.

A throttled page was answered by Glue with the whole listing, and a
listing that ignored max_results came back in one page; either way the
test passed without following a NextToken. Turn the Glue fallback off
so a throttled page is retried instead, and require at least three
distinct request tokens. Repeated tokens from retries are counted once.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

# Without the Glue fallback, a throttled page is retried, so Athena serves every page.
@pytest.mark.parametrize("cursor", [{"glue_metadata_fallback": False}], indirect=["cursor"])
def test_listing_reads_every_page(self, cursor, monkeypatch):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review after the rebase (relayed): FINDINGS → repaired in 5041b76

  • Reviewer: Codex CLI 0.157.1 (codex exec, model gpt-6-sol, model_reasoning_effort=high, --sandbox read-only), session 01a0f836-8bba-7cf2-b281-fdde829dddeb. Static review only. The prompt omitted the PR number, description, commit messages, and self-review findings. Snapshot at the head, unchanged afterwards.
  • Base 49481d900e9e0098e3edd562f1a858ce91ec0573, head 6cef032c4f5c1ee5a0809cc18346e4f14a3d88e0. Covered: both changed tests and neighbouring throttling tests, session tables/views/setup and cursor fixture, Athena listing/retry/Glue fallback paths, conventions.
  • Finding 1 (verified, repaired): the live listing test could pass without following Athena's NextToken. A throttled page was answered by Glue's whole listing, and a regression that dropped max_results returned all nine session entries in one page. Reproduced locally: forcing MaxResults to 50 passed the old version. Repair: glue_metadata_fallback=False, so a throttled page is retried by the default policy, plus len(set(tokens)) >= 3 from a pass-through recorder; retries repeat a token, so they cannot inflate the count. With the repair, the forced-50 regression fails (assert 1 >= 3) and the first-page-only regression fails (Right contains 5 more items).
  • Finding 2 (deferred, documented): concurrent table creation/deletion could change page membership if NextToken were an offset. Not fixed. One probe on a leftover schema showed no shift when a table was added or removed before the current page, and a dedicated database would add DDL to every run. The assumption is stated in the PR description.
  • Non-actionable: the stubbed resume test is independent of real throttling, and its exact request assertion catches both re-reading an earlier page and skipping the retry.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent follow-up on the repair (relayed): CLEAN

  • Reviewer: Codex CLI 0.157.1 (codex exec, model gpt-6-sol, model_reasoning_effort=high, --sandbox read-only), session 01a0f83d-108b-7e32-ba88-396821237828. Static review of patch 6cef032c..5041b763 with the full diff 49481d90..5041b763 for context. Snapshot unchanged afterwards.
  • Reviewer's conclusions: the repair closes finding 1. With the Glue fallback off, the recorder sees Athena's requests, and the nine session entries at max_results=4 need at least three distinct page requests, so neither a single oversized page nor a listing that stops early satisfies both assertions. Retries reuse a page's token and cannot inflate the count.
  • Non-actionable: sustained real throttling can still exhaust the default eight attempts and fail this live test, like any live metadata request. The concurrent-schema-change assumption from finding 2 remains.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 1, 2026 16:14
@laughingman7743
laughingman7743 merged commit ecb19c1 into master Oct 1, 2026
11 checks passed
@laughingman7743
laughingman7743 deleted the fix/842-listing-resume-test-stub branch October 1, 2026 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: test_listing_resumes_after_a_failed_glue_request under real metadata throttling

1 participant