Stub every page in the listing resume test - #871
Conversation
| assert requests[0] is None | ||
| assert requests[1] == requests[2] | ||
| assert len(requests) == len(set(requests)) + 1 | ||
| assert requests == [None, "1", "1", "2"] |
There was a problem hiding this comment.
Self-review round 1 (behavior and implementation): CLEAN
- Base
659676c07e2c09397b7cbc5740cc10bfe6fe41cb, head8b8686bcfaca1a9a3ec53376c2412de1cfdb61ff; inventory:tests/pyathena/test_cursor.py(one test, no production code). - Failure path:
BaseCursor.list_table_metadata→_with_glue_fallbackresolves the cursor's catalog to AwsDataCatalog, so the injected throttle goes to the unreachable Glue client;not glue.reachableproves 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
requestslist and the ordered names fail if the listing restarts from page 1 (checked locally by resettingnext_tokenper attempt:At index 2 diff: 't0' != 't2') or if the throttled page is skipped. - Fidelity: the stub ignores
MaxResults;max_results=2is 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. |
There was a problem hiding this comment.
Self-review round 2 (claims, callers, AWS operation): FINDINGS, repaired in the PR description
- Base
659676c07e2c09397b7cbc5740cc10bfe6fe41cb, head8b8686bcfaca1a9a3ec53376c2412de1cfdb61ff; 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_onceon 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 theexpectedlisting, which real Glue answered. Run 36218921080 (3.13): one warning, one Glue failure, then a realThrottlingException ... (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 throughoperations=, 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
ListTableMetadatacall goes topages, 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).
| 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"] |
There was a problem hiding this comment.
Independent review (relayed): CLEAN
- Reviewer: Codex CLI 0.157.1 (
codex exec, model gpt-6-astra,model_reasoning_effort=high,--sandbox read-only), session01a0e709-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, head8b8686bcfaca1a9a3ec53376c2412de1cfdb61ff, 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.reachablecatches bypassing Glue;{"Name": ...}entries and an absentNextTokenmatch what_list_table_metadataconsumes. - Non-actionable observations, all existing dependencies shared with sibling tests:
unreachable_glue(tests/pyathena/util.py:54) assumes nothing listens on127.0.0.1:9; the session fixture (tests/pyathena/conftest.py:16) still uses real S3/Athena; the stub does not checkMaxResults/catalog/database, and retry-suppression removal is covered by neighbouring tests rather than this one. No changes made.
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>
17391fa to
6cef032
Compare
| cursor.get_table_metadata("one_row") | ||
| assert calls == ["get_table_metadata"] * 2 | ||
|
|
||
| def test_listing_reads_every_page(self, cursor): |
There was a problem hiding this comment.
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), head6cef032c4f5c1ee5a0809cc18346e4f14a3d88e0. Full pass over the whole diff, not only the new commits, because upstream changedtests/pyathena/conftest.py,tests/pyathena/util.pyandpyathena/common.py. - Upstream check:
BaseCursor.list_table_metadataand_with_glue_fallbackare unchanged in the paging/resume path. The session now createsTABLESandVIEWSfromtests/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) listsENV.schemafour 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, resettingnext_tokenper attempt fails it.- Limitation: a run whose listing is throttled is answered by Glue and passes without exercising the pages; it does not fail.
There was a problem hiding this comment.
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 istest_listing_reads_every_pageonly. - Behavior:
glue_metadata_fallback=Falsegoes through the indirectcursorfixture, the same way thedisabledcase oftest_throttled_metadata_without_gluedoes. A throttled page is then retried byRetryConfig()(8 attempts,ThrottlingExceptionincluded) 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), andMaxResultsforced 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
NextTokenoffset assumption. The commit message's "Repeated tokens from retries are counted once" matchesset(tokens). - Cost: unchanged, about
ceil(entries in schema / 4)ListTableMetadatacalls plus any retries, and no queries. - Validation at 5041b76:
just lintpasses; both tests passed; theTestCursor 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"] |
There was a problem hiding this comment.
Self-review round 2 after the rebase (claims, callers, AWS operation): CLEAN
- Base
49481d900e9e0098e3edd562f1a858ce91ec0573, head6cef032c4f5c1ee5a0809cc18346e4f14a3d88e0; claims checked: PR body, commit messages, test comments. - "No test would follow a real
NextToken": othermax_results/NextTokenuses intests/pyathena(test_cursor.py:300-319,aio/test_cursor.py:261-282) mocklist_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_tablespagesGetTables, which returns views too, so the filtered names match. - Views are included because
ListTableMetadatareturns them (seen on a leftover schema:v_one_row,view_one_row). - The comment avoids a table count, so adding a table to
TABLESkeeps 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)ListTableMetadatacalls 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 head6cef032c4f5c1ee5a0809cc18346e4f14a3d88e0; 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): |
There was a problem hiding this comment.
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), session01a0f836-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, head6cef032c4f5c1ee5a0809cc18346e4f14a3d88e0. 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 droppedmax_resultsreturned all nine session entries in one page. Reproduced locally: forcingMaxResultsto 50 passed the old version. Repair:glue_metadata_fallback=False, so a throttled page is retried by the default policy, pluslen(set(tokens)) >= 3from 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
NextTokenwere 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.
There was a problem hiding this comment.
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), session01a0f83d-108b-7e32-ba88-396821237828. Static review of patch6cef032c..5041b763with the full diff49481d90..5041b763for 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=4need 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.
WHAT
Rewrite
TestCursor::test_listing_resumes_after_a_failed_glue_requestso everyListTableMetadatarequest 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 (TABLESandVIEWSintests/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
NextTokenvalues 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
NextTokenis 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
expectedlisting, to the real Athena API withRetryConfig(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
expectedlisting).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 SQLAlchemytest_base.pyfiles stub the Athena metadata operations they call throughthrottle_metadata_apibefore the asserted requests, so none of those requests reaches Athena.Those that read
expectedfirst 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.next_tokentoNoneat the start of each Athena attempt inBaseCursor.list_table_metadatafails the resume test (['t0', 't1', ...2', 't3', ...] == ['t0', 't1', 't2', 't3', 't4']).Right contains 5 more items).max_results(always 50) fails the listing test (assert 1 >= 3); the version before 5041b76 passed in that case.ListTableMetadataappliesExpressionafter paging, so pages can be empty yet carry aNextToken, and the order is not by name. This is why the listing test filters on the client instead of usingExpression.🤖 Generated with Claude Code