[laughingman7743]

WHAT

Remove tests/pyathena/s3fs/test_cursor.py::TestS3FSCursor::test_on_start_query_execution, the row of #1246 that #1252 kept until #1243 rewrote the connection callback test, and move its one unique check, an on_start_query_execution given through the connection's cursor_kwargs, into the offline test_connection.py::test_cursor_accepts_connection_callbacks, which now gives the callbacks through the connection's attributes, through its cursor_kwargs alone, and through its cursor_kwargs over other callbacks on the connection, for every cursor class.

WHY

Part of #1239, carried over from #1246. On master, test_connection.py::test_cursor_accepts_connection_callbacks (rewritten by #1253) runs on_start_query_execution and on_poll through each cursor class, and S3FSCursor is in its START_CALLBACK_CURSORS. The removed test repeated that contract with a live Athena query, except that it set the callback through cursor_kwargs; that path is shared by every cursor class (pyathena/connection.py:654-657 and pyathena/common.py:259), so the connection test runs it for each of them. Its test IDs gain a -connection, -cursor_kwargs, or -cursor_kwargs_over_connection suffix, and it has 63 cases instead of 21 (#1258 added the aio cursors to it). The callback given to execute() stays tested by TestS3FSCursor::test_on_start_query_execution_execute.

TEST

Python 3.15.0. At 7b04c97, rebased on fb22d08 after #1258 changed this test into an async test over 21 cursor classes:

• just format: no changes. just lint: passed.
• Mutation checks, offline with --noconftest and placeholder settings; git diff -- pyathena/ was empty after each:
  • Removing the self._call_on_start_query_execution(self.query_id, options) call from S3FSCursor.execute() (pyathena/s3fs/cursor.py:203) fails the 3 S3FSCursor cases of test_cursor_accepts_connection_callbacks.
  • Storing the connection's callback instead of the constructor argument (pyathena/common.py:259) fails 24 cursor_kwargs cases.
  • Letting the connection's callbacks win over cursor_kwargs in Connection._cursor() fails 42 cases.
  • Calling the cursor's callback only when the connection also has one (pyathena/common.py:1270) fails the 12 cursor_kwargs cases of the cursors that run on_start_query_execution.
  • Changing the kwargs.pop("on_start_query_execution", ...) of Connection._cursor() to kwargs.get(...), so that the callback reaches the constructor twice, fails the 42 cursor_kwargs cases with TypeError.
  • Without a mutation, the 63 cases pass, and the offline tests/pyathena/test_connection.py passes (229).

No live tests were run locally, because the change removes a live test and extends an offline one. CI after Ready, at 7b04c97: Test run 39085589490 passed (test / run (3.15), lint); test-sqla and test-sqla-async were skipped by the changes path filter, since this PR changes no SQLAlchemy or core path.

🤖 Generated with Claude Code

[laughingman7743]

Self-review of ea99b58e17b52252940951210ec445e2f167d9bc..b125bfa4c728f12b31c82ea6c118dedc149758f2.

Round 1 (behavior and test quality): CLEAN. The removed test passed the callback through the connection's cursor_kwargs, which Connection.cursor() merges into the constructor arguments for every cursor class (pyathena/connection.py:620), and S3FSCursor.execute() calls it at pyathena/s3fs/cursor.py:203. Removing that call fails test_connection.py::test_cursor_accepts_connection_callbacks[S3FSCursor] offline. contextlib, connect, and ENV stay in use by the remaining tests; just lint passed.

Round 2 (claims and operation): CLEAN. On ea99b58, START_CALLBACK_CURSORS includes S3FSCursor (tests/pyathena/test_connection.py:55), the cover asserts the started query ID and the polled state, and TestS3FSCursor::test_on_start_query_execution_execute keeps the execute() callback. The change saves one Athena query per run; no local live run, as the PR body states.

[laughingman7743]

Independent review (relayed): Codex CLI 0.163.0, model gpt-6-astra, ChatGPT login, codex exec -s read-only, session 01a14c71-f1d3-7a93-9fe7-0c9e7bd4c423. Static review of ea99b58e..b125bfa4 in a clean detached snapshot, with the diff and the test policy. Covered: connection construction and cursor arguments, S3FSCursor construction and execution, the shared callback dispatch, and the callback tests under tests/pyathena/. Result: FINDINGS (1).

P2: the removed test was the only one that gave on_start_query_execution through cursor_kwargs; changing its kwargs.pop(...) in Connection._cursor() (pyathena/connection.py:654) to kwargs.get(...) passes every remaining test, while conn.cursor() would raise TypeError for a duplicated keyword.

Repaired in 6349697: this test gives the callback through cursor_kwargs over a connection default and expects the configured one. That mutation now fails with got multiple values for keyword argument 'on_start_query_execution', and letting the connection's callback win fails too (offline). The row becomes a MERGE; the PR title and body are updated.

Self-review of the repair b125bfa4..6349697b: round 1 CLEAN (the test uses RecordingCursor, which records the constructor arguments that Connection.cursor() passes, the boundary under test); round 2: the PR title and body are corrected from a removal to a merge; just lint passed.

[laughingman7743]

Independent follow-up review (relayed): Codex CLI 0.163.0, model gpt-6-astra, ChatGPT login, codex exec -s read-only, session 01a14c75-14f7-7f93-88bd-8ee554205975. Static review of b125bfa4..6349697b. It confirmed that the added check catches the pop to get mutation. Result: FINDINGS (1).

P2: RecordingCursor records the argument before the base constructor stores it, so storing connection.on_start_query_execution instead of the constructor argument (pyathena/common.py:259) passed; the other execution tests give the connection's callback or an execute() callback.

Repaired in 8aeb688: this test runs the callbacks given through the connection's attributes and through its cursor_kwargs for every cursor class, and the RecordingCursor check is reverted. Mutations, offline: that store fails the 6 cursor_kwargs cases of the cursors that run on_start_query_execution; pop to get fails the 14 cursor_kwargs cases; removing the S3FS call fails both S3FSCursor cases. 28 cases pass without a mutation; just lint passed.

Self-review of 6349697b..8aeb6883: round 1 CLEAN (the offline_connection fixture is function-scoped, so setting cursor_kwargs does not leak; on_poll is now also checked through cursor_kwargs); round 2: the PR title and body now describe the parametrized test, its renamed IDs, and the 28 cases.

[laughingman7743]

Independent follow-up review 2 (relayed): Codex CLI 0.163.0, model gpt-6-astra, ChatGPT login, codex exec -s read-only, session 01a14c79-d42b-7eb2-a2d2-bac88a609d35. Static review of 6349697b..8aeb6883. It confirmed that the parametrized test catches the stored-callback and pop to get mutations. Result: FINDINGS (2).

1 (P2): the cursor_kwargs case assigns them after the connection is built, so a constructor that dropped only on_start_query_execution from cursor_kwargs (after pyathena/connection.py:365) would pass. Not repaired: the constructor stores cursor_kwargs as one dict for every key, which TestConnection::test_cursor_arraysize and test_cursor_kwargs_override_connection_defaults check through _connection(cursor_kwargs=...); a key-specific drop is not a path the code has.

2 (P2): with one source per case, the connection's callback winning over cursor_kwargs passed. Repaired in 96408aa: the cursor_kwargs case also sets other callbacks on the connection and asserts that they do not run; letting the connection's callbacks win fails the 14 cursor_kwargs cases, and storing the connection's callback still fails 6 (offline). just lint passed.

Self-review of the repair: round 1 CLEAN; round 2: the PR body's description of the test still holds (two sources per class, 28 cases).

[laughingman7743]

Independent follow-up review 3 (relayed): Codex CLI 0.163.0, model gpt-6-astra, ChatGPT login, codex exec -s read-only, session 01a14c7f-6fc4-77d1-83c5-0d90a279deb4. Static review of 8aeb6883..96408aa9. It confirmed that the repair rejects the connection's callbacks winning and invoking both sources. Result: FINDINGS (1).

P2: the repair put connection callbacks into every cursor_kwargs case, so calling the cursor's callback only when the connection also has one (pyathena/common.py:1270) passed.

Repaired in 911f8b4: the test has three sources, connection, cursor_kwargs alone, and cursor_kwargs_over_connection (42 cases). Mutations, offline: that guard fails 6 cases; the connection's callbacks winning fails 28; storing the connection's callback fails 12; pop to get fails 28 with TypeError; removing the S3FS call fails 3. just lint passed. Self-review of the repair: round 1 CLEAN; round 2: the PR body now lists the three sources, the 42 cases, and these mutation results.

[laughingman7743]

Independent follow-up review 4 (relayed): Codex CLI 0.163.0, model gpt-6-astra, ChatGPT login, codex exec -s read-only, session 01a14c83-90bd-78e0-bf49-4fe2422356f2. Static review of 96408aa9..911f8b44 in a clean detached snapshot of 911f8b4. Result: CLEAN. Covered: the repair, fixture isolation, callback forwarding and execution for all 14 cursor classes, async completion, and cleanup. It found that all three sources assert delivery (on_poll receives the FAILED state; the six cursors that run it deliver the query ID to the start callback), and that the override case still rejects delivery to the connection's callbacks.

[laughingman7743]

Rebase onto fb22d08 after #1258 rewrote this test into an async test over 21 cursor classes, including the aio cursors; the patch series is unchanged, and the conflicts in this test were resolved by keeping master's async structure and adding the given_by parameter. Re-checked at 7b04c97 offline: 63 cases pass and tests/pyathena/test_connection.py passes (229); the mutations fail 3 (S3FS call), 24 (stored connection callback), 42 (connection wins), 12 (guard), and 42 (pop to get) cases; just lint passed.

Independent follow-up review of the rebase (relayed): Codex CLI 0.163.0, model gpt-6-astra, ChatGPT login, codex exec -s read-only, session 01a14c91-0029-75d3-b124-53270880a7ff, with the rebased diff and the range-diff against 911f8b4. Result: CLEAN. It found that the test keeps master's 21 classes, async execution, bounded future wait, and cleanup; that all seven aio constructors forward the callbacks; and that every other test of the file is unchanged from master.