Skip to content

Stub every page in the listing resume test #871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laughingman7743 merged 5 commits into from
Oct 1, 2026
Merged

Stub every page in the listing resume test #871

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ecf23b
Stub every page in the listing resume test
laughingman7743 Sep 28, 2026
8b0d8a8
Simplify the stubbed listing resume test
laughingman7743 Sep 28, 2026
2a83e8d
Check that a listing reads every page against Athena
laughingman7743 Sep 28, 2026
6cef032
Take the session's tables for the listing test from tests.pyathena.ta…
laughingman7743 Oct 1, 2026
5041b76
Make the listing test read every page from Athena
laughingman7743 Oct 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 37 additions & 13 deletions tests/pyathena/test_cursor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
from pyathena.util import RetryConfig
from tests import ENV
from tests.pyathena.conftest import connect
from tests.pyathena.tables import TABLES, VIEWS
from tests.pyathena.util import succeeded_query_execution, throttle_metadata_api, unreachable_glue

_logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -1571,34 +1572,57 @@ def test_other_metadata_exception_keeps_its_retries(self, cursor, monkeypatch):
cursor.get_table_metadata("one_row")
assert calls == ["get_table_metadata"] * 2

# 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):

@laughingman7743 laughingman7743 Oct 1, 2026

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.

@laughingman7743 laughingman7743 Oct 1, 2026

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.

client = cursor.connection.client
list_table_metadata = client.list_table_metadata
tokens = []

def record_token(**kwargs):
tokens.append(kwargs.get("NextToken"))
return list_table_metadata(**kwargs)

monkeypatch.setattr(client, "list_table_metadata", record_token)
# Other tests add and drop their own tables, so only the session's are compared.
session_tables = sorted(t.name for t in (*TABLES, *VIEWS))

names = [m.name for m in cursor.list_table_metadata(max_results=4)]

assert sorted(n for n in names if n in session_tables) == session_tables
# At four per page, the session's tables and views span three pages or more.
assert len(set(tokens)) >= 3

@pytest.mark.parametrize(
"cursor", [{"retry_config": RetryConfig(attempt=1)}], indirect=["cursor"]
)
def test_listing_resumes_after_a_failed_glue_request(self, cursor, monkeypatch):
# A page throttled mid-listing is read again, not the pages before it.
expected = sorted(m.name for m in cursor.list_table_metadata(max_results=2))
# The pages are stubbed so that real throttling cannot change the requests.

@laughingman7743 laughingman7743 Sep 28, 2026

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).

pages = {
None: {"TableMetadataList": [{"Name": "t0"}, {"Name": "t1"}], "NextToken": "1"},
"1": {"TableMetadataList": [{"Name": "t2"}, {"Name": "t3"}], "NextToken": "2"},
"2": {"TableMetadataList": [{"Name": "t4"}]},
}
client = cursor.connection.client
list_table_metadata = client.list_table_metadata
requests = []

def throttle_second_page_once(**kwargs):
requests.append(kwargs.get("NextToken"))
def throttle_second_request_once(**kwargs):
token = kwargs.get("NextToken")
requests.append(token)
if len(requests) == 2:
raise ClientError(
{"Error": {"Code": "ThrottlingException", "Message": "Rate exceeded"}},
"ListTableMetadata",
)
return list_table_metadata(**kwargs)
return pages[token]

monkeypatch.setattr(client, "list_table_metadata", throttle_second_page_once)
self._unreachable_glue(cursor.connection, monkeypatch)
monkeypatch.setattr(client, "list_table_metadata", throttle_second_request_once)
glue = self._unreachable_glue(cursor.connection, monkeypatch)

assert sorted(m.name for m in cursor.list_table_metadata(max_results=2)) == expected
# Every page once, and the throttled second page a second time.
assert len(expected) > 2
assert requests[0] is None
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"]

@laughingman7743 laughingman7743 Sep 28, 2026

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.

@laughingman7743 laughingman7743 Oct 1, 2026

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.

assert not glue.reachable

@pytest.mark.parametrize(
("cursor", "catalog_name"),
Expand Down
Loading