Audit the test suite for unnecessary and flaky tests

Summary

Audit tests/pyathena/ for tests that are not needed and for flaky tests, then fix them in grouped PRs. A test is needed when it protects a contract that no other test protects, at the lowest cost that still exercises the real behavior.

Facts (master `59169c5`, 2026-10-17)

tests/pyathena/ collects 4,396 tests:

Directory Tests

filesystem 1,321

top-level files 798

sqlalchemy 726

pandas 574

aio 379

polars 202

arrow 157

spark 123

s3fs 116

The largest files:

File	Tests
`filesystem/test_s3.py`	616
`sqlalchemy/test_compiler.py`	247
`pandas/test_result_set.py`	243
`sqlalchemy/test_base.py`	229
`filesystem/test_s3_async.py`	215
`pandas/test_cursor.py`	212
`filesystem/test_s3_core.py`	210

In the 400 Test workflow runs since 2026-10-09, these tests failed in one run and passed in another run of the same commit:

Failures	Test
2	`filesystem/test_s3_async.py::TestAioS3StreamedFile::test_write_failed_part_while_waiting` (test_write_failed_part_while_waiting fails intermittently since queued parts are cancelled #1237)
2	`polars/test_cursor.py::TestPolarsCursor::test_null_vs_empty_string`
1	`polars/test_result_set.py::TestPolarsDataFrameIterator::test_itertuples_value_python_does_not_hold`
1	`arrow/test_cursor.py::TestArrowCursor::test_complex_unload_as_polars`
1	`test_cursor.py::TestCursor::test_contain_special_character_query_with_parameter`
1	`aio/test_util.py::test_create_in_thread_cancelled_construction_error`

Older failed runs (2026-09-19 to 2026-10-09, mostly the SQLAlchemy reflection suite) are not classified yet.

test_write_failed_part_while_waiting fails intermittently since queued parts are cancelled #1237 shows the pattern this audit looks for. The test asserted which parts called upload_part, an implementation detail. A correct change to the cancellation order made it flaky.
PYTEST_RERUN reruns a test once only for two Athena service errors ("Amazon Athena experienced an internal error", "Invalid S3 request"). It does not hide other flakiness.

Criteria

A test is a candidate for removal or rewriting when it:

asserts implementation details instead of observable behavior, such as call order, call counts, private method sequencing, or log text that is not part of a contract;
duplicates another test's contract, through parametrization or a near copy in sync, async, and aio variants, where a shared helper already covers them;
depends on thread or task timing, wall-clock sleeps, or ordering between concurrent workers without synchronizing it;
calls AWS where a stubbed or offline test exercises the same behavior, or runs more queries than its contract needs;
passes without the behavior it names, because a mock or fake omits what it checks.

The SQLAlchemy compliance suite under tests/sqlalchemy/ keeps its upstream classes and plugin conventions, so this audit only records its flakiness.

Plan

Flakiness
- Classify the older failures by whether another run of the same commit passed.
- Reproduce each candidate under pytest -n 8 load.
- Record the cause.
Necessity
- Review tests/pyathena/ by directory against the criteria.
- List each finding with file::test, the criterion, the cost, and the proposed change: remove, merge, rewrite, or make offline.
Results
- Post the inventory on this issue for the maintainer's review before changing tests.
- Then open one sub-issue per fixing PR, with numbered sections.
- Each PR keeps test IDs, marks, fixture scopes, and cleanup where tests are reorganized, as the test conventions require.

Handoff: first findings from the flakiness classification. This session continues #1233; the audit continues in another session.

Data: 1,340 Test workflow runs from 2026-09-01 to 2026-10-17, 56 of them failed. A candidate failed in one run and passed in another run of the same commit. The scripts and data are in a local scratch directory and are not committed, so the commands below reproduce them.

gh api "repos/pyathena-dev/PyAthena/actions/workflows/test.yaml/runs?created=2026-09-01..2026-10-09&status=failure&per_page=100"
gh run view <id> -R pyathena-dev/PyAthena --log-failed < /dev/null | grep -oE 'FAILED tests/\S+'

Of the 11 candidates, most are not timing-dependent. Some assert library- or version-specific details that differ between runs of one commit, which may use different Python or dependency versions. Others, at a commit under development, depended on state that the tests set up:

Test	Failure	Likely cause
`filesystem/test_s3_async.py::TestAioS3StreamedFile::test_write_failed_part_while_waiting`	`[2] == [1, 2]`	Asserts which parts ran; timing since #1231 (#1237).
`aio/test_util.py::test_create_in_thread_cancelled_construction_error`	an extra "exception in shielded future" log record	Timing between cancellation and a shielded future.
`polars/test_result_set.py::TestPolarsDataFrameIterator::test_itertuples_value_python_does_not_hold`	`'year 0'` against `'year must be in 1..9999, not 0'`	Matches an exception message that differs by Python version.
`arrow/test_cursor.py::TestArrowCursor::test_complex_unload_as_polars`	`Map(Int32, Int32)` against `List(Struct(...))`	A dtype that differs by Polars version.
`polars/test_cursor.py::TestPolarsCursor::test_null_vs_empty_string`, `test_cursor.py::TestCursor::test_contain_special_character_query_with_parameter`	`format requires a parenthesised mapping key`; `'NoneType' object has no attribute 'to_list'`	One run (38560114057); check what differed in that run.
`arrow/test_cursor.py::TestArrowCursor::test_managed_results_match_result_file`	microseconds missing from a timestamp	One run (37174556837); check the reader versions.
`pandas/test_cursor.py::TestPandasCursor::test_get_csv_engine_explicit_specification`	`no attribute '_metadata'`	Depends on a private attribute.
`aio/test_cursor.py::TestAioCursor::test_throttled_metadata_reads_glue`, `test_cursor.py::TestCursor::test_throttled_metadata_reads_glue`, `test_glue.py::TestGlueMetadataClient::test_reads_what_athena_reports`	Glue table properties (`presto_view`, `location`, ...) differ	Compares the whole parameter dictionaries that Glue returns.

The maintainer noted on 2026-10-17 that AI-written tests are of poor quality. Patterns to add to the criteria:

matching exception or log message text that is not a contract;
asserting dtypes or reprs that depend on the library version;
patching private methods or attributes;
asserting the exact arguments of internal calls.

Keep the write tests that check behavior (GH-1239)

Inventory of the necessity audit at master 59169c5. Every test function in tests/pyathena/ was reviewed. No test is changed before the maintainer's review. Each sub-issue below lists its rows with the covering test.

Criteria added on 2026-10-17

The audit applied criteria 1–5 above, with the patterns of the previous comment counted under criteria 1 and 8, and these three:

asserts what the code literally says, such as a constructor storing its arguments, a default value, a constant, a __repr__, or a dataclass field, or tests Python or a third-party library instead of PyAthena;
only raises coverage: exercises a branch or a private helper that has no user-visible contract, while public-API tests already exercise the real behavior;
asserts dtypes, reprs, or messages that differ between Python, pandas, Polars, or PyArrow versions.

Baseline

At 21cbf05 (v3.15.0, 2025-07-19), before AI-assisted development, the suite had 349 test functions: 333 in tests/pyathena/ and 16 in the compliance suite tests/sqlalchemy/test_suite.py.
At 59169c5, tests/pyathena/ has 2,048 test functions.
The original tests are kept, except 42 that the findings name:
- parameter trims of 30 TestPandasCursor and TestAsyncPandasCursor tests, where ({"unload": True}, "pyarrow") repeats ({"unload": True}, "auto"), or where chunksize and the engine do not change the query;
- 10 TestAsyncPandasCursor tests (test_many_as_pandas, test_show_columns, test_empty_result_ddl, ...) that run the sync tests' queries through the same AthenaPandasResultSet;
- sqlalchemy/test_base.py::TestSQLAlchemyAthena::test_reflect_table: restore MetaData(), which 7793651 changed to MetaData(schema=ENV.schema), making the test a copy of test_reflect_table_with_schema;
- pandas/test_util.py::test_to_sql_with_multiple_partitions: assert sorted(cursor.fetchall()), [...] always passes; change , to ==.

Counts

Test functions at master 59169c5, after the cross-check corrections below.

#	Sub-issue	Test functions	KEEP	REMOVE	MERGE	REWRITE	Saved per run
#1240	Sync filesystem (`filesystem/test_s3.py`, `test_s3_writer.py`, `test_s3_object.py`, `test_reader.py`, `test_init.py`)	364	260	56	16	32	13 real-S3 cases, about 120–140 MiB of multipart transfers
#1241	Async filesystem and S3 core (`filesystem/test_s3_async.py`, `test_s3_core.py`, `test_s3_executor.py`, `test_s3_path_pairing.py`, `test_s3_path.py`, `test_s3_errors.py`)	278	169	70	18	21	about 450 S3 requests, about 100 MiB of uploads
#1242	SQLAlchemy (`sqlalchemy/`, `aio/sqlalchemy/`)	333	237	62	15	19	about 140 Athena queries, 2 metadata API calls
#1243	Top-level (`test_*.py`)	366	277	70	4	15	about 75 Athena queries, about 12 metadata requests
#1244	pandas (`pandas/`, `aio/pandas/`)	194	88	50	9	47	about 150 Athena queries, 5 external tables
#1245	aio and Spark (`aio/` except `sqlalchemy/`, `pandas/`; `spark/`)	224	102	65	29	28	about 85 Athena queries, 4 Spark sessions, about 8 calculations
#1246	Polars, Arrow, S3FS (`polars/`, `arrow/`, `s3fs/`)	289	212	55	6	16	about 44 Athena queries
	Total	2,048	1,345	428	97	178	about 490 Athena queries, about 450 S3 requests, about 220–240 MiB of S3 transfers

The open PRs change these counts:

Break result sets after failed or cancelled fetches and drop the cross-thread guards #1238 deletes 14 of the REMOVE tests: 5 in test_result_set.py, 4 in pandas/test_result_set.py, 4 in polars/test_result_set.py, and aio/test_util.py::test_async_retry_api_call_stop_when.
Move S3File writes onto a multipart write session #1232 deletes TestS3File::test_write_interrupted_while_waiting_for_part (REMOVE) and TestS3File::test_upload_chunk_empty_file_touches (REWRITE). After it, TestS3File::test_discard and test_commit_stops_at_failed_part are KEEP, and test_upload_chunk_small_file and test_upload_chunk_multipart are REWRITE: they are the only tests that a second commit() or discard() sends no request.
Move S3File writes onto a multipart write session #1232 fixes TestAioS3StreamedFile::test_write_failed_part_while_waiting (test_write_failed_part_while_waiting fails intermittently since queued parts are cancelled #1237); Remove and rewrite unnecessary tests in the async S3 filesystem and S3 core tests #1241 re-checks it after Move S3File writes onto a multipart write session #1232.

Patterns

Copies across sync, async, and aio cursors or filesystems that run the same live queries or S3 calls through shared code. Examples: the 37 AWS tests in TestAioS3FileSystem that pass through one-line _sync_fs delegations (test_info_file, test_ls_dirs, test_checksum, test_sign); TestAsyncPandasCursor::test_integer_na_values; TestAioArrowCursor::test_fetchall and its Polars and S3FS copies.
Patches of private methods and asserts on their call arguments. Examples: TestS3FileSystem::test_mv_null_version_onto_key (_copy_file, _delete_objects); the TestS3File max-parts tests that read executor.submit_to.call_args_list or patch the module wait; spark/test_common.py tests that patch _calculate and _wait_for_idle_session; TestPandasCursor::test_get_csv_engine_explicit_specification, which failed in CI on _metadata.
Assertions that pass whatever happens. Examples: the 9 TestComplexDataTypes tests (43 queries asserting is not None or isinstance(list)); TestSQLAlchemyAthena::test_filter_func (len(result) >= 0); TestCursor::test_callback_thread_safety; test_to_sql_with_multiple_partitions.
Parametrizations and queries beyond the contract. Examples: the sync and aio test_binary_parameters_and_literals (100 queries between them); test_throttled_columns_across_cursor_types (6 cursor types for a shared internal cursor); TestCursor::test_executemany_rowcount (3 DML kinds); TestCursor::test_binary_parameter (7 queries for 7 values).
Constructors, defaults, constants, and Python behavior. Examples: all of sqlalchemy/test_map.py; TestS3ObjectSummary::test_frozen; TestS3AioThreadPool::test_init (ThreadPoolExecutor's min(32, cpu + 4)); TestExecuteOptions::test_defaults; the test_arraysize_default copies in the aio cursors.
Tests of a library wrapper. Examples: TestArrowCursor::test_complex_as_polars, test_complex_unload_as_polars, and test_many_as_polars (pl.from_arrow); TestPolarsCursor::test_as_arrow (to_arrow).
Tests of a branch that has no caller: the abort=True branch of S3FileSystem._finish_multipart_upload (see below).

Of the flaky tests listed above, TestArrowCursor::test_complex_unload_as_polars and TestPandasCursor::test_get_csv_engine_explicit_specification are REMOVE, and test_glue.py::TestGlueMetadataClient::test_reads_what_athena_reports merges into test_cursor.py::TestCursor::test_throttled_metadata_reads_glue. TestPolarsDataFrameIterator::test_itertuples_value_python_does_not_hold has matched only "year" since 90c94d9 and is KEEP. The others are KEEP and remain for the flakiness classification.

Verification

43 REMOVE claims, from all slices, were checked by reading the removed test, its cover, and the code they exercise. 26 held, 3 held after their citation was corrected, and 14 did not. The 14 became 6 KEEP and 8 MERGE.
20 rows cited a cover that is itself removed, deleted by an open PR, or loses the case. Each was resolved in its sub-issue, for example by keeping TestS3FileSystem::test_setxattr_omits_unset_system_metadata and TestS3MultipartUploadPart::test_to_api_repr_with_checksum.
While the sub-issues were written, one more conflict came up: test_connection.py::TestConnection::test_cursor_arraysize_over_page_size was REMOVE, but the aio arraysize tests merge into it, and AioCursor has its own arraysize setter (pyathena/aio/cursor.py:96). It is KEEP.
Each PR re-checks the cover of every row it applies. A row whose cover does not hold becomes MERGE or KEEP.

Source finding

S3FileSystem._finish_multipart_upload(..., abort=True) has no caller. Its only caller, _copy_object_with_multipart_upload (pyathena/filesystem/s3.py:1706), passes abort=False and aborts the upload itself. At master, S3File.commit() has its own copy of the wait logic (s3.py:3362). #1232 (fb54bea) moves that wait into _S3WriteSession and leaves _finish_multipart_upload and its caller unchanged. Five test_finish_multipart_upload* tests exercise only the abort=True branch. #1240 removes three of them and rewrites two through the multipart copy path. Removing the parameter and the branch is a source change outside this audit.

Order

#1232 (82d2ad2) already fixes #1237: both parts of TestAioS3StreamedFile::test_write_failed_part_while_waiting meet at a threading.Barrier(2) in the upload_part mock before either fails, so #1237 can close with #1232.

#	Sub-issue	After	Reason
#1240	Sync filesystem	#1232	It changes the `TestS3File` rows and the write tests they cite.
#1241	Async filesystem and S3 core	#1232; with or after #1240	Seven async removals go with their sync twins, and two core rows rely on tests that #1240 keeps.
#1242	SQLAlchemy	—	No open PR touches these files.
#1243	Top-level	#1238	It deletes 5 rows in `test_result_set.py` and changes `test_util.py` and `tests/pyathena/util.py`.
#1244	pandas	#1238, #1233; #1245	#1238 deletes 4 rows and #1233 changes `test_read_parquet_filesystem`; #1245 adds `aio/test_common.py::TestWithAsyncFetch::test_cancel_without_query` and merges the arraysize tests.
#1245	aio and Spark	#1238; with or after #1243	#1243's MERGE and REWRITE targets cover the aio connection, binary parameter, and arraysize rows.
#1246	Polars, Arrow, S3FS	#1238, #1233	#1238 deletes 4 rows and #1233 changes 4 rows; #1236 is merged and leaves the rows unchanged.

Tests of #1232 reviewed against these criteria

#1232 (merged as d0d3b64) moved the write path of S3File onto _S3WriteSession. Before the merge, its tests were revised against the criteria of this issue and the maintainer's guidance of 2026-10-17. Mock-based local tests are fine when the logic is complex and the test tries several patterns. Coverage-only and self-evident tests are not.

Removed
- 22 TestS3WriteSession tests. They repeated a contract that the S3File tests check, or they only guarded misuse of the internal API.
- TestS3File.test_write_interrupted_while_waiting_for_part, which had the same contract as test_part_scheduling_interrupted.
- test_concurrent_aborts: no public seam can make the second abort enter while the first runs. The asynchronous file (Move AioS3StreamedFile writes onto the multipart write session #1228) is to test it through its own paths.
Kept in tests/pyathena/filesystem/test_s3_writer.py::TestS3WriteSession: only contracts that no S3File test reaches.
- A failed part's body and error are released without the garbage collector.
- A request that the executor drops fails the write, for both the creation and a part.
- Finished requests are consumed before their callbacks run, for the creation, the readiness, the completion, and the abort.
- A cancelled readiness waiter does not block the others.
Synchronization in TestS3File: the private hooks that the tests used to synchronize are gone. They now use public seams:
- the executor's submit_to;
- the futures it resolves;
- the retry stop of Stop the S3 retries of cancelled calls and of failed requests' siblings #1231.
Two private patches remain, both documented in the tests:
- a condition that signals the session's waits, which the abort ordering tests use to end the running part only once the abort waits for it;
- ready(), patched in three tests to inject an interrupt into the wait for the creation.
Against S3: test_write_transaction_rollback now also checks that no multipart upload is left and that a later commit() raises.
Evidence: each kept or rewritten test fails against a mutation that removes the behavior it names. For example, the abort ordering tests failed in 30 of 30 runs with the abort's wait removed. Codex reviewed the revision 5 times; the last review was clean.
Not covered by this review: the other tests of tests/pyathena/filesystem/test_s3.py and test_s3_async.py, which Move S3File writes onto a multipart write session #1232 only adapted to the session. They remain in scope of Remove and rewrite unnecessary tests in the sync S3 filesystem tests #1240 and Remove and rewrite unnecessary tests in the async S3 filesystem and S3 core tests #1241. test_write_failed_part_while_waiting fails intermittently since queued parts are cancelled #1237 is closed; Move S3File writes onto a multipart write session #1232 fixed it with a barrier in 82d2ad2.

Restore the tests that only they protected and bound the copy checks (G…

Remove and rewrite redundant S3 executor tests (GH-1239)

Restore the async and executor checks that round one found uncovered (G…

Wake every overlapping call once the peak is reached (GH-1239)

Restore the async checks that the independent review found uncovered (G…

Bound and tighten the repaired async and executor tests (GH-1239)

Break the reference cycle of the stopped retry in the cancelled read …

Bound the one-worker copy tests and let overlapping_calls see an exce…

Check that the async delegations pass their arguments through (GH-1239)

Fail the bounded awaits without waiting for the cancellation (GH-1239)

Compare the async delegations by everything they return or write (GH-…

Drop the test watchdog (GH-1239)

Fail the part copy once the other part runs in a waiting test (GH-1239)

Keep the running part of the abort test until the failure stops it (G…

Closing note: the flakiness classification and every sub-issue are complete.

Sub-issues: test_write_failed_part_while_waiting fails intermittently since queued parts are cancelled #1237, Remove and rewrite unnecessary tests in the sync S3 filesystem tests #1240–Document which tests the suite needs #1247, and Review the PyHive-derived DB API tests repeated for each cursor class #1260 are closed by their PRs, as are the follow-ups Make two S3FS result-set tests fail without the behavior they name #1255, Build the pandas result-set tests through the constructor #1257, and Bound the caller-thread start test and test the typed JSON conversion with hint-sensitive values #1263.
Flaky candidates of the first comment
- Removed or merged: arrow/test_cursor.py::TestArrowCursor::test_complex_unload_as_polars, pandas/test_cursor.py::TestPandasCursor::test_get_csv_engine_explicit_specification, and test_glue.py::TestGlueMetadataClient::test_reads_what_athena_reports.
- Rewritten: test_itertuples_value_python_does_not_hold matches only "year"; test_create_in_thread_cancelled_construction_error checks that the loop's exception handler receives nothing instead of counting log records; test_throttled_metadata_reads_glue compares the reported metadata through _metadata_view instead of Glue's whole parameter dictionaries; test_write_failed_part_while_waiting fails intermittently since queued parts are cancelled #1237 is closed.
The three candidates left for classification were not flaky. Both runs tested a commit under development:
- Run 38560114057 (polars/test_cursor.py::TestPolarsCursor::test_null_vs_empty_string, test_cursor.py::TestCursor::test_contain_special_character_query_with_parameter) was a workflow_dispatch of feat/python-3-15 at 9d405b8. Only the Python 3.15 job failed, before the support of Support Python 3.15 #1164 was complete.
- Run 37174556837 (arrow/test_cursor.py::TestArrowCursor::test_managed_results_match_result_file) tested Read managed query results as a CSV result file in the pandas, Arrow, and Polars cursors #1061 at 301b22e, the PR that added this test in fd80f98.
The last 10 Test workflow runs on master succeeded, through 9acbcca on 2026-10-18.

This parent issue has no closing PR of its own, so it is closed manually.

Navigation Menu

Repository navigation

Audit the test suite for unnecessary and flaky tests #1239

Summary

Facts (master `59169c5`, 2026-10-17)

Criteria

Plan

Criteria added on 2026-10-17

Baseline

Counts

Patterns

Verification

Source finding

Order

Tests of #1232 reviewed against these criteria

23 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Directory	Tests
filesystem	1,321
top-level files	798
sqlalchemy	726
pandas	574
aio	379
polars	202
arrow	157
spark	123
s3fs	116

Audit the test suite for unnecessary and flaky tests #1239

Description

Summary

Facts (master 59169c5, 2026-10-17)

Criteria

Plan

Activity

laughingman7743 commented on Oct 17, 2026

laughingman7743 commented on Oct 17, 2026

Criteria added on 2026-10-17

Baseline

Counts

Patterns

Verification

Source finding

Order

laughingman7743 commented on Oct 17, 2026

Tests of #1232 reviewed against these criteria

23 remaining items

laughingman7743 commented on Oct 18, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Facts (master `59169c5`, 2026-10-17)