Skip to content

Remove and rewrite unnecessary tests in the top-level cursor, connection, converter, and utility tests #1243

New issue
New issue
Closed
#1253
Closed
Remove and rewrite unnecessary tests in the top-level cursor, connection, converter, and utility tests#1243
#1253
Labels
enhancement

Description

@laughingman7743
laughingman7743
opened on Oct 17, 2026

Summary

Part of #1239. Files: test_cursor.py, test_async_cursor.py, test_connection.py, test_converter.py, test_formatter.py, test_model.py, test_options.py, test_parser.py, test_result_set.py, test_util.py, test_glue.py. At master 59169c5 with the cross-check corrections: 366 test functions: 277 KEEP, 70 REMOVE, 4 MERGE, 15 REWRITE. #1238 deletes 5 of the REMOVE tests, so this PR applies 65 REMOVE. The change saves about 75 Athena queries per run (43 from TestComplexDataTypes, about 20 from TestCursor::test_executemany_rowcount, 6 from TestCursor::test_binary_parameter, 2 from TestCursor::test_complex_with_type_hints, 3 from single-query removals) and about 12 Athena or Glue metadata requests (test_glue.py merge and removals, TestCursor::test_wrapped_glue_throttling_reads_glue). No original test is changed.

Paths are relative to tests/pyathena/. [original] marks a test that existed at 21cbf05 (v3.15.0).

Order

  1. After Break result sets after failed or cancelled fetches and drop the cross-thread guards #1238 (06adf23). It deletes test_result_set.py::TestAthenaResultSet::{test_fetch_with_close_during_take, test_fetch_with_close_during_request, test_fetch_with_close_during_retries, test_fetch_all_rows_with_close_during_request, test_fetch_all_rows_with_close_during_retries} (REMOVE here), TestAthenaResultSet::test_fetch_counts_rows_taken_before_error and test_util.py::test_retry_api_call_stopped_by_stop_when (KEEP here), and the helpers _ClosingDeque, close_during_next_request, close_during_next_retries. It adds TestAthenaResultSet::test_fetch_after_failed_fetch. test_result_set.py, test_util.py and util.py are rebased on it.
  2. The aio and Spark sub-issue depends on rows here. It merges its copies into the results of this PR, so it lands with or after this one:
    • aio/test_connection.py::test_empty_work_group_disables_environment_fallback relies on the Connection work_group="" case kept by the test_environment_fallback MERGE (section 2).
    • aio/test_connection.py::{test_connect_keyword_only, test_connection_constructor_keyword_only, test_cursor_constructor_keyword_only, test_cursor_execute_keyword_only, test_cursor_unknown_constructor_keyword, test_cursor_accepts_connection_callbacks} merge into the rewritten keyword-only and callback parametrizations (section 2).
    • aio/test_cursor.py::TestAioCursor::test_binary_parameter relies on the one-query rewrite of TestCursor::test_binary_parameter (section 1).
    • aio/test_cursor.py::TestAioCursor::test_executemany_rowcount drops [insert]/[delete] as this file does; both cite TestCursor::test_iceberg_table [original] and arrow/test_cursor.py::TestArrowCursor::test_iceberg_table for the per-statement rowcounts.
    • spark/test_common.py::TestSparkBaseCursor::test_calculate_interrupt_without_kill_on_interrupt cites TestCursor::test_execute_interrupted_while_starting_without_kill_on_interrupt, which stays (KEEP).
    • aio/test_cursor.py::TestAioCursor::{test_cache_size_different_schema, test_cache_size_different_catalog, test_cache_search_prefers_latest_execution} cite the TestCursor tests of the same names (KEEP). The matching is shared: _match_previous_query (pyathena/common.py:1122) is called from pyathena/aio/common.py:357.
    • The aio arraysize tests merge into test_connection.py::TestConnection::test_cursor_arraysize*, including test_cursor_arraysize_over_page_size, which therefore stays (KEEP). AioCursor has its own arraysize setter (pyathena/aio/cursor.py:96), and the aio sub-issue removes TestAioCursor::test_invalid_arraysize.
    • aio/test_util.py::test_async_retry_api_call_retries cites test_util.py::test_retry_metadata_errors (KEEP), not test_retry_api_call.
  3. Other sub-issues cite rows here: the SQLAlchemy sub-issue (sqlalchemy/test_base.py::TestSQLAlchemyAthena::{test_select_nested_struct_query, test_select_array_with_nested_struct}) relies on the rewritten TestCursor::test_complex_with_type_hints; the Polars, Arrow and S3FS sub-issue (s3fs/test_cursor.py::TestS3FSCursor::test_on_start_query_execution) relies on the rewritten test_connection.py::test_cursor_accepts_connection_callbacks.

Findings

1. test_cursor.py

test change criteria reason covered by
TestComplexDataTypes::test_struct_types, test_array_types, test_map_types, test_complex_combinations, test_array_types_basic, test_array_types_with_structs, test_array_types_json_cast, test_array_operations, test_array_converter_behavior REMOVE 4, 5, 6, 2 43 Athena queries that mostly assert is not None or isinstance(list), log instead of failing, call the private _to_struct/_to_map/_to_array, or test Athena's own array functions. TestCursor::test_complex [original], TestCursor::test_complex_with_type_hints (after its rewrite), TestCursor::test_fetch_complex_values; test_converter.py::test_to_array_*, test_to_struct_*, test_to_map_*
TestCursor::test_timestamp_result_precision REMOVE 2 TIMESTAMP(0) and TIMESTAMP(9) results are columns ts_p0 and ts_p9 of the temporal query. TestCursor::test_temporal_types; test_converter.py::test_to_datetime_any_precision
TestCursor::test_execute_qmark_parameters_skip_cache REMOVE 1, 2 Patches the private _build_start_query_execution_request/_find_previous_query_id for the #941 contract that the integration test proves with real reuse checks. TestCursor::test_cache_size_with_qmark_parameters
TestCursor::test_callback_thread_safety REMOVE 2, 3, 5 The helper thread asserts only if stored_query_id, so nothing fails if the callback did not run in time. TestCursor::test_execute_with_callback
TestCursor::test_on_poll_invoked_each_iteration REMOVE 1, 2 Calls the private _poll with a patched _get_query_execution; the per-iteration on_poll calls are asserted on the public execute() path. TestCursor::test_execute_kill_on_interrupt, TestCursor::test_on_poll_connection_level, TestCursor::test_on_poll_cursor_level
TestCursor::test_on_poll_none_is_noop REMOVE 6, 7 "A None callback does nothing" on the private _poll; every integration test runs with on_poll=None. every TestCursor integration test
TestCursor::test_wrapped_glue_throttling_reads_glue REMOVE 2 Same input and assertions as the stricter test, under a default policy that does not retry MetadataException anyway. TestCursor::test_wrapped_glue_throttling_skips_metadata_exception_retries
TestDictCursor::test_null_vs_empty_string REMOVE 2, 4 DictCursor uses the same converter; only the row shape differs. TestCursor::test_null_vs_empty_string, TestDictCursor::test_fetchone [original]
TestCursor::test_binary_parameter REWRITE 4 Assert the seven values in one SELECT CAST(%(v0)s AS VARBINARY), ... and compare the one row (7 queries to 1). formatting: test_formatter.py::TestDefaultParameterFormatter::test_format_binary
TestCursor::test_complex_with_type_hints REWRITE 4 Select the three hinted expressions in one query from one_row_complex and assert the row by exact equality, which makes the isinstance lines unnecessary (3 queries to 1). —
TestCursor::test_executemany_rowcount[insert], [delete] REWRITE 2, 4 Keep only [update]: the rowcount sum does not depend on the DML kind (about 20 fewer queries). per-statement rowcounts: TestCursor::test_iceberg_table [original], arrow/test_cursor.py::TestArrowCursor::test_iceberg_table
TestCursor::test_cache_search_stops_at_expired_execution REWRITE 1 Assert that the result is None and that one page was listed, instead of the private _list_query_executions kwargs (max_results=50) and the absence of log text. —
TestCursor::test_cache_search_reads_pages_up_to_cache_size REWRITE 1 Assert the match on page two and that a match after cache_size executions is not found, instead of the call_args_list of the private _list_query_executions. —

2. test_connection.py

test change criteria reason covered by
test_environment_fallback, test_empty_settings_disable_both_environment_fallbacks MERGE 2 pyathena.connect(**kwargs) returns Connection(**kwargs), so the factory parametrization runs each case twice; drop the pyathena.connect param (16 cases to 8, 2 to 1), keeping the Connection work_group="" case. into the Connection params of the same tests
test_connect_keyword_only, test_cursor_constructor_keyword_only, test_cursor_execute_keyword_only REWRITE 6, 1 Assert only that a positional argument raises TypeError for each factory and class (the 4.0.0 keyword-only contract), instead of the literal inspect.signature, a mocked Session, and bind() checks. —
test_cursor_accepts_connection_callbacks REWRITE 1 Assert that the connection's callbacks run on a stubbed StartQueryExecution/GetQueryExecution, instead of the private _on_start_query_execution/_on_poll attributes. —
TestConnection::test_s3_client_built_once_across_threads REWRITE 1 Assert one session.client("s3") call and one shared client across 8 threads; drop the private _s3_client_lock.locked() element. —
TestConnection::test_close_closes_built_clients REWRITE 1 Assert that close() closes every client that was built; drop the private conn._glue._client is None and conn._s3_client is None checks. —

3. test_converter.py

test change criteria reason covered by
test_nested_json_empty_string REMOVE 2 Both cases ('[""]' as array(json), "" in map(varchar, json)) are params of the broader test. test_typed_json_elements
TestDefaultTypeConverter::test_struct_conversion REMOVE 2 The same JSON, None, empty, invalid, and native inputs as the module-level _to_struct tests, through a one-line dispatch. test_to_struct_json_formats, test_to_struct_athena_native_formats
TestDefaultTypeConverter::test_array_conversion REMOVE 2 The same inputs as the module-level _to_array tests. test_to_array_json_formats, test_to_array_athena_native_formats
TestDefaultTypeConverter::test_array_varchar_keeps_strings REMOVE 2 A subset of the input of the covering test. TestDefaultTypeConverter::test_array_varchar_prevents_number_inference, test_native_array_items
TestDefaultTypeConverter::test_array_integer_converts_to_int, test_array_boolean_converts, test_array_with_null REMOVE 2, 5 The untyped JSON path gives the same result, so they pass without type-hint support; the typed cases are params of the compact-array test. test_typed_compact_array
TestDefaultTypeConverter::test_map_varchar_integer, test_row_type_hint REMOVE 5, 2 The JSON input decodes to the asserted ints without the hint, and a broken typed branch falls back to _to_map. TestDefaultTypeConverter::test_map_native_format_with_hints, TestDefaultTypeConverter::test_row_native_format_with_hints; JSON branch: test_parser.py::TestTypedValueConverter::test_map_varchar_integer (KEEP)
TestDefaultTypeConverter::test_simple_type_hint REMOVE 6 "hello" as varchar returns "hello". —
TestDefaultTypeConverter::test_type_hint_caching, test_hive_syntax_caching REMOVE 1 Assert the private _parsed_hints cache dict. —
TestDefaultTypeConverter::test_fallback_on_malformed_value, test_fallback_preserves_struct_value, test_fallback_returns_untyped_result REMOVE 5, 7, 2 Their own comments say that both paths return None or the typed path succeeds, so no fallback is exercised. test_to_array_invalid_formats; test_parser.py::TestTypedValueConverter::test_struct_json_name_based_type_matching
TestDefaultTypeConverter::test_hive_syntax_through_converter REMOVE 5, 2 "[1, 2, 3]" with array<int> passes without the hint. TestDefaultTypeConverter::test_normalize_hive_syntax_nested
TestDefaultTypeConverter::test_hive_syntax_struct_through_converter REMOVE 2 The same input, hint, and expectation as the covering test. TestDefaultTypeConverter::test_normalize_hive_syntax_struct
TestDefaultTypeConverter::test_normalize_hive_syntax_noop, test_normalize_hive_syntax_replaces REMOVE 1, 7, 2 Unit tests of the private static _normalize_hive_syntax; the documented Hive-syntax contract is tested through convert(). TestDefaultTypeConverter::test_normalize_hive_syntax_nested, test_normalize_hive_syntax_struct, test_normalize_hive_syntax_mixed
test_to_struct_athena_complex_cases REWRITE 5 Assert the exact expected value (the original string) instead of result == input_value or isinstance(result, dict), which accepts any dict. —
TestDefaultTypeConverter::test_normalize_hive_syntax_map REWRITE 5 Use the native {x=1, y=2} so that map<string,int> decides the int values; the JSON '{"x": 1, "y": 2}' gives ints without the hint. —

4. test_formatter.py

test change criteria reason covered by
test_escape_trino_doubles_single_quotes REWRITE 1, 7 Assert its unique case, a backslash before a quote stays literal, through formatter.format("SELECT %(v)s", {"v": "a\\'b"}) instead of the private _escape_trino. quote doubling: TestDefaultParameterFormatter::test_quote_cannot_terminate_literal_in_delete

5. test_options.py

test change criteria reason covered by
TestExecuteOptions::test_defaults REMOVE 6 Asserts the dataclass field defaults. —
TestExecuteOptions::test_is_frozen REMOVE 6 Tests @dataclass(frozen=True), which is Python's behavior. —
TestExecuteOptions::test_merge_does_not_mutate_original, test_resolve_given_options_preserves_new_instance_behavior, test_resolve_returns_distinct_default_instances REMOVE 6, 1 Instance identity and non-mutation of a frozen dataclass through replace(). —
TestExecuteOptions::test_resolve_preserves_callback_and_hint_references REMOVE 1, 7 Asserts that resolve() does not copy the dicts, an identity detail. —
TestExecuteOptions::test_merge_overrides_take_precedence REMOVE 2 Override precedence, including False and 0, is the parametrized test. TestExecuteOptions::test_merge_precedence
TestExecuteOptions::test_merge_without_applied_overrides_is_equal REMOVE 2, 6 None overrides are ignored, which the covering test asserts. TestExecuteOptions::test_merge_ignores_none_overrides
TestExecuteOptions::test_resolve_with_none_returns_defaults_with_overrides REMOVE 2 The same as the {"work_group": "primary"} param of the covering test. TestExecuteOptions::test_resolve_without_base_preserves_override_values
TestExecuteOptions::test_merge_raises_on_unknown_field MERGE 2 resolve(options, ...) calls merge(), so this is the options=ExecuteOptions(...) param of the target. into TestExecuteOptions::test_resolve_rejects_unknown_non_none_fields

6. test_parser.py

test change criteria reason covered by
TestTypeSignatureParser::test_parameterized_type, test_varchar_with_length REMOVE 2 Exactly the decimal(10, 2) and varchar(255) params of the parametrized test. TestTypeSignatureParser::test_parameterized_type_keeps_suffix
TestTypeSignatureParser::test_array_of_integer REMOVE 2 The same shape as array(varchar) and array(int). TestTypeSignatureParser::test_array_type, TestTypeSignatureParser::test_type_alias_in_complex_type
TestTypeSignatureParser::test_trailing_modifier_after_paren REMOVE 7 Its docstring calls decimal(10, 2) extra hypothetical; the real suffix case is tested. TestTypeSignatureParser::test_parameterized_type_keeps_suffix[time(3) with time zone]
TestTypedValueConverter::test_simple_varchar, test_simple_integer REMOVE 6, 2 "hello" returns "hello", "42" returns 42. test_converter.py::test_typed_compact_array
TestTypedValueConverter::test_array_of_varchar, test_array_of_integer, test_row_named_fields, test_nested_row REMOVE 2 (5 for test_array_of_integer) The same inputs through the internal TypedValueConverter as through the public DefaultTypeConverter.convert(); test_nested_row and test_row_named_fields have identical inputs. test_converter.py::TestDefaultTypeConverter::test_array_varchar_prevents_number_inference, test_converter.py::test_typed_compact_array, test_converter.py::TestDefaultTypeConverter::test_row_native_format_with_hints, test_converter.py::TestDefaultTypeConverter::test_row_with_nested_struct

7. test_result_set.py

#1238 deletes TestAthenaResultSet::{test_fetch_with_close_during_take, test_fetch_with_close_during_request, test_fetch_all_rows_with_close_during_request, test_fetch_with_close_during_retries, test_fetch_all_rows_with_close_during_retries}; they are not listed.

test change criteria reason covered by
TestAthenaResultSet::test_get_query_results_after_close REMOVE 7, 2 Tests the guard of the private _get_query_results and its message text; the public behavior after close is tested. TestAthenaResultSet::test_fetch_after_close
TestWithResultSet::test_is_mixin, test_precedes_cursor_bases REMOVE 6, 1 Assert __bases__ tuples, which restate the class statements. —

8. test_util.py

test change criteria reason covered by
test_retry_api_call, test_retry_api_call_with_none_error REMOVE 5, 2 With attempt=1 the call count is 1 whether or not the error is retryable, so they pass with a broken retry predicate. retry until success: test_retry_metadata_errors (rows with 2 calls); non-retryable: test_retry_metadata_errors (rows with 1 call); no-response error: test_is_retryable_error
TestRetryConfig::test_default_attempts REMOVE 6 Asserts the default constant 8. —
test_retry_api_call_jitter_varies REMOVE 6 Tests that tenacity's wait_random is random; the bounds that PyAthena composes are the covering test's. test_retry_api_call_waits_with_jitter
test_without_retries REMOVE 7, 2 Field copying in the private _without_retries; its use, the Glue fallback skipping MetadataException retries, is tested through the cursor. test_cursor.py::TestCursor::test_wrapped_glue_throttling_skips_metadata_exception_retries, test_cursor.py::TestCursor::test_other_metadata_exception_keeps_its_retries

9. test_glue.py

test change criteria reason covered by
TestGlueMetadataClient::test_reads_what_athena_reports MERGE 2, 4 The same Glue-versus-Athena comparison as the cursor test (about 11 metadata requests); add view_one_row and the expression filter to the target. into test_cursor.py::TestCursor::test_throttled_metadata_reads_glue
TestGlueMetadataClient::test_reports_a_missing_table REMOVE 2 EntityNotFoundException for a missing table is asserted through the public get_table_metadata. test_cursor.py::TestCursor::test_glue_answer_that_the_table_is_missing
TestGlueMetadataClient::test_rejects_an_unsupported_catalog REMOVE 7, 2 A ValueError guard of the internal client that the cursor never reaches, because the cursor checks usable_for first. TestGlueMetadataClient::test_supports, test_cursor.py::TestCursor::test_throttled_metadata_without_glue[outside-glue]
TestGlueMetadataClient::test_stops_after_it_cannot_reach_glue REMOVE 2 Unreachable Glue disables the fallback, which is asserted through the cursor. test_cursor.py::TestCursor::test_unreachable_glue_is_not_tried_again
TestGlueMetadataClient::test_client_leaves_out_athena_endpoint REWRITE 1, 3 Keep the endpoint and api_version check, as test_connection.py::TestConnection::test_s3_client_leaves_out_athena_endpoint does for S3; drop the 8-thread barrier that asserts the private glue._lock.locked(). —

Notes

  • Kept after the cross-check:
    • test_cursor.py::TestCursor::test_execute_interrupted_while_starting_without_kill_on_interrupt is KEEP. TestCursor::test_execute_without_kill_on_interrupt interrupts the polling after the start succeeded; only this test covers the if not self._kill_on_interrupt: return start() branch of BaseCursor._start_execution.
    • test_parser.py::TestTypedValueConverter::test_map_varchar_integer is KEEP. It is the only check of the JSON branch of _convert_typed_map; its DefaultTypeConverter twin in test_converter.py stays REMOVE. It can become a hint-sensitive JSON param of test_converter.py::test_typed_json_elements.
    • test_connection.py::TestConnection::test_cursor_arraysize_over_page_size is KEEP. It becomes the merge target of the aio test_cursor_arraysize_over_page_size, and after that merge it is the only test of the AioCursor/AioDictCursor rejection above 1,000.
  • Borderline KEEPs:
    • test_cursor.py::TestCursor::{test_cache_size_different_schema, test_cache_size_different_catalog, test_cache_search_prefers_latest_execution} patch the private _list_query_executions and call _find_previous_query_id. They protect the cache-context contract of PyAthena caching does not check schema or catalog names #739/Fix/739 cache schema catalog #740, and the aio sub-issue removes its copies in their favor.
    • test_cursor.py::TestCursor::test_execute_internal_options_passthrough patches private methods; f691cfe kept it on purpose.
    • The SIGINT tests (TestCursor::test_cancel_in_stopped_scope, test_execute_kill_on_interrupt, test_execute_kill_on_interrupt_failure, test_execute_without_kill_on_interrupt, test_execute_interrupted_while_starting, test_execute_interrupted_while_starting_failure, test_async_cursor_execute_interrupted_while_starting) use private members and events, not sleeps. test_async_cursor_execute_interrupted_while_starting belongs in test_async_cursor.py.
    • test_cursor.py::TestCursor::{test_execute_with_options, test_execute_kwargs_override_options, test_connection_level_callback, test_multiple_callbacks_invoked, test_on_poll_connection_level, test_on_poll_cursor_level} and test_async_cursor.py::TestAsyncCursor::test_on_poll cost 1 query each and can share a connection.
    • test_cursor.py::TestCursor::test_cancel [original] and test_async_cursor.py::TestAsyncCursor::test_cancel [original] use sleep(randint(5, 10)) and a 10,000 × 10,000 CROSS JOIN (criteria 3, 4).
    • test_cursor.py::TestCursor::test_throttled_metadata_reads_glue, the target of the test_glue.py MERGE, is listed in Audit the test suite for unnecessary and flaky tests #1239 as failing on differing Glue table properties, as is test_glue.py::TestGlueMetadataClient::test_reads_what_athena_reports.
    • test_result_set.py::TestAthenaResultSet::{test_fetch_all_rows_skips_column_labels_only_on_first_page, test_fetch_all_rows_as_csv} use the private _fetch_all_rows/_fetch_all_rows_as_csv for the managed-results CSV parity (Custom converters do not apply to pandas, Arrow, and Polars results with managed query result storage #1028/PandasCursor returns different date, array, map, and row values on managed query result storage #1042).
    • test_connection.py::test_cursor_execute_supported_keywords (patches _execute) and test_cursor_execute_unknown_keyword (vars(cursor)) are the two halves of the BREAKING: Reject unknown cursor keyword arguments #1102 contract.
    • test_converter.py::{test_to_struct_athena_native_formats, test_to_struct_athena_nested_formats}: about half of their params have the same shape.
  • Unused after the change:
    • test_cursor.py: the module _logger and the imports logging, json, _to_array, _to_map, _to_struct (only TestComplexDataTypes uses them); call if the cache-search rewrite drops call_args_list.
    • test_result_set.py (after Break result sets after failed or cancelled fetches and drop the cross-thread guards #1238): the imports AioBaseCursor, WithAsyncFetch, BaseCursor, CursorIterator, WithFetch, WithResultSet.
    • test_util.py: _WithCodeError, _NoResponseError, _test_retry, and the imports _without_retries and typing.Any.
    • test_parser.py: the TypeNode import.
    • test_glue.py: the imports BotoConnectionError, unreachable_glue (still used by test_cursor.py), and threading/ThreadPoolExecutor after the endpoint rewrite.
    • test_formatter.py: the _escape_trino import.
  • Each row's cover is re-checked before the row is applied. A row whose cover does not hold becomes MERGE or KEEP.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancement

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions