Skip to content

Remove and rewrite unnecessary tests in the async S3 filesystem and S3 core tests #1241

Closed
#1259
Closed
Remove and rewrite unnecessary tests in the async S3 filesystem and S3 core tests#1241
#1259
Labels
enhancement

Description

@laughingman7743
laughingman7743
opened on Oct 17, 2026

Summary

Part of #1239. Files: filesystem/test_s3_async.py, filesystem/test_s3_core.py, filesystem/test_s3_executor.py, filesystem/test_s3_path_pairing.py, filesystem/test_s3_path.py, filesystem/test_s3_errors.py. At master 59169c5, with the cross-check corrections, they have 278 test functions: 169 KEEP, 70 REMOVE, 18 MERGE, 21 REWRITE. The 37 removed AWS tests in test_s3_async.py send about 450 S3 requests and upload about 100 MiB per run; the 16 checksum multipart cases account for about 80 MiB. The AWS rewrites cut test_rm_recursive from 10 files to 2 and drop test_write_transaction[1 KiB] and one size of test_upload_cp_file. test_copy_object_with_multipart_upload_max_workers stops waiting out a 5 s timeout. All tests in the other five files are offline.

Paths are relative to tests/pyathena/. [original] marks a test that existed at 21cbf05 (v3.15.0). None of the tests in these six files is [original].

Order

  1. test_write_failed_part_while_waiting fails intermittently since queued parts are cancelled #1237 owns filesystem/test_s3_async.py::TestAioS3StreamedFile::test_write_failed_part_while_waiting, so that test is not listed here. Move S3File writes onto a multipart write session #1232 (commit 82d2ad2) already fixes it: both parts meet at a threading.Barrier(2) in the upload_part mock before either fails, the alternative that test_write_failed_part_while_waiting fails intermittently since queued parts are cancelled #1237 names.
  2. Move S3File writes onto a multipart write session #1232. In filesystem/test_s3_async.py it modifies the REWRITE rows TestAioS3File::test_open_parts_in_flight and TestAioS3File::test_open_failed_part_while_waiting, and the KEEP tests TestAioS3FileSystem::test_transaction_pipe_file_write and TestAioS3FileSystem::test_transaction_put_file_failed_write. In filesystem/test_s3.py it modifies covers cited below: TestS3File::test_write_parts_in_flight, TestS3File::test_write_failed_part_while_waiting, TestS3File::test_discard_in_stopped_scope, and TestS3FileSystem::{test_open_multipart_with_checksum, test_append_multipart_with_checksum, test_put_file_multipart_with_checksum, test_pipe_file_multipart_with_checksum, test_write_transaction_rollback, test_clear_multipart_uploads_after_listed_upload_is_aborted}. Move S3File writes onto a multipart write session #1232 changes no test in test_s3_core.py, test_s3_executor.py, test_s3_path_pairing.py, test_s3_path.py or test_s3_errors.py. Read UNLOAD timestamps at microseconds #1233 and Break result sets after failed or cancelled fetches and drop the cross-thread guards #1238 change none of the six files.
  3. The sync filesystem sub-issue, or the same PR. These async rows remove a test together with its sync twin, and cite the covers that remain after both go:
    • TestAioS3FileSystem::test_mv_null_version_onto_key, test_mv_null_version_conflicts_depend_on_bucket_state, test_clear_multipart_uploads_empty, test_put_file_multipart_with_checksum, test_pipe_file_multipart_with_checksum, test_clear_multipart_uploads_after_listed_upload_is_aborted, and TestAioS3StreamedFile::test_discard_in_stopped_scope.
    • TestAioS3FileSystem::test_mv_null_version_directory_bucket_does_not_read_bucket_state relies on the sync sub-issue's merge into filesystem/test_s3.py::TestS3FileSystem::test_mv_without_null_key_comparison_does_not_read_bucket_state.
    • TestAioS3FileSystem::test_mv_null_version_failure_does_not_delete relies on the sync sub-issue's rewrite of filesystem/test_s3.py::TestS3FileSystem::test_mv_null_version_failure_does_not_delete, and test_mv_null_version_onto_key on its rewrite of test_mv_null_version_reads_each_bucket.
    • TestS3Core::test_replace_object_metadata_omits_unset_fields relies on filesystem/test_s3.py::TestS3FileSystem::test_setxattr_omits_unset_system_metadata, which the sync sub-issue keeps.
    • The merge of TestS3Core::test_complete_multipart_upload_preserves_part_checksums relies on filesystem/test_s3_object.py::TestS3MultipartUploadPart::test_to_api_repr_with_checksum, which the sync sub-issue keeps unchanged with its 20 cases.
  4. Within this sub-issue: TestAioS3FileSystem::test_transaction_put_file_block_size merges into the rewritten test_put_file_in_transaction_open_parameters; TestAioS3FileSystem::test_rm relies on the rewritten test_rm_recursive; TestS3AioThreadPool::test_init relies on the rewritten test_max_workers_none; TestS3ClientError::test_properties relies on the merge of test_os_error_unknown_code, which keeps http_status_code is None.

Findings

1. filesystem/test_s3_async.py

Most AioS3FileSystem methods are one-line delegations, await self._thread_pool.run(self._sync_fs.<method>, ...): _info, _ls, _cat_file, _exists, _rm_file, _pipe_file/_put_file outside a transaction, _get_file, _mkdir, _makedirs, _touch, _rmdir. sign, metadata, get_tags/put_tags, checksum, clear_multipart_uploads and invalidate_cache call self._sync_fs directly. AioS3File is a body-less subclass of S3File that differs only in its executor. "sync X" means filesystem/test_s3.py::TestS3FileSystem::X.

TestAioS3StreamedFile::test_write_failed_part_while_waiting (REWRITE) is left to #1237, which #1232 fixes, and is not listed.

Offline, TestAioS3FileSystem:

test change criteria reason covered by
TestAioS3FileSystem::test_copy_object_with_multipart_upload_part_sizes[1,4] REMOVE 2, 1 The ranges come from the shared S3Core.plan_multipart_copy/part_ranges, the async copy never passes max_workers to the plan, and the test asserts upload_part_copy call kwargs. filesystem/test_s3_core.py::TestS3Core::test_part_ranges; sync test_copy_object_with_multipart_upload_part_sizes
TestAioS3FileSystem::test_copy_object_with_multipart_upload_small_head_object_size[0,10] REWRITE 1 The async single-request branch is its own code; assert with Stubber that HeadObject is followed by one CopyObject with CopySource.VersionId=v1 and no other request, instead of mocked core calls and _call.call_count == 1. (plan: filesystem/test_s3_core.py::TestS3Core::test_plan_multipart_copy_fits_single_request)
TestAioS3FileSystem::test_copy_object_with_multipart_upload_failed_listing REMOVE 2 The annotations are listed inside the shared S3Core.plan_multipart_copy, before the creation that needs the plan. sync test_copy_object_with_multipart_upload_failed_listing; TestAioS3FileSystem::test_copy_object_with_multipart_upload_copies_source
TestAioS3FileSystem::test_copy_object_with_multipart_upload_max_workers REWRITE 3 overlapping_calls(3) with 4 parts makes the 4th call wait out the 5 s timeout; pass timeout=0.5 as test_copy_object_with_multipart_upload_shares_threads does and keep the GH-1159 assertion. —
TestAioS3FileSystem::test_max_workers_none REWRITE 6, 8, 1 Asserts private _sync_fs.max_workers/_thread_pool.max_workers against ThreadPoolExecutor's default formula; assert only that with max_workers=None _info() works and a mocked-core multipart copy runs. file default: TestAioS3File::test_open_max_workers[None]
TestAioS3FileSystem::test_cancelled_call_stops_retries REMOVE 2 _info is only self._thread_pool.run(self._sync_fs.info, ...), and cancel-stops-retries is the pool's contract. filesystem/test_s3_executor.py::TestS3AioThreadPool::test_run_cancelled_while_retrying[False]
TestAioS3FileSystem::test_copy_object_with_multipart_upload_failed_annotation_skips_queued_copies REWRITE 1 Busy-polls CPython's private ThreadPoolExecutor._work_queue.qsize(); hold a1 on a gate in the test's copy_object_annotation with max_workers=1 until a2/a3 are admitted, then assert one annotation request. —
TestAioS3FileSystem::test_copy_object_with_multipart_upload_invalid_block_size[2] REMOVE 2, 1 The check and its message are in the shared S3Core.plan_multipart_copy, and the test matches the message text. sync test_copy_object_with_multipart_upload_invalid_block_size
TestAioS3FileSystem::test_put_file_mode[overwrite,create] REMOVE 2 Outside a transaction, put_file is _sync_fs.put_file. sync test_put_file_mode
TestAioS3FileSystem::test_pipe_file_compression (intrans=False cases) REWRITE 2 Drop the two intrans=False cases, which run _sync_fs.pipe_file; keep intrans=True, which runs _pipe_file_in_transaction. sync test_pipe_file_compression
TestAioS3FileSystem::test_transaction_put_file_block_size MERGE 2, 1 Asserts the same fs.open(...) call as the target test. into TestAioS3FileSystem::test_put_file_in_transaction_open_parameters (after its rewrite)
TestAioS3FileSystem::test_put_file_in_transaction_open_parameters[2] REWRITE 1 Calls the private _put_file_in_transaction; call fs.put_file() inside fs.transaction and assert the same open() parameters, absorbing test_transaction_put_file_block_size. —
TestAioS3FileSystem::test_rm_requests REWRITE 2, 1 Only the parallel DeleteObjects to two buckets is async code; keep that request check and drop the bucket rejection, per-key error text and cache invalidation, which are shared _delete_paths/_raise_delete_errors/_delete_batch. sync test_rm_bucket, test_rm_errors, test_rm_request_error_invalidates_cache, test_rm_paths_across_buckets
TestAioS3FileSystem::test_rm_maxdepth REWRITE 1 Mocks _sync_fs.find and asserts its call args; stub ListObjectsV2 with a key deeper than maxdepth and assert that key is not in the DeleteObjects request (GH-962). —
TestAioS3FileSystem::test_get_version_path_destination MERGE 2 Same contract as the target, the local destination of a version, with a Path destination. into TestAioS3FileSystem::test_get_version (parametrize the destination)
TestAioS3FileSystem::test_expand_path_glob_lists_stem_prefix REWRITE 1, 6 Asserts a kwarg that fsspec's _glob passes to the private _sync_fs._find; assert the ListObjectsV2 Prefix with Stubber. —
TestAioS3FileSystem::test_mv_null_version_onto_key[3] REMOVE 2, 1 Async _mv makes one _sync_fs._move_pairs call (pyathena/filesystem/s3_async.py:392), and the test asserts _call/private _copy_file args. filesystem/test_s3_path_pairing.py::TestS3PathPairing::test_move_pairs_null_version_bucket_state; sync test_mv_null_version_request_versions, test_mv_null_version_reads_each_bucket (after its rewrite); TestAioS3FileSystem::test_mv
TestAioS3FileSystem::test_mv_null_version_conflicts_depend_on_bucket_state[3] REMOVE 2 Conflict detection is in the shared _move_pairs; the ValueError propagates through the same thread_pool.run. filesystem/test_s3_path_pairing.py::TestS3PathPairing::test_move_pairs_distinguishes_null_version_source; sync test_mv_conflicting_destinations (4th case)
TestAioS3FileSystem::test_mv_null_version_directory_bucket_does_not_read_bucket_state REMOVE 2 Shared _move_pairs. sync test_mv_without_null_key_comparison_does_not_read_bucket_state (after the sync merge)
TestAioS3FileSystem::test_mv_null_version_failure_does_not_delete[lookup,copy] REMOVE 2 The lookup failure is raised in the shared _move_pairs; a copy failure that does not delete is the async contract of test_mv_copy_failure. sync test_mv_null_version_failure_does_not_delete (after its rewrite); TestAioS3FileSystem::test_mv_copy_failure
TestAioS3FileSystem::test_clear_multipart_uploads_race[5] REMOVE 2 clear_multipart_uploads is self._sync_fs.clear_multipart_uploads(path), with the same Stubber script. sync test_clear_multipart_uploads_race
TestAioS3FileSystem::test_clear_multipart_uploads_empty REMOVE 2 Same delegation. sync test_list_and_clear_multipart_uploads (final clear on an empty prefix)
TestAioS3FileSystem::test_multipart_copy_uses_creation_algorithm[4] REMOVE 2 The checksum algorithm and type travel in core's S3MultipartUpload; the async orchestrator passes them through, and its request-level path is test_copy_object_with_multipart_upload_copies_source. sync test_multipart_copy_uses_creation_algorithm

AWS, TestAioS3FileSystem (fs fixture with connect()):

test change criteria reason covered by
TestAioS3FileSystem::test_open_multipart_with_checksum[4], test_put_file_multipart_with_checksum[4], test_pipe_file_multipart_with_checksum[4], test_append_multipart_with_checksum[4] REMOVE 2, 4 put_file/pipe_file outside a transaction are _sync_fs delegations, open() is S3File with another executor, and the checksum handling is S3File/core code; about 170 requests and 80 MiB per run. sync test_open_multipart_with_checksum, test_append_multipart_with_checksum; offline sync test_put_file_open_parameters, test_pipe_file_buffered_s3_parameters; TestAioS3FileSystem::test_write_transaction[10 MiB]
TestAioS3FileSystem::test_clear_multipart_uploads_after_listed_upload_is_aborted REMOVE 2, 4, 1 Delegation to _sync_fs.clear_multipart_uploads; patches list_multipart_uploads and asserts its call. sync test_clear_multipart_uploads_race[NoSuchUpload]
TestAioS3FileSystem::test_read[6] REMOVE 2, 4 The core get_object and private _fetch_range checks duplicate the sync test; the file read is S3File code, and the aio executor's parallel ranges are covered offline. sync test_read; TestAioS3File::test_open_parallel_requests
TestAioS3FileSystem::test_write[1 KiB, 1 MiB] REMOVE 2, 4 Both sizes are below the 5 MiB block, so the file sends one PutObject and the aio executor is unused. sync test_write
TestAioS3FileSystem::test_write_transaction[1 KiB] REWRITE 2, 4 Keep only [10 MiB], the only real-S3 multipart write through S3AioExecutor. sync test_write_transaction
TestAioS3FileSystem::test_write_transaction_rollback REMOVE 2, 4 AioS3File inherits discard, and the async transaction rollback is covered offline. sync test_write_transaction_rollback; TestAioS3FileSystem::test_transaction_pipe_put_file[False]
TestAioS3FileSystem::test_append[1 KiB, 1 MiB] REMOVE 2, 4 Small appends follow the S3File rewrite path with no executor. sync test_append
TestAioS3FileSystem::test_ls_buckets REMOVE 2, 4 _ls delegation. sync test_ls_buckets
TestAioS3FileSystem::test_ls_dirs REMOVE 2, 4 _ls/_pipe_file/_touch delegations. sync test_ls_dirs
TestAioS3FileSystem::test_info_bucket, test_info_dir, test_info_file REMOVE 2, 4 _info delegation; every field assertion is about the S3Object that the sync info builds. sync test_info_bucket, test_info_dir, test_info_file
TestAioS3FileSystem::test_find_maxdepth REMOVE 2, 4 maxdepth, including the maxdepth=0 ValueError, goes to the sync _find unchanged. sync test_find_maxdepth; TestAioS3FileSystem::test_find
TestAioS3FileSystem::test_find_withdirs REMOVE 2, 4 withdirs goes to the sync _find unchanged. sync test_find_withdirs; TestAioS3FileSystem::test_glob (fsspec _glob calls _find(withdirs=True))
TestAioS3FileSystem::test_du REMOVE 2, 4, 6 fsspec's async _du runs over async _find(detail=True), whose detail dict with sizes test_find checks. sync test_du; TestAioS3FileSystem::test_find
TestAioS3FileSystem::test_exists_bucket, test_exists_object REMOVE 2, 4 _exists delegation. sync test_exists_bucket, test_exists_object
TestAioS3FileSystem::test_rm_file REMOVE 2, 4 _rm_file delegation. sync test_rm_file
TestAioS3FileSystem::test_rm REMOVE 2, 4 Async _rm of one key runs the same _rm/_delete_objects path as the recursive test. TestAioS3FileSystem::test_rm_recursive (after its rewrite); sync test_rm
TestAioS3FileSystem::test_rm_recursive REWRITE 4 Use 2 files instead of 10; all 10 fit in one DeleteObjects batch, so the extra files exercise nothing more. —
TestAioS3FileSystem::test_touch REMOVE 2, 4 _touch delegation. sync test_touch; TestAioS3FileSystem::test_touch_sync_wrapper
TestAioS3FileSystem::test_pipe_cat[2] REMOVE 2, 4 _pipe_file/_cat_file delegations, still exercised as setup in the kept AWS tests. sync test_pipe_cat
TestAioS3FileSystem::test_cat_ranges REMOVE 2, 4 _cat_file(start, end) delegation. sync test_cat_ranges
TestAioS3FileSystem::test_put[2], test_put_with_callback[2] REMOVE 2, 4 _put_file delegation outside a transaction. sync test_put, test_put_with_callback
TestAioS3FileSystem::test_upload_cp_file[1 MiB] REWRITE 2, 4 Keep one size; both sizes take the same single CopyObject branch of async _copy_file. —
TestAioS3FileSystem::test_move_null_version_onto_key_sync_wrapper[3] REMOVE 2 mv() is sync(self.loop, self._mv, ...), the same coroutine as the async test, and test_move_recursive also runs the wrapper. TestAioS3FileSystem::test_move_null_version_onto_key
TestAioS3FileSystem::test_move REMOVE 2, 4 The async gather and delete are covered by the recursive move, and single-file pairing is the shared _move_pairs. TestAioS3FileSystem::test_move_recursive; sync test_move
TestAioS3FileSystem::test_get_file REMOVE 2, 4 _get_file delegation. sync test_get_file
TestAioS3FileSystem::test_open_returns_aio_s3_file REMOVE 2, 4, 6 The isinstance check is already made offline, and a 10-byte read never uses the executor. TestAioS3File::test_open_max_workers, TestAioS3File::test_open_parallel_requests; sync test_read
TestAioS3FileSystem::test_checksum REMOVE 2, 4 checksum is self._sync_fs.checksum. sync test_checksum
TestAioS3FileSystem::test_sign REMOVE 2, 3, 4 sign is self._sync_fs.sign, and the test sleeps 1 s. sync test_sign
TestAioS3FileSystem::test_mkdir_and_rmdir REMOVE 2, 4 _mkdir/_makedirs/_rmdir/rmdir delegations. sync test_mkdir_and_rmdir
TestAioS3FileSystem::test_metadata_and_tags REMOVE 2, 4 metadata/setxattr/getxattr/get_tags/put_tags call _sync_fs directly. sync test_metadata_getxattr_setxattr, test_get_and_put_tags
TestAioS3FileSystem::test_pandas_write_csv REMOVE 2, 4 The same S3File PutObject write as the sync test; the async fsspec registration is kept by the read test. sync test_pandas_write_csv; TestAioS3FileSystem::test_pandas_read_csv
TestAioS3FileSystem::test_sync_wrappers REMOVE 6, 2 Tests fsspec's mirror_sync_methods; kept tests already call fs.glob, fs.mv and fs.open. TestAioS3FileSystem::test_glob, TestAioS3FileSystem::test_move_recursive
TestAioS3FileSystem::test_invalidate_cache REMOVE 5, 6 The size is 4 with or without invalidation, so the test passes when invalidation is broken; the method is a _sync_fs delegation. sync test_invalidate_cache_* (offline)

TestAioS3File and TestAioS3StreamedFile:

test change criteria reason covered by
TestAioS3File::test_open_parts_in_flight[True] REWRITE 2 At #1232 the pyathena.filesystem.s3.wait patch is gone and the gates make in_flight == [1, 2, 2, 2] deterministic; the remaining change drops the [True] case, whose asynchronous=True filesystem gives S3ThreadPoolExecutor, the sync file's executor. filesystem/test_s3.py::TestS3File::test_write_parts_in_flight
TestAioS3File::test_open_failed_part_while_waiting[True] REWRITE 2 At #1232 an event holds both parts until both are submitted, so submitted == [1, 2] asserts that part 3 is never submitted; the remaining change drops the [True] case, which runs the sync file's executor. filesystem/test_s3.py::TestS3File::test_write_failed_part_while_waiting
TestAioS3File::test_open_version_id REMOVE 2, 1 _open forwards version_id to S3File, and the sync test asserts the same info() call and more. sync test_open_version_id
TestAioS3File::test_open_lookup_parameters REMOVE 2, 1 _open forwards the kwargs to S3File, whose lookup sends the SSE-C parameters. sync test_open_lookup_parameters
TestAioS3File::test_get_ranges[6] REMOVE 2, 7 Calls S3File._get_ranges directly with the same 6 parameter sets and no async code. filesystem/test_s3.py::TestS3File::test_get_ranges
TestAioS3StreamedFile::test_write_parts_in_flight REWRITE 1 Keep the S3AioThreadPool.submit_to patch, the only way to see queued parts, but assert max(in_flight) <= 2 and the completed part numbers instead of the exact [1, 2, 2, 2]. —
TestAioS3StreamedFile::test_commit_failed_part_first MERGE 1 Fabricates private _writer.upload/_parts futures and calls private _complete_upload; parametrize the target so that part 2 fails while part 1 retries, and assert that close() raises part 2's error within EVENT_TIMEOUT. into TestAioS3StreamedFile::test_failed_part_stops_running_parts
TestAioS3StreamedFile::test_discard_in_stopped_scope REMOVE 2, 1 The abort goes through S3MultipartWriter.abort to S3Core.abort_multipart_upload (unstoppable_retries), shared with S3File; the test calls private _discard/_close_without_commit. sync test_abort_multipart_upload_in_stopped_scope; test_util.py::test_retry_api_call_unstoppable
TestAioS3StreamedFile::test_cancelled_read_frees_ranges REWRITE 1 Globally patches asyncio.wait to fake a cancellation; cancel the f.read() task while get_object is held on an event, then check the weakrefs. —

2. filesystem/test_s3_core.py

S3Core, S3DeleteBatch and S3ObjectSummary are documented public API (docs/filesystem.md, "Typed S3 operations"); the request a method sends is the contract.

test change criteria reason covered by
TestS3Core::test_properties REMOVE 6 Checks that retry_config and request_kwargs return the constructor arguments, and the RetryConfig() default. — (filtering of request_kwargs: TestS3Core::test_get_object, test_put_object, test_get_bucket_versioning)
TestS3Core::test_request_kwargs_go_to_operations_that_accept_them REMOVE 2 The same rule is asserted in other tests: request_kwargs reach the operations that accept them and are dropped from the others. TestS3Core::test_get_object, test_put_object, test_get_bucket_versioning, test_object_tagging, test_operation_params[head_bucket]
TestS3Core::test_get_bucket_versioning_translates_errors REMOVE 2 Every operation translates errors through the same S3Core.call(); this repeats the 404 case for one more operation. TestS3Core::test_call_translates_errors; filesystem/test_s3_errors.py::TestS3ClientError::test_os_error_by_error_code
TestS3Core::test_delete_objects_translates_errors REMOVE 2 Same shared call() translation, 403 to PermissionError. TestS3Core::test_call_translates_errors[403]
TestS3Core::test_replace_object_metadata_omits_unset_fields REMOVE 2 The same CopyObject request, with no unset headers and StorageClass STANDARD, is stubbed through the public setxattr(). filesystem/test_s3.py::TestS3FileSystem::test_setxattr_omits_unset_system_metadata
TestS3DeleteBatch::test_from_paths_rejects_bucket_paths MERGE 2 Only this test checks the documented from_paths() ValueError for a keyless path in a mixed list; add one pytest.raises(ValueError) on [S3Path("b1", "a"), S3Path("b1")] to the target. into TestS3DeleteBatch::test_from_paths
TestS3ObjectSummary::test_frozen REMOVE 6 Checks that a frozen=True dataclass raises on assignment, which is Python's behavior. —
TestS3Core::test_get_object_translates_errors MERGE 2 Same call() translation; move its unique __cause__ ClientError assertion, which s3.py reads for InvalidRange, and the 416 InvalidRange case into the target. into TestS3Core::test_call_translates_errors
TestS3Core::test_head_object_requires_a_key, test_delete_object_requires_a_key, test_put_object_rejects_paths, test_multipart_upload_requires_keys, test_create_multipart_upload_rejects_versions MERGE 2 Five single-purpose copies of the same ValueError check for a path without a key or with a version. into TestS3Core::test_object_operations_reject_paths (alongside TestS3Core::test_copy_rejects_paths)
TestS3Core::test_create_bucket_rejects_acls MERGE 2 The same canned-ACL validation as the ACL table; add ("create_bucket", ("bucket",), acl=...) rows. into TestS3Core::test_acl_validation
TestS3Core::test_multipart_upload_requires_identity (12 cases) MERGE 2 All 4 methods call one shared _multipart_upload_request(); keep the 3 missing fields on one method plus one case per other method, about 6 cases. reduced parametrization of the same test
TestS3Core::test_upload_part MERGE 2 Both tests cover the upload's identity overriding per-call fields for upload_part; fold its SSECustomerAlgorithm pass-through and PartNumber=99 override into the target. into TestS3Core::test_upload_part_uses_creation_algorithm_and_identity
TestS3Core::test_complete_multipart_upload_preserves_part_checksums (20 cases) MERGE 2 The 10 checksum fields × copy repeat the per-field mapping of S3MultipartUploadPart, and the core only picks f"Checksum{algorithm}"; keep only the MultipartUpload={"Parts": []} precedence assertion, in the target. into TestS3Core::test_complete_multipart_upload; mapping: filesystem/test_s3_object.py::TestS3MultipartUploadPart::test_to_api_repr_with_checksum; selection: TestS3Core::test_complete_multipart_upload_uses_creation_algorithm, test_multipart_upload_preserves_access_point_identity
TestS3ObjectSummary::test_from_response MERGE 2 test_list_objects already builds summaries from responses with full fields, defaults and .path; only StorageClass is extra. into TestS3Core::test_list_objects (add StorageClass to the stubbed entry)
TestS3Core::test_generate_presigned_url (3 cases) REWRITE 1 Replaces core.call with a MagicMock and asserts its arguments; sign locally as test_generate_presigned_url_signs_locally does and assert the parsed URL: the key other for the override case, versionId present or absent, response-content-type present, no x-amz-request-payer. —

3. filesystem/test_s3_executor.py

S3AioThreadPool and S3AioExecutor are documented in docs/aio.md ("Threads", "Executor strategy").

test change criteria reason covered by
TestS3AioThreadPool::test_init REMOVE 6, 8 Re-computes ThreadPoolExecutor's default min(32, cpu + 4) formula. None: filesystem/test_s3_async.py::TestAioS3FileSystem::test_max_workers_none (after its rewrite); 0: filesystem/test_s3_async.py::TestAioS3File::test_open_invalid_max_workers
TestS3AioExecutor::test_init REMOVE 6, 2 Every test that builds S3AioExecutor(loop=...) without max_workers exercises the default, and the ValueError is the pool's. TestS3AioExecutor::test_submit_from_default_executor; filesystem/test_s3_async.py::TestAioS3File::test_open_invalid_max_workers
TestS3AioThreadPool::test_shutdown REMOVE 6, 2 Once the inner executor exists, "pending calls still run" and "submit after shutdown raises" are ThreadPoolExecutor's semantics. TestS3AioExecutor::test_shutdown_before_dispatch
TestS3AioThreadPool::test_wait_shielded_cancelled REMOVE 1, 2 Polls the private Future._done_callbacks; the GH-1186 contract that a late failure after cancel is not logged is asserted through the real wait_shielded() callers. filesystem/test_s3_async.py::TestAioS3FileSystem::test_copy_object_with_multipart_upload_cancelled_completion[True], test_copy_object_with_multipart_upload_cancelled_creation[True]
TestS3AioExecutor::test_submit REMOVE 2 The result and exception propagation go through the shared _S3WorkItem.run. TestS3AioExecutor::test_submit_from_default_executor, TestS3AioExecutor::test_shutdown_before_dispatch; exception: TestS3Executor::test_submit_to
TestS3AioExecutor::test_executor_shut_down, TestS3Executor::test_submit_to_executor_shut_down, TestS3Executor::test_submit_to_cancelled_executor_shut_down REMOVE 1, 7 Each calls the private executor._pool.shutdown() to force a dispatch failure that shutdown() orders away, exercising only the defensive settle() exception branch. TestS3Executor::test_submit_to_loop_shutdown
TestS3AioExecutor::test_loop_shutdown MERGE 2 submit_to() wraps submit(), so a cancelled outer future implies a cancelled inner one; assert both futures in the target. into TestS3Executor::test_submit_to_loop_shutdown
TestS3AioThreadPool::test_run_cancelled_after_return REWRITE 3 Relies on time.sleep(0.1) for the worker to return before task.cancel(); wrap pool.submit with a spy that captures the concurrent future and wait on future.result(EVENT_TIMEOUT) before cancelling, then assert one call (GH-1203). —
TestS3AioThreadPool::test_run_cancelled_before_start REWRITE 1 Polls the private pool._get_executor()._work_queue.qsize(); delete the loop, since the task has called submit() by the time await asyncio.to_thread(started.wait) returns. —
TestS3AioThreadPool::test_fork REWRITE 1 Uses the private _lock, _reset_after_fork() and _forked_executors; run a real os.fork() (skipped without it, multi-threaded fork DeprecationWarning filtered) in which the child submits sum([1, 2]) and exits with os._exit(0 if ok else 1), and assert the status from waitpid. —

4. filesystem/test_s3_path_pairing.py

test change criteria reason covered by
TestS3PathPairing::test_conflict_candidates_one_per_pair REMOVE 1, 7 Asserts that duplicate candidates are not deduplicated so that moves send the same HEAD requests as before (1a73556), a request count and not a contract. candidate selection: TestS3PathPairing::test_move_pairs_null_version_bucket_state, test_move_bucket_names_are_a_collection, test_move_pairs_directory_without_object, test_move_pairs_version_of_missing_directory_key
TestS3PathPairing::test_move_pairs MERGE 2 A null version moved onto its key staying in place is the default-bucket-state case of the target; add its extra ordinary pair there. into TestS3PathPairing::test_move_pairs_null_version_bucket_state

5. filesystem/test_s3_path.py

test change criteria reason covered by
TestS3Path::test_str_spells_the_version_query_as_version_id REMOVE 2 Follows from the two covers: parse maps the version_id spelling to S3Path("bucket", "obj", "v1"), and str of a versioned path spells it versionId. TestS3Path::test_parse["bucket/obj?version_id=v1"], TestS3Path::test_names[versioned]

6. filesystem/test_s3_errors.py

test change criteria reason covered by
TestS3ClientError::test_properties REMOVE 6, 2 Checks that code, message and http_status_code return response fields, which the os_error tests depend on. TestS3ClientError::test_os_error_by_error_code (code, message), test_os_error_by_http_status_code (status), test_os_error_by_error_code_errno (after the merge below, no status)
TestS3ClientError::test_os_error_unknown_code MERGE 2 One more code-to-errno row; the merged row keeps the http_status_code is None assertion. into TestS3ClientError::test_os_error_by_error_code_errno as ("UnknownCode", errno.EIO)

Notes

Borderline KEEPs:

  • filesystem/test_s3_async.py::TestAioS3FileSystem::test_rm_request_error_keeps_errors: async _delete_objects collects results with its own asyncio.gather(..., return_exceptions=True) (pyathena/filesystem/s3_async.py:356-363); the sync twin never reaches it.
  • filesystem/test_s3_executor.py::TestS3AioThreadPool::test_run_context: the only test of the documented run() context propagation; TestS3Executor::test_submit_context[S3AioThreadPool] never calls run().
  • TestAioS3FileSystem::test_copy_object_with_multipart_upload_copies_source, _failed_part, _failed_annotation: Stubber request-level tests of the separate async multipart copy; _failed_annotation counts annotation requests.
  • TestAioS3FileSystem::test_copy_object_with_multipart_upload_waits_for_running_parts, _cancelled[1,2], _cancelled_completion, _cancelled_creation, _cancelled_queued_parts, _cancelled_queued_completion, TestAioS3StreamedFile::test_flush_cancelled_then_exit_with_exception: short sleeps or one asyncio.sleep(0) only give a regression a chance to show; _cancelled_queued_completion replaces the private fs._thread_pool.submit to learn when the completion is queued (Free failed part bodies without the GC and abort an async copy's upload created during loop shutdown #1186).
  • TestAioS3FileSystem::test_cp_file_failed_multipart_copy_invalidates_cache, test_cp_file_directory, test_mv, test_cp_file_multipart_parameters[2]: patch _info/_copy_file, and are the only tests of async _copy_file's invalidation, directory skip and parameter popping (cp_file() sends block_size and max_workers to S3 for some object sizes #967), and of _mv's parallel copies.
  • TestAioS3FileSystem::test_internal_file_system_not_cached: checks private _sync_fs/S3FileSystem._cache; a slimmer version would assert that skip_instance_cache instances do not share dircache.
  • TestAioS3StreamedFile::test_append_failed_part_copy: waits on private f._parts[:1] for determinism.
  • AWS TestAioS3FileSystem::test_glob, test_find, test_pandas_read_csv, test_open_async_write_read[2], test_open_async_append_exclusive, test_move_recursive, test_move_null_version_onto_key (opt-in): the only real-S3 tests of the async _glob/_find, the registered async filesystem, AioS3StreamedFile and async _mv.
  • filesystem/test_s3_core.py::TestS3Core::test_call_failure_frees_request[2], test_call_failure_through_executor_frees_request, filesystem/test_s3_executor.py::TestS3Executor::test_submit_to_failure_frees_arguments[2] (Free failed part bodies without the GC and abort an async copy's upload created during loop shutdown #1186): gc.disable() and weakref checks that a failed part body is freed.
  • filesystem/test_s3_executor.py::TestS3AioExecutor::test_shutdown_loop_stopped, test_shutdown_interpreter_finalizing: patch asyncio.run_coroutine_threadsafe as a pass-through spy and sys.is_finalizing; both guard against a hang.
  • filesystem/test_s3_core.py::TestS3Core::test_create_bucket[8], test_create_bucket_configuration[9]: large parametrizations; each row is a documented rule.

Unused after the change:

  • filesystem/test_s3_async.py imports: S3StorageClass (only test_info_*), datetime and UTC (only test_info_file), urllib.parse and urllib.request (only test_sign), chain (only test_read), boto3 (only test_put_file_mode), pathlib.Path (only test_get_file), stop_retries_on (only TestAioS3StreamedFile::test_discard_in_stopped_scope), S3File (only TestAioS3File::test_get_ranges). Callback becomes unused if the rewrite of test_put_file_in_transaction_open_parameters calls fs.put_file() without it.
  • filesystem/test_s3_async.py: the fs fixture's request.param branch is used only by test_read's indirect=["fs"]; the fixture can drop request.
  • filesystem/test_s3_core.py: from unittest import mock after the test_generate_presigned_url rewrite.
  • filesystem/test_s3_executor.py: import os after TestS3AioThreadPool::test_init is removed, unless the test_fork rewrite uses os.fork.

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