Repository navigation
Remove and rewrite unnecessary top-level cursor, connection, converter, and utility tests - #1253
Conversation
Apply the rows of #1243 to the cursor, connection, converter, formatter, options, parser, result set, utility, and Glue tests. Remove tests that restate the code or repeat another test's contract, rewrite tests that asserted private state to assert results through public calls, and merge the Glue client comparison into the cursor's Glue fallback test. Rows whose cover did not fail when the behavior was broken become merges or stay: the array(varchar) of numbers only, the error without a response, the retry jitter, the retry policy copy, and the typed-to-untyped conversion fallback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The typed-to-untyped fallback test converts a value that its hint does not describe instead of a value the typed path cannot parse yet. The cache search tests tell a stopped search from a failed one and check the work group of each request. The type-hint test compares reprs, so 123 and 123.0 differ. The Glue client test keeps its check that eight threads build one client. The no-response checks of is_retryable_error get their own test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
71a34f3 to
cf0d76f
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
||
| def test_fallback_returns_untyped_result(self): | ||
| """When typed conversion returns None, untyped conversion is used.""" | ||
| """A value that the type hint does not describe is converted without the hint.""" |
There was a problem hiding this comment.
Self-review round one (implementation behavior, /code-review high and /simplify), base 713d57b, head 9daa4b1. Reviewed at c22be6c; repairs in cf0d76f and 9daa4b1.
FINDINGS, repaired in cf0d76f. The rewritten fallback test expected the raw string that the typed path returns for [{a=[1, 2]}, {b=[3]}] with array(row(a array(integer))), which made a converter limitation the expected value. It now converts {a=1, b=2} with a hint that does not describe it (array(integer)), so the untyped result is the expected value. Removing the fallback in DefaultTypeConverter.convert() fails it (mutation check, offline).
| ) | ||
|
|
||
| def test_cache_search_stops_at_expired_execution(self, caplog): | ||
| def test_cache_search_stops_at_expired_execution(self): |
There was a problem hiding this comment.
Round one, repaired in cf0d76f: _find_previous_query_id returns None for a failed search too, so None and one list request did not tell the expiry stop from an exception. The test now also searches with cache_expiration_time=3 * 3600 and expects the expired execution. Mutations that skip the stop, stop after one page, or raise in the search each fail it.
| ids=["last_searched", "after_cache_size"], | ||
| ) | ||
| def test_cache_search_reads_pages_up_to_cache_size(self, position, expected): | ||
| """With cache_size=60, the search reads the 60 latest executions over two pages.""" |
There was a problem hiding this comment.
Round one, repaired in cf0d76f: QueryExecutionHistory records each request, and the test asserts the WorkGroup of both ListQueryExecutions requests; passing another work group fails it. In 9daa4b1 the filler count of the expiry test is derived from LIST_QUERY_EXECUTIONS_MAX_RESULTS.
Deferred: the sibling cache tests (test_cache_size_different_schema, test_cache_size_different_catalog, test_cache_search_prefers_latest_execution) still patch _list_query_executions; #1243 keeps them as borderline KEEP rows.
|
|
||
| def test_complex_with_type_hints(self, cursor): | ||
| # 1. Basic complex columns from one_row_complex | ||
| # Without the hints, the numbers in the structs and the map are strings. |
| ], | ||
| [self._metadata_view(cursor.get_table_metadata(t)) for t in tables], | ||
| sorted(self._metadata_view(m) for m in cursor.list_table_metadata()), | ||
| sorted(m.name for m in cursor.list_table_metadata(expression="one_row")), |
There was a problem hiding this comment.
Round one, repaired in cf0d76f: the listing filtered by expression is compared sorted. Whether Athena and Glue apply Expression alike is checked by the pending live run.
|
|
||
| def contended_client(*args, **kwargs): | ||
| created.append((args, glue._lock.locked())) | ||
| created.append(args) |
There was a problem hiding this comment.
Round one, repaired in cf0d76f: the rewrite had dropped the eight-thread check, which left the lock of GlueMetadataClient.client without a test. The check is kept without the private glue._lock.locked(); removing the lock fails it (mutation check, offline).
|
|
||
|
|
||
| @pytest.mark.parametrize("error", [ValueError("no response"), _NoResponseError()]) | ||
| def test_is_retryable_error_without_response(error): |
There was a problem hiding this comment.
Round one, repaired in cf0d76f: the no-response checks ran once per parameter of test_is_retryable_error; they are their own test, parametrized over ValueError and an error whose response is None.
| execute_signature.bind(cursor, operation="SELECT 1") | ||
| with pytest.raises(TypeError, match="positional"): | ||
| cursor.execute("SELECT %(value)s", parameters, "group") | ||
| with pytest.raises(TypeError): |
There was a problem hiding this comment.
Round one, deferred with reasons:
pytest.raises(TypeError)withoutmatch="positional": the message is CPython's, whichdocs/testing.mddoes not count as a contract. Mutations that accept a positional setting inConnection,Cursor,AsyncCursor,DictCursor, andCursor.execute()fail the tests.test_cursor_execute_keyword_onlyopens a cursor per class (about 3 s offline). A real call is kept as the evidence: the positional call reaches AWS or a future in the mutated case.test_binary_parameterloses its seven test IDs: Remove and rewrite unnecessary tests in the top-level cursor, connection, converter, and utility tests #1243 rewrites it to one query.
|
|
||
|
|
||
| # Athena's answers to a query or calculation that fails at its first status request. | ||
| FAILED_EXECUTION_RESPONSES = { |
There was a problem hiding this comment.
Round one, repaired in 9daa4b1: the failed QueryExecution stub keeps only the fields that AthenaQueryExecution needs.
Deferred: TestGlueMetadataClient::test_rejects_an_unsupported_catalog stays removed; the ValueError guard of the internal client is not reachable through the cursor, which checks usable_for first (#1243 criterion 7).
| ], | ||
| [self._metadata_view(cursor.get_table_metadata(t)) for t in tables], | ||
| sorted(self._metadata_view(m) for m in cursor.list_table_metadata()), | ||
| sorted(m.name for m in cursor.list_table_metadata(expression="one_row")), |
There was a problem hiding this comment.
Self-review round two (claims, callers, AWS operation, evidence), base 713d57b, head 9daa4b1. FINDINGS in the PR description only; corrected.
Claims checked:
- No file under
pyathena/changes:git diff --name-onlylists onlytests/. - Counts were stale after the round-one repairs: 299 test functions and 665 collected tests, not 298 and 663; corrected.
- About 75 fewer Athena queries:
executemany_tableruns 3 statements and each removedtest_executemany_rowcountcase 7 more (20 for two cases); 43 + 20 + 6 + 2 + 3. The addedview_one_rowandexpressionreads add 4 metadata requests; the description now says so. - The fallback row described the round-one test; it now describes the
rowvalue with a non-matching hint. on_start_query_executionruns only for the synchronous and aio SQL cursors:docs/usage.md"Supported cursor types";AsyncCursorand the Spark cursors do not call_call_on_start_query_execution.- The keyword-only contract names
connect(),aio_connect(), the connection classes, and the cursor classes (docs/usage.md, "Keyword arguments in PyAthena 4.0"), not the abstract bases. _without_retriesis used only bypyathena/sqlalchemy/base.py.- The documented defaults:
RetryConfignote (127 s of waits with the defaults),ExecuteOptionsattributes ("0 (default)"), and "ExecuteOptionsis immutable" indocs/usage.md.
AWS operation: test_throttled_metadata_reads_glue compares Glue parameters with Athena's, which #1239 lists as an occasional failure; the merge adds one table to that comparison. The live run is pending, so the description records local and offline evidence only.
Restore the DictCursor NULL and empty-string test, whose result set converts its own rows, and the test that merge() leaves the original options unchanged, as docs/usage.md states. Add native date and timestamp arrays to the live complex-value query and the parser test. Compare the type-hinted row with ==, plus the types of its values, so the order of map entries does not matter. Round the jitters, and let the build-once checks wait for a second build. Answer each stubbed request once, and reject a ListQueryExecutions page above 50, as Athena does. Pass connect() one positional argument, so a factory that accepts one is caught. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| ,ARRAY['x', ' ', 'a,b'] AS col_space_comma | ||
| ,ARRAY[json_parse('1234567890'), json_parse('"a,b"')] AS col_json | ||
| ,ARRAY[DATE '2023-01-01', DATE '2023-12-31'] AS col_date | ||
| ,ARRAY[CAST('2023-01-01 12:00:00.123' AS TIMESTAMP(3))] AS col_timestamp |
There was a problem hiding this comment.
Independent review (relayed): Codex CLI codex exec -s read-only, codex-cli 0.163.0, model gpt-6-astra, session 01a149e0-ff11-7e53-bf88-a81c1c84e361. Static review of a detached snapshot at 9daa4b1 against merge-base 713d57b; no tests, builds, or GitHub access; the prompt left out the PR number, description, commit messages, and earlier findings. The snapshot was unchanged afterwards. Result: FINDINGS (11). Repairs in c94aac0.
-
P2:
AthenaDictResultSetconverts its own rows (pyathena/result_set.py:964), soTestDictCursor::test_null_vs_empty_stringhad no cover; turning''into None would pass every remaining DictCursor test. Repaired: the test is restored (test_cursor.py:1913, identical to the base). -
P2: removing
TestComplexDataTypes::test_array_types_basicleft native DATE/TIMESTAMP arrays without a test; treating:as unsafe in_parse_array_nativereturns the original string. Repaired: the livetest_fetch_complex_valuesselects a date array and aTIMESTAMP(3)array, andtest_to_array_athena_native_formatshas both shapes; the:mutation fails the offline test.
There was a problem hiding this comment.
Independent follow-up (relayed) on point 10: the offline test_to_array_athena_native_formats cases repeated the contract of the live test_fetch_complex_values columns; docs/testing.md counts them as different evidence only for opt-in live tests or cases Athena cannot produce. Repaired in 3fa2d7e: the offline cases are removed; the live query keeps col_date and col_timestamp. The follow-up found points 1, 3, 4, 7, and 11 closed and the answered_requests() exit check sound.
| if multiplier: | ||
| # Random, so the six waits do not add the same jitter. Rounded, so that | ||
| # the float residuals of a constant jitter do not differ. | ||
| assert len({round(jitter, 6) for jitter in jitters}) > 1 |
There was a problem hiding this comment.
- P2: with a constant jitter such as
multiplier / 10, the float residuals ofwaited - basediffer, solen(set(jitters)) > 1passed. Repaired: the jitters are rounded to 6 digits; the constant-jitter mutation fails the test.
There was a problem hiding this comment.
Independent follow-up (relayed): Codex CLI, gpt-6-astra, static review of c94aac0 against 9daa4b1 with the repair diff; FINDINGS. A constant jitter of min(multiplier, 0.0000005) puts the residuals on both sides of the 6-digit rounding, so point 2 stayed open. Repaired in 3fa2d7e: the test asserts that the six jitters spread over more than multiplier / 100. A constant jitter of multiplier / 10 and one of min(multiplier, 0.0000005) both fail it (mutation checks, offline). Six uniform jitters spread over less than a hundredth of the range with a probability of about 6e-10.
There was a problem hiding this comment.
Second independent follow-up (relayed): Codex CLI, gpt-6-astra, static review of 3fa2d7e against c94aac0 with the repair diff. Self-review of the repair before it, both perspectives: the spread check fails a constant jitter of multiplier / 10 and one of min(multiplier, 0.0000005); the PR description's counts (301 test functions, 667 collected tests) and table were corrected. Codex result: the removal of the offline temporal-array cases is CLEAN; one P3 finding: six valid uniform jitters spread over less than multiplier / 100 with a probability of about 6e-10 per case, which would fail the test. Accepted as is: the probability is far below that of the service errors the suite already reruns, and fixing the random draws would hide the randomness the test checks.
| assert data[1]["vel"]["x"] == 1.5 | ||
| assert row == expected | ||
| # == does not tell 123 from 123.0. | ||
| assert _types(row) == _types(expected) |
There was a problem hiding this comment.
- P2: the
reprcomparison required the map entries' order, which Athena does not define. Repaired:==plus_types(), the types of the values in the shape of the row, which compares dictionaries without order.
| def test_merge_raises_on_unknown_field(self): | ||
| with pytest.raises(TypeError, match="unknown_field"): | ||
| ExecuteOptions().merge(unknown_field="value") | ||
| def test_merge_ignores_none_overrides(self): |
There was a problem hiding this comment.
- P2: no remaining test checked that
merge()returns a new instance and leaves the original unchanged, whichdocs/usage.mdstates. Repaired:TestExecuteOptions::test_merge_does_not_mutate_originalis restored (test_options.py:33, identical to the base).
| @pytest.mark.parametrize("cursor_class", CURSOR_CLASSES) | ||
| def test_cursor_execute_keyword_only(offline_connection, cursor_class): | ||
| with _open_cursor(offline_connection, cursor_class) as cursor, pytest.raises(TypeError): | ||
| cursor.execute("SELECT %(value)s", {"value": 1}, "group") |
There was a problem hiding this comment.
- P2: no remaining synchronous test calls
execute(operation=...). Rejected:docs/usage.mddocuments the operation and parameters as positional and the other settings as keyword-only; it does not document the keyword form, and Remove and rewrite unnecessary tests in the top-level cursor, connection, converter, and utility tests #1243 rewrites the test to the rejection of a positional setting.
Also repaired in this commit, from the #1245 work: test_connect_keyword_only passes one positional argument. With two, a factory that accepted one positional setting still raised TypeError for the second.
| created.append(args) | ||
| # Without the lock, another thread would build a client meanwhile. | ||
| if len(created) == 1: | ||
| second_build.wait(1) |
There was a problem hiding this comment.
- P2: removing the lock still passed if one thread finished the build before the others entered. Repaired: the first build waits up to 1 s for a second build, so without the lock a second thread builds meanwhile; with the lock no second build happens and the wait times out. Removing the S3 or the Glue lock fails the tests.
There was a problem hiding this comment.
Independent follow-up (relayed): FINDINGS. Without the lock, the 1 s wait for a second build still passes if every other released thread stays descheduled until the first client is cached. Deferred with reasons: without the private lock, the test cannot observe contention inside the property, and #1243 removes the private _s3_client_lock.locked() / glue._lock.locked() checks. The test now misses a missing lock only if all seven released threads stay descheduled for a full second; removing either lock failed it in each offline run.
| assert cursor._on_start_query_execution is on_start_query_execution | ||
| assert cursor._on_poll is on_poll | ||
| with ( | ||
| answered_requests(offline_connection.client), |
There was a problem hiding this comment.
- P2: the dictionary stub answered
GetQueryExecutionfor ever, so a cursor that kept polling would hang. Repaired:answered_requests()intests/pyathena/util.pyanswers each operation once; a repeat raisesAssertionError, and the block raises it again at exit when the cursor caught it. Treating FAILED as not terminal failstest_cursor_accepts_connection_callbacks[Cursor]in about 1 s.
|
|
||
| def test_fallback_returns_untyped_result(self): | ||
| """When typed conversion returns None, untyped conversion is used.""" | ||
| """A value that the type hint does not describe is converted without the hint.""" |
There was a problem hiding this comment.
- P2:
{unclosedwithrow(a integer)lost its test. Rejected: Athena does not return an unframed row value; the removed test also asserted only that both paths return None (Remove and rewrite unnecessary tests in the top-level cursor, connection, converter, and utility tests #1243 criteria 5 and 7).
|
|
||
| def contended_client(*args, **kwargs): | ||
| created.append((args, glue._lock.locked())) | ||
| created.append(args) |
There was a problem hiding this comment.
- P2: the
ValueErrorofGlueMetadataClientfor an unsupported catalog is untested. Rejected: the cursor checksusable_forbefore it calls the client, so no caller reaches the guard (Remove and rewrite unnecessary tests in the top-level cursor, connection, converter, and utility tests #1243 criterion 7); round one recorded the same deferral.
| """ | ||
| self.list_requests.append(request) | ||
| # ListQueryExecutions rejects a MaxResults above 50. | ||
| if request["MaxResults"] > 50: |
There was a problem hiding this comment.
- P3: the history stand-in accepted pages above Athena's limit of 50. Repaired: a
MaxResultsabove 50 raisesInvalidRequestException;LIST_QUERY_EXECUTIONS_MAX_RESULTS = 51now failstest_cache_search_reads_pages_up_to_cache_size.
| """ | ||
| self.list_requests.append(request) | ||
| # ListQueryExecutions rejects a MaxResults above 50. | ||
| if request["MaxResults"] > 50: |
There was a problem hiding this comment.
Self-review of the repair c94aac0, both perspectives; narrow scope by git range-diff 713d57bd..9daa4b12 713d57bd..c94aac06 (one new commit, earlier commits unchanged). Base 713d57b, head c94aac0. CLEAN.
Round one (behavior):
answered_requests()re-raises a swallowed repeat at exit; an exception in the block propagates through the generator, so the exit check does not mask it.- The 1 s wait for a second build bounds the passing path at 1 s per test.
_types()compares tuples and lists alike on both sides.- The hinted second query of
test_fetch_complex_valuesleaves the new date and timestamp columns unhinted, so both queries expect the same row. - Offline: the changed files pass (544 passed); the six repair mutations fail their tests.
Round two (claims): the PR description now lists the three restored or moved rows. The counts are 301 test functions and 669 collected tests, and 2 single-query removals rather than 3. The repair commit message matches the diff. The Athena rendering of [2023-01-01, 2023-12-31] and [2023-01-01 12:00:00.123] is unverified until the live run.
…cases The live complex-value query already checks the native date and timestamp arrays, so the offline parser cases repeated its contract. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WHAT
Apply the rows of #1243 to
tests/pyathena/test_cursor.py,test_connection.py,test_converter.py,test_formatter.py,test_options.py,test_parser.py,test_result_set.py,test_util.py,test_glue.py, and the helpers intests/pyathena/util.py. This diff changes no file underpyathena/.TestComplexDataTypes, the tests that assert private state or class bases, and the tests whose contract another test checks with the same kind of evidence.TestCursor::test_binary_parameterandtest_complex_with_type_hintsto select their values in one query each (7 and 3 queries to 1), and keep onlytest_executemany_rowcount[update].QueryExecutionHistory, a client stand-in intests/pyathena/util.pythat pagesListQueryExecutionsbyMaxResults. They assert the search result and the number of list requests instead of the arguments of the private_list_query_executions.test_cursor_accepts_connection_callbacksto run each cursor'sexecute()against stubbed responses for a failed query or calculation.answered_requests()intests/pyathena/util.pyanswers each request once, so a cursor that kept polling fails instead of hanging. It asserts thaton_pollruns for every cursor and thaton_start_query_executionruns only for the synchronous SQL cursors, asdocs/usage.mdstates.TypeError.execute()andconnect()/Connectionare called. The constructors are bound withinspect.signature(cursor_class).bind(...): a call throughConnection.cursor()passes every setting by keyword, so a positional setting there also fails as a duplicate keyword; such a call did not fail the test whenCursororAsyncCursoraccepteds3_staging_dirpositionally. The abstractBaseCursor,SparkBaseCursor, andWithResultSetare no longer listed; the documented contract coversconnect(), the connection classes, and the cursor classes.TestGlueMetadataClient::test_reads_what_athena_reportsintoTestCursor::test_throttled_metadata_reads_glue, which now also readsview_one_rowand a listing filtered byexpression.test_to_struct_athena_complex_casesassert exact values. The three inputs parse into dictionaries, not into the original string that Remove and rewrite unnecessary tests in the top-level cursor, connection, converter, and utility tests #1243 expected.TestGlueMetadataClient::test_client_leaves_out_athena_endpointwithout the privateglue._lock.locked(), asTestConnection::test_s3_client_built_once_across_threadsdoes for S3; no other test checks that the connection builds one Glue client. In both, the first build waits up to 1 s for a second one, so a missing lock fails them regardless of thread scheduling. Move the no-response checks oftest_is_retryable_errortotest_is_retryable_error_without_response.These rows changed after their cover was re-checked by breaking the behavior in the source:
test_converter.py::TestDefaultTypeConverter::test_array_varchar_keeps_stringstest_array_varchar_prevents_number_inference[1234, 5678, hello]still gives strings, so the cover passed;[1234, 5678]is now its parameter.test_util.py::test_retry_api_call_with_none_errortest_is_retryable_errorresponseisNone.test_util.py::test_retry_api_call_jitter_variestest_retry_api_call_waits_with_jittertest_util.py::test_without_retries_without_retries; only the SQLAlchemy dialect does, and no other test checks that the attempts and waits are copied.test_converter.py::TestDefaultTypeConverter::test_fallback_returns_untyped_resultDefaultTypeConverter.convert()was removed. The test now converts arowvalue with a hint that does not describe it.test_util.py::TestRetryConfig::test_default_attempts,test_options.py::TestExecuteOptions::test_defaults,test_is_frozenRetryConfignote on the default waits, theExecuteOptionsattributes, and "ExecuteOptionsis immutable" indocs/usage.md), whichdocs/testing.mdcounts as contracts.test_cursor.py::TestDictCursor::test_null_vs_empty_stringAthenaDictResultSetconverts its own rows (pyathena/result_set.py), so no other test fails when it turns''into None.test_options.py::TestExecuteOptions::test_merge_does_not_mutate_originaldocs/usage.mdstates thatmerge()returns a new instance; no other test checks the original afterwards.test_cursor.py::TestComplexDataTypes::test_array_types_basicTestCursor::test_fetch_complex_valuesselects them.test_options.py::TestExecuteOptions::test_merge_raises_on_unknown_fieldoptions=ExecuteOptions(...)parameter oftest_resolve_rejects_unknown_non_none_fieldsalready callsmerge().The change removes about 75 Athena queries per run: 43 from
TestComplexDataTypes, 20 fromtest_executemany_rowcount[insert]/[delete](10 each, with the Iceberg table of theexecutemany_tablefixture), 8 from the binary and type-hint rewrites, and 2 from single-query removals.test_throttled_metadata_reads_gluesends 4 more metadata requests (2 to Athena, 2 to Glue), and the removedtest_glue.pyand Glue-fallback tests sent about 14.WHY
Closes #1243. Part of #1239: these tests restated the code, asserted private state, or repeated a contract that another test checks with the same kind of evidence, as described in
docs/testing.md.TEST
Python 3.15.0, at 3fa2d7e (on 713d57b).
just format: no changes.just lint: passed.--noconftestand placeholder settings,tests/pyathena/test_{cursor,connection,converter,formatter,options,parser,result_set,util,glue}.py: 542 passed, 4 skipped; the 5 failed and 77 errors are tests that need AWS or its fixtures.--noconftestand placeholder settings; every source mutation was reverted andgit diff -- pyathena/was empty after each run. Each mutation named in the rows, and in the self-review repairs, failed the kept, merged, or rewritten test that covers it, except where the table above records a change.TestComplexDataTypes,test_timestamp_result_precision,test_execute_qmark_parameters_skip_cache,test_callback_thread_safety,test_wrapped_glue_throttling_reads_glue, and the threetest_glue.pyremovals) were checked by reading the covers, not by mutation; the covers passed in the live run.uv run --env-file .env pytest -n 4over the changed test files of both PRs (tests/pyathena/test_{cursor,connection,converter,formatter,options,parser,result_set,util,glue}.py, the changedtests/pyathena/aio/files and directories, andtests/pyathena/spark/): 965 passed, 1 skipped, in 275 s.🤖 Generated with Claude Code