Skip to content

Remove and rewrite unnecessary tests in the sync S3 filesystem tests #1240

New issue
New issue
Closed
#1254
Closed
Remove and rewrite unnecessary tests in the sync S3 filesystem tests#1240
#1254
Labels
enhancement

Description

@laughingman7743
laughingman7743
opened on Oct 17, 2026

Summary

Part of #1239. Files: filesystem/test_s3.py (TestS3FileSystem, TestS3File, TestCompressedBuffer), filesystem/test_s3_writer.py, filesystem/test_s3_object.py, filesystem/test_reader.py, filesystem/test_init.py. At master 59169c5 with the cross-check corrections: 364 test functions: 260 KEEP, 56 REMOVE, 16 MERGE, 32 REWRITE. After #1232: #1232 deletes 2 of them (1 REMOVE, 1 REWRITE); TestS3File::test_discard (REMOVE) and TestS3File::test_commit_stops_at_failed_part (REWRITE) become KEEP; TestS3File::test_upload_chunk_small_file and TestS3File::test_upload_chunk_multipart change from REMOVE to REWRITE. The tables below list the state after #1232: 362 test functions: 262 KEEP, 52 REMOVE, 16 MERGE, 32 REWRITE. Every row except four runs offline. Those four save, per run, 13 real-S3 cases: about 80 MiB of multipart uploads and read-backs (test_put_file_multipart_with_checksum, test_pipe_file_multipart_with_checksum, 4 cases each), 4 of the 6 cases of test_core_multipart_upload_with_checksum (about 40-60 MiB), and 2 CreateMultipartUpload, 3 ListMultipartUploads and 3 AbortMultipartUpload requests (test_clear_multipart_uploads_after_listed_upload_is_aborted). No row runs Athena queries.

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

Order

  • After Move S3File writes onto a multipart write session #1232 (fb54bea), which moves the S3File writes onto _S3WriteSession. In these files it:
    • deletes TestS3File::test_write_interrupted_while_waiting_for_part (REMOVE) and TestS3File::test_upload_chunk_empty_file_touches (REWRITE), and adds TestS3File::test_upload_chunk_empty_file (an empty file gets PutObject without a body instead of touch());
    • changes rows of this issue: TestS3File::{test_interrupted_creation, test_creation_scheduling_interrupted, test_repeated_creation_interrupt, test_write_exceeding_max_parts, test_write_exceeding_max_parts_abort_failure, test_write_exceeding_max_parts_abort_interrupted, test_write_parts_in_flight, test_write_failed_part_while_waiting, test_append_failed_part_copy, test_commit_failure_and_discard, test_commit_stops_at_failed_part, test_discard, test_discard_in_stopped_scope, test_upload_chunk_small_file, test_upload_chunk_multipart} and TestS3FileSystem::{test_put_file_multipart_with_checksum, test_pipe_file_multipart_with_checksum, test_clear_multipart_uploads_after_listed_upload_is_aborted};
    • changes covers cited here: TestS3File::{test_commit_failure_and_interrupted_abort, test_creation_cancelled_before_start, test_creation_failure, test_discard_on_event_loop_thread, test_discard_waits_for_running_parts, test_interrupted_creation_stops_retries, test_multipart_write_request_parameters, test_part_scheduling_interrupted, test_write_failed_part_after_last_wait} and TestS3FileSystem::{test_open_multipart_with_checksum, test_append_multipart_with_checksum, test_write_transaction_rollback, test_put_file_failed_write_aborts_multipart_upload, test_pipe_file_failed_write, test_put_file_failed_write, test_pipe_file_compression_failed_write};
    • makes commit() after a discarded or failed write raise S3MultipartStateError, which the TestS3File rows below keep;
    • adds filesystem/test_s3_writer.py::TestS3WriteSession::{test_cancelled_waiter, test_dropped_request_fails_the_write, test_failed_part_is_released, test_outcomes_before_callbacks}, which this audit did not review.
  • The TestS3File rows were re-checked against the Move S3File writes onto a multipart write session #1232 head. The TestS3FileSystem rows only share the file and need a rebase.
  • Rows that the async and core sub-issue depends on:
    • TestS3FileSystem::test_setxattr_omits_unset_system_metadata stays (KEEP); the async and core sub-issue removes filesystem/test_s3_core.py::TestS3Core::test_replace_object_metadata_omits_unset_fields, which it covered.
    • filesystem/test_s3_object.py::TestS3MultipartUploadPart::test_to_api_repr_with_checksum stays unchanged (KEEP, 20 cases); the async and core sub-issue merges TestS3Core::test_complete_multipart_upload_preserves_part_checksums on that basis.
    • The async removals of 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 go with the sync removals here. The async test_mv_null_version_onto_key row cites TestS3FileSystem::test_mv_null_version_reads_each_bucket after its REWRITE here.
    • TestS3FileSystem::test_move_pairs_looks_up_only_conflict_candidates and filesystem/test_s3_path_pairing.py::TestS3PathPairing::test_conflict_candidates_one_per_pair (async and core sub-issue) are both removed; neither is the cover of the other.
  • Within this issue, apply each REWRITE/MERGE target with the rows that cite it: TestS3FileSystem::test_mv_null_version_reads_each_bucket (for test_mv_null_version_onto_key), TestS3FileSystem::test_mv_without_null_key_comparison_does_not_read_bucket_state (for the directory-bucket MERGE), TestS3File::test_write_parts_in_flight (for test_open_max_workers), TestS3File::test_repeated_creation_interrupt, TestS3FileSystem::test_pipe_file_failed_write_aborts_multipart_upload and TestS3File::test_commit_failure_and_discard (for test_interrupted_creation and test_write_exceeding_max_parts_abort_failure), TestS3File::test_write_exceeding_max_parts (for test_write_exceeding_max_parts_without_close), TestS3File::test_write_max_parts (for test_writer_empty_final_buffer_at_part_limit).

Findings

1. filesystem/test_s3.py

#1232 deletes TestS3File::test_write_interrupted_while_waiting_for_part and TestS3File::test_upload_chunk_empty_file_touches; they are not listed. After #1232, TestS3File::test_discard asserts that commit() after discard() raises S3MultipartStateError and that a second discard() sends nothing, and TestS3File::test_commit_stops_at_failed_part asserts the requests sent (upload_part for parts 1-3 only, the abort last, no completion) without patching wait; both are KEEP and not listed.

test change criteria reason covered by
TestS3FileSystem::test_mv_null_version_onto_key REMOVE 1, 2 Mocks the private _copy_file/_delete_objects and asserts their arguments; the bucket-state decision belongs to S3PathPairing. filesystem/test_s3_path_pairing.py::TestS3PathPairing::test_move_pairs_null_version_bucket_state; TestS3FileSystem::test_mv_null_version_request_versions; TestS3FileSystem::test_mv_null_version_reads_each_bucket (after its REWRITE)
TestS3FileSystem::test_mv_null_version_conflicts_depend_on_bucket_state REMOVE 1, 2 Same private-mock pattern; whether the conflict is raised is S3PathPairing logic. filesystem/test_s3_path_pairing.py::TestS3PathPairing::test_move_pairs_distinguishes_null_version_source; TestS3FileSystem::test_mv_conflicting_destinations (4th case)
TestS3FileSystem::test_copy_pairs_destination_lookup MERGE 1, 7 Only this test checks the fs wiring if sources and pairing.looks_up_destination (s3.py:1474); keep one assertion that a trailing-slash destination makes no isdir() request. into TestS3FileSystem::test_copy_version
TestS3FileSystem::test_move_pairs_looks_up_only_conflict_candidates REMOVE 1, 7 Calls the private _move_pairs, patches _head_object and counts its calls; the duplicate-HEAD count is not a contract. filesystem/test_s3_path_pairing.py::TestS3PathPairing::{test_move_pairs, 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}
TestS3FileSystem::test_copy_object_with_multipart_upload_request_parameters REMOVE 1, 2 Asserts kwargs and call_count of mocked core methods and the private _finish_multipart_upload; the Stubber test checks the same routing at the request level. TestS3FileSystem::test_copy_object_with_multipart_upload_copies_source; TestS3FileSystem::test_copy_object_with_multipart_upload_replace_directives; filesystem/test_s3_core.py::TestS3Core::test_plan_multipart_copy
TestS3FileSystem::test_copy_object_with_multipart_upload_head_object_size REMOVE 1, 2 Asserts upload_part_copy kwargs on mocked core methods; the ranges and the unpinned "null" version are planner contracts tested directly. filesystem/test_s3_core.py::TestS3Core::{test_plan_multipart_copy, test_plan_multipart_copy_source_version}; TestS3FileSystem::test_copy_object_with_multipart_upload_copies_source
TestS3FileSystem::test_put_file_block_size MERGE 1, 2 Only this test checks the default max_workers=fs.max_workers (s3.py:2038); add a call without block_size/max_workers to the target. into TestS3FileSystem::test_put_file_open_parameters
TestS3FileSystem::test_open_max_workers REMOVE 6 Asserts that S3File.max_workers stores the given value or the fs default. TestS3FileSystem::test_open_parameters_named_as_request_fields; TestS3File::test_write_parts_in_flight
TestS3FileSystem::test_find_maxdepth_counts_levels_like_fsspec MERGE 2 maxdepth 1 and 2 are already fsspec-compared cases; add ("dir", 3, False) and the maxdepth=0 ValueError. into TestS3FileSystem::test_find_matches_fsspec
TestS3FileSystem::test_mkdir_creates_bucket_in_us_east_1_without_location_constraint, TestS3FileSystem::test_mkdir_creates_bucket_with_acl, TestS3FileSystem::test_mkdir_creates_bucket_in_region MERGE 2 Near copies of one CreateBucket request check; parametrize as (client_region, kwargs, expected_request) with the invalid-ACL ValueError as one case. into TestS3FileSystem::test_mkdir_creates_bucket
TestS3FileSystem::test_mv_null_version_directory_bucket_does_not_read_bucket_state MERGE 2 Same documented "other moves do not make this request" contract; a directory-bucket path is one more case. into TestS3FileSystem::test_mv_without_null_key_comparison_does_not_read_bucket_state (after its REWRITE)
TestS3FileSystem::test_pipe_file_small_drops_max_workers MERGE 2 Same mocked core.put_object single-request check; pass max_workers=2 in the target. into TestS3FileSystem::test_pipe_file_small_uses_put_object
TestS3FileSystem::test_ls_from_cache_with_cached_object REWRITE 1, 7 Assert that fs.ls()/fs.exists() on bucket/key/child with a cached object entry does not raise TypeError and makes a request, instead of calling the private _ls_from_cache. —
TestS3FileSystem::test_missing_object_keeps_cached_parent_listing_without_it REWRITE 1 Run exists() on the missing sibling, then assert that the listed key is answered with no request, as test_missing_bucket_keeps_bucket_listing_without_it does. —
TestS3FileSystem::test_cache_read_with_concurrent_invalidation REWRITE 1 Keep the InvalidatingDirCache race and drive it through info()/exists()/ls() instead of the private _head_bucket/_head_object/_ls_buckets/_ls_dirs. —
TestS3FileSystem::test_cache_eviction_with_concurrent_invalidation REWRITE 1 Call info(..., refresh=True) and ls(..., refresh=True) from the two threads instead of the private methods. —
TestS3FileSystem::test_ls_dirs_partial_listing_bypasses_cache REWRITE 1, 7 Exercise the prefix case through find(prefix=...) and assert that a later complete ls() is served from the cache; drop the next_token case. —
TestS3FileSystem::test_ls_dirs_empty_refresh_evicts_cached_listing REWRITE 1 Assert through fs.ls("s3://bucket/dir", refresh=True) followed by one fs.ls() request, instead of the private _ls_dirs. —
TestS3FileSystem::test_finish_multipart_upload_request_parameters REWRITE 1, 7 Write through fs.open("wb", ContentType=..., RequestPayer=..., SSECustomerAlgorithm=...) with _record_requests, once succeeding and once with a failing upload_part, and assert the CompleteMultipartUpload and AbortMultipartUpload request fields (GH-946). —
TestS3FileSystem::test_mv_keeps_given_paths REWRITE 1 Use _serve_keys({"src"}) and assert the store is {"dst/"} instead of mocking _copy_file/_delete_objects. —
TestS3FileSystem::test_mv_without_null_key_comparison_does_not_read_bucket_state REWRITE 1 Use _serve_keys with get_bucket_versioning raising PermissionError and assert that each move succeeds (store contents). —
TestS3FileSystem::test_mv_null_version_reads_each_bucket, TestS3FileSystem::test_mv_null_version_does_not_cache_bucket_state REWRITE 1 Use a request-level fake serving two buckets and assert the GetBucketVersioning count and the CopyObject/DeleteObjects requests ("once per bucket, not cached between moves"). —
TestS3FileSystem::test_mv_null_version_failure_does_not_delete REWRITE 1 Make get_bucket_versioning or copy_object fail in _serve_keys and assert that the store is unchanged and no DeleteObjects request was sent. —
TestS3FileSystem::test_cp_file_multipart_parameters REWRITE 1 Record the CopyObject request of the small case and assert it has no block_size/max_workers fields (GH-967); drop the >5 GiB private-routing case. —
TestS3FileSystem::test_copy_object_with_multipart_upload_small_head_object_size REWRITE 1 Use Stubber with head_object returning size 0/10 and VersionId v1, then expect one copy_object with CopySource.VersionId=v1. plan side: filesystem/test_s3_core.py::TestS3Core::test_plan_multipart_copy_fits_single_request
TestS3FileSystem::test_pipe_file_failed_write_aborts_multipart_upload REWRITE 1 Make core.upload_part raise and assert the AbortMultipartUpload request and no completion, instead of replacing the private _create_executor and asserting executor.shutdown. put_file analogue: TestS3FileSystem::test_put_file_failed_write_aborts_multipart_upload
TestS3FileSystem::test_cache_lookup_concurrent_parameters REWRITE 1, 7 Run the interleaving through info(path, **LOOKUP_KWARGS) and info(path, **other_key) with the same InterleavedDirCache, then assert later lookups make no HEAD requests. —
TestS3FileSystem::test_cache_lookup_expiry REWRITE 1, 7 Use info(path, **LOOKUP_KWARGS) with the patched time.time and count HeadObject requests before and after expiry. —
TestS3FileSystem::test_get_file_parent_through_symlink REMOVE 6, 7 Checks how the OS resolves link/.. under open()/makedirs. TestS3FileSystem::test_get_file_creates_parent_directories
TestS3FileSystem::test_get_file_version_id REMOVE 1, 2 Its distinctive assertion is fs.info.call_count == 1 on a mocked lookup. TestS3FileSystem::test_get_version; TestS3FileSystem::test_question_mark_keys_and_null_version
TestS3FileSystem::test_read_interrupted_submission_stops_running_reads REMOVE 1, 3 Replaces the private f._executor.submit, busy-waits with time.sleep(0.01), and checks which ranges were sent. TestS3FileSystem::test_failed_read_stops_running_reads; test_util.py::test_retry_api_call_stopped_by_event
TestS3FileSystem::test_interrupted_read_frees_ranges REMOVE 1 Patches the module-level wait and checks frame clearing with weakrefs. TestS3FileSystem::test_failed_read_frees_other_ranges
TestS3FileSystem::test_finish_multipart_upload REMOVE 1, 7 Calls the private _finish_multipart_upload with hand-built futures and checks the exact complete_multipart_upload call. TestS3FileSystem::test_copy_object_with_multipart_upload_copies_source; TestS3FileSystem::test_multipart_copy_uses_creation_algorithm
TestS3FileSystem::test_finish_multipart_upload_aborts_on_failure (RuntimeError, KeyboardInterrupt) REMOVE 7 Tests only the abort=True branch, which has no caller. TestS3FileSystem::test_copy_object_with_multipart_upload_failed_part; TestS3FileSystem::test_copy_object_with_multipart_upload_interrupted_creation
TestS3FileSystem::test_finish_multipart_upload_without_abort REMOVE 1, 7 Checks private-helper bookkeeping (a pending future is not cancelled, _call is not called). TestS3FileSystem::test_copy_object_with_multipart_upload_failed_part
TestS3FileSystem::test_finish_multipart_upload_waits_for_running_parts REMOVE 1, 7 Tests the abort=True branch, patches the module wait and asserts its call_args_list. TestS3FileSystem::test_copy_object_with_multipart_upload_interrupted_part_scheduling; TestS3File::test_discard_waits_for_running_parts
TestS3FileSystem::test_finish_multipart_upload_does_not_wait_for_cancelled_parts REMOVE 3, 7 Tests the abort=True branch; that an unstarted Future() is cancellable is self-evident. TestS3FileSystem::test_copy_object_with_multipart_upload_interrupted_part_scheduling
TestS3FileSystem::test_finish_multipart_upload_abort_failure_does_not_mask_the_original_error REWRITE 1, 7 Drive _copy_object_with_multipart_upload with Stubber (stub_multipart_copy(fail_part=True) plus an abort error) and assert that the part error, not the abort error, is raised; drop the log text match. —
TestS3FileSystem::test_finish_multipart_upload_stops_at_failed_part REWRITE 1, 7 Drive _copy_object_with_multipart_upload with part 1 blocked, part 2 failing and part 3 queued, and assert that part 3 is never requested and the abort is sent, with no wait patch (GH-1178). —
TestS3FileSystem::test_copy_object_with_multipart_upload_interrupted_scheduling REMOVE 1 Patches threading.Thread.start globally to hit a narrow window; the outcome matches the interrupted-creation test. TestS3FileSystem::test_copy_object_with_multipart_upload_interrupted_creation
TestS3FileSystem::test_copy_object_with_multipart_upload_repeated_creation_interrupt REMOVE 1 Replaces future.result and patches the module wait, tied to the order of internal waits. TestS3FileSystem::test_copy_object_with_multipart_upload_interrupted_creation
TestS3FileSystem::test_copy_object_with_multipart_upload_interrupted_cleanup REMOVE 1 Patches every module wait and matches undocumented warning text. —
TestS3FileSystem::test_ls_sparse_entries_keep_defaults REMOVE 6, 7 Asserts S3Object constructor defaults for a response shape S3 does not return. —
TestS3FileSystem::test_object_version_info_from_markers REMOVE 1, 2 Asserts exact _call kwargs of the marker paging that S3Core owns. filesystem/test_s3_core.py::TestS3Core::test_list_object_versions_from_markers
TestS3FileSystem::test_object_version_info_paginates REMOVE 1, 2 Checks paging through assert_any_call on request kwargs. filesystem/test_s3_core.py::TestS3Core::test_list_object_versions; TestS3FileSystem::test_object_version_info_excludes_sibling_keys; TestS3FileSystem::test_object_version_info
TestS3FileSystem::test_info_keeps_head_object_fields REMOVE 7 A snapshot of the whole info() dict that fails whenever a field is added. filesystem/test_s3_core.py::TestS3Core::test_head_object; TestS3FileSystem::test_info_file
TestS3FileSystem::test_head_object_version_aware MERGE 1, 2 Only this test checks that info()["version_id"] is None without version_aware when the response has VersionId; parametrize the target on version_aware instead of calling the private _head_object. into TestS3FileSystem::test_info_version_aware_heads_listed_file
TestS3FileSystem::test_chmod_recursive_interrupted_submission_stops_running_requests REMOVE 1, 3 Replaces executor.submit and the private _create_executor; the outcome depends on timing. TestS3FileSystem::test_chmod_recursive_failure_stops_running_requests
TestS3FileSystem::test_put_tags_merge REMOVE 1, 2 Asserts the exact Get/PutObjectTagging request list. TestS3FileSystem::test_get_and_put_tags; filesystem/test_s3_core.py::TestS3Core::test_object_tagging
TestS3FileSystem::test_put_tags_invalid_mode REMOVE 2 The same ValueError, match="Mode must be" is asserted before any request in the integration test. TestS3FileSystem::test_get_and_put_tags
TestS3FileSystem::test_list_multipart_uploads_paginates REMOVE 1, 2 Marker paging checked through assert_any_call is S3Core iterator behavior. filesystem/test_s3_core.py::TestS3Core::{test_list_multipart_uploads, test_list_multipart_uploads_from_markers}
TestS3FileSystem::test_clear_multipart_uploads_empty REMOVE 7 Hits the if not uploads: return branch. TestS3FileSystem::test_list_and_clear_multipart_uploads (the final clear runs on an empty prefix)
TestS3FileSystem::test_clear_multipart_uploads_checks_all_results REMOVE 1, 5 Mocks the executor, the futures and as_completed and asserts that each future.result is called once. TestS3FileSystem::test_clear_multipart_uploads_race[AccessDenied]
TestS3FileSystem::test_put_file_multipart_with_checksum (4 cases) REMOVE 2, 4 put_file() writes through the same open(..., s3_additional_kwargs=...) path; about 40 MiB of S3 transfer per run. TestS3FileSystem::{test_open_multipart_with_checksum, test_append_multipart_with_checksum, test_put_file_open_parameters}
TestS3FileSystem::test_pipe_file_multipart_with_checksum (4 cases) REMOVE 2, 4 A large pipe_file() writes through the same S3File multipart path; about 40 MiB of S3 transfer per run. TestS3FileSystem::{test_open_multipart_with_checksum, test_append_multipart_with_checksum, test_pipe_file_buffered_s3_parameters}
TestS3FileSystem::test_core_multipart_upload_with_checksum (6 cases) REWRITE 2, 4 Keep only ("CRC32", "FULL_OBJECT") with and without copy, the cases that check the full-object CRC value (2 real-S3 runs instead of 6). TestS3FileSystem::{test_open_multipart_with_checksum, test_append_multipart_with_checksum, test_list_and_clear_multipart_uploads}
TestS3FileSystem::test_clear_multipart_uploads_after_listed_upload_is_aborted REMOVE 1, 2, 4 Patches the public method under test to abort mid-flight and asserts assert_called_once_with, against real S3. TestS3FileSystem::test_clear_multipart_uploads_race[NoSuchUpload]
TestS3FileSystem::test_ls_buckets_follows_pages REWRITE 1 Keep fs.ls("s3://") == ["a", "b"] across two pages (GH-1059) and drop the _call.call_args_list equality. request shape: filesystem/test_s3_core.py::TestS3Core::test_list_buckets
TestS3FileSystem::test_setxattr_keeps_system_metadata REMOVE 2 Both branches are asserted in core, and real S3 checks GH-975. filesystem/test_s3_core.py::TestS3Core::test_replace_object_metadata; TestS3FileSystem::test_metadata_getxattr_setxattr
TestS3File::test_interrupted_creation (10 cases) MERGE 1, 2, 3 Delivers a real SIGINT in a pthread_kill loop for a recovery path that a simulated interrupt reaches; merge the write/append axis into the target, which at #1232 also asserts that commit() raises S3MultipartStateError. into TestS3File::test_repeated_creation_interrupt; pipe/put: TestS3FileSystem::{test_pipe_file_failed_write_aborts_multipart_upload, test_put_file_failed_write_aborts_multipart_upload}; abort failure: TestS3File::test_commit_failure_and_discard[abort_fails=True], filesystem/test_s3_writer.py::TestS3MultipartWriter::test_writer_failed_abort_is_retryable
TestS3File::test_repeated_creation_interrupt (4 cases) REWRITE 1, 2 Keep the repeated-interrupt contract with the wb/ab axis, drop the autocommit and abort_fails axes, and assert the AbortMultipartUpload request without call_count == 2; keep the commit() raise added by #1232. abort failure: TestS3File::test_commit_failure_and_discard[abort_fails=True]
TestS3File::test_creation_scheduling_interrupted MERGE 1, 8 Patches threading.Thread.start, which relies on ThreadPoolExecutor spawning a thread inside submit(); add a creation stage to the target's executor.submit wrapper (both assert the commit() raise at #1232). into TestS3File::test_part_scheduling_interrupted; filesystem/test_s3_executor.py::TestS3Executor::test_submit_to
TestS3File::test_cancelled_async_write_finishes REMOVE 6, 7 Shows that cancelling asyncio.to_thread does not stop its thread, which is Python behavior. —
TestS3File::test_append_part_copy_ranges (max_workers 1, 4) MERGE 1, 2 The ranges come from S3Core.part_ranges and max_workers no longer affects them; add (b"a" * 129, b"b", True, True) to the target. into TestS3File::test_append; filesystem/test_s3_core.py::TestS3Core::{test_part_ranges, test_part_ranges_max_parts}
TestS3File::test_write_max_parts (3 cases x autocommit) REWRITE 2 Keep the 3 boundary cases and drop the autocommit axis (6 to 3 cases). deferred commit: TestS3FileSystem::test_write_transaction
TestS3File::test_write_exceeding_max_parts (3 cases x autocommit) REWRITE 1 Assert that no fs.core.upload_part call has part_number > MULTIPART_UPLOAD_MAX_PARTS, that the abort is sent and nothing is completed or put, and that a deferred commit() raises S3MultipartStateError; drop the executor.submit_to.call_args_list and executor.shutdown checks. —
TestS3File::test_write_exceeding_max_parts_without_close MERGE 1, 2 Only asserts executor.shutdown.assert_called_once(); add a case without the with block (closed file, abort sent). into TestS3File::test_write_exceeding_max_parts
TestS3File::test_write_exceeding_max_parts_abort_failure (autocommit x2) REMOVE 1, 2 Same _close_without_commit to discard() path as a failed commit; matches log text and executor.submit_to.call_args_list. TestS3File::test_commit_failure_and_discard[abort_fails=True] (at #1232 also the commit() raise); filesystem/test_s3_writer.py::TestS3MultipartWriter::test_writer_failed_abort_is_retryable
TestS3File::test_write_exceeding_max_parts_abort_interrupted MERGE 2 Only this test asserts that the executor is shut down when the abort is interrupted, and at #1232 that a later commit() raises S3MultipartStateError matching "failed to abort"; add both assertions to the target (wrap its executor). into TestS3File::test_commit_failure_and_interrupted_abort
TestS3File::test_write_parts_in_flight (3 cases x max_workers 1/2/3) REWRITE 1 #1232 already drops the count of wait(return_when=FIRST_COMPLETED) calls; reduce max_workers to [1, 2] and keep the in-flight bound (GH-1158). —
TestS3File::test_write_failed_part_while_waiting (autocommit x max_workers 1/2/3) REWRITE 1 Keep GH-1173 (the write raises the part's error, closes without commit, commit() raises S3MultipartStateError) and assert that every fs.core.upload_part part number is at most max_workers instead of reading executor.submit_to.call_args_list; reduce max_workers to [1, 2]. —
TestS3File::test_append_failed_part_copy (max_workers 1, 4) REWRITE 1 Assert the part numbers of fs.core.upload_part calls instead of the submitted ones, and signal the failed copy with an event that upload_part_copy sets. —
TestS3File::test_multipart_write_keyword_named_as_argument (8 cases) REMOVE 1, 7 Lowercase kwargs cannot collide because operation_params keeps only service-model members. filesystem/test_s3_core.py::TestS3Core::test_operation_params
TestS3File::test_upload_chunk_small_file (autocommit x2) REWRITE 1 Write a small file through S3File (mode="wb", with and without autocommit), commit, then call discard() and commit() again and assert one PutObject and no further request (GH-719, GH-1211 from #1232); drop the private _upload_chunk return values. —
TestS3File::test_upload_chunk_multipart (autocommit x2) REWRITE 1 Write a multipart file through S3File, commit, then call discard() and commit() again and assert one CompleteMultipartUpload, no PutObject and no further request (GH-1211 from #1232); drop the private _upload_chunk return values. —
TestS3File::test_commit_failure_and_discard (error x abort_fails) REWRITE 1, 2 Keep GH-1014/945/1186 (one abort, the upload kept when the abort fails, the commit() raise from #1232); drop the error axis and the caplog text assertion. —
TestS3File::test_discard_in_stopped_scope REMOVE 2 The unstoppable abort retries live in S3Core.abort_multipart_upload, shared with the copy path. TestS3FileSystem::test_abort_multipart_upload_in_stopped_scope; test_util.py::test_retry_api_call_unstoppable
TestCompressedBuffer::test_compress (gzip, bz2, xz) REMOVE 2, 6 The bz2/xz cases test fsspec's codecs; the gzip round trip is checked through pipe_file. TestS3FileSystem::{test_pipe_file_compression, test_pipe_file_compression_multipart}
TestCompressedBuffer::test_compress_non_contiguous_memoryview REMOVE 2 pipe_file passes the memoryview unchanged to CompressedBuffer.compress. TestS3FileSystem::test_pipe_file_compression_non_contiguous_memoryview

2. filesystem/test_s3_writer.py

test change criteria reason covered by
TestS3MultipartWriter::test_writer_short_reads REMOVE 7 Both callers pass a BytesIO buffer, which never returns a short read; covers the retry loop of the private _read_block. —
TestS3MultipartWriter::test_writer_copy_parts (4 cases) REMOVE 2 iter_copy_parts enumerates S3Core.part_ranges, which is tested directly. filesystem/test_s3_core.py::TestS3Core::{test_part_ranges, test_part_ranges_max_parts}; filesystem/test_s3.py::TestS3File::test_append
TestS3MultipartWriter::test_writer_empty_final_buffer_at_part_limit REMOVE 2 The final flush of a file at exactly the part limit reaches iter_parts on an empty buffer end to end. filesystem/test_s3.py::TestS3File::test_write_max_parts (after its REWRITE)
TestS3MultipartWriter::test_writer_invalid_first_number (2 methods x 0/4) REMOVE 1, 7 first_part_number=0 cannot be passed by any caller, the =4 case is the part-limit error, and it matches message regexes. TestS3MultipartWriter::test_writer_part_limit_includes_copies
TestS3MultipartWriter::test_writer_invalid_destination (no key, version) REMOVE 2 S3FileTarget.resolve and the core validate the destination first. filesystem/test_s3.py::TestS3FileSystem::test_open_invalid_for_writing; filesystem/test_s3_core.py "Cannot write to a version" cases
TestS3MultipartWriter::test_writer_per_request_parameters_override_inherited_parameters REMOVE 7 No caller passes request_kwargs to S3MultipartWriter(). —
TestS3MultipartWriter::test_writer_requires_upload REMOVE 7 RuntimeError("not initialized") guards internal misuse that no public path can trigger. —
TestS3MultipartWriter::test_writer_requests REWRITE 7 Keep the Stubber check of per-operation filtering, checksum propagation and initiate() idempotency, pass the parameters per call as S3File does, and drop the params["ContentType"] = "changed" copy check. —

3. filesystem/test_s3_object.py

test change criteria reason covered by
TestS3Object::test_init_empty_applies_defaults REMOVE 6 Asserts the defaults size == 0 and storage_class == STANDARD. —
TestS3Metadata::test_init_defaults REMOVE 6 Default values for an empty response. —
TestS3ObjectVersion::test_init_delete_marker REMOVE 6 Asserts that fields absent from a delete-marker response read as None. —
TestS3MultipartUpload::test_init_without_list_fields REMOVE 6 Asserts that absent fields read as None. —
TestS3MultipartUploadPart::test_to_api_repr_omits_missing_checksums (copy x2) MERGE 2, 6 Omitting None values is one more case of the target (botocore rejects None); drop the six is None property asserts. into TestS3MultipartUploadPart::test_to_api_repr_with_checksum (kept unchanged)

4. filesystem/test_reader.py

test change criteria reason covered by
TestReadAheadStream::test_read_all MERGE 2 read() with no size is one more read_size value; add -1. into TestReadAheadStream::test_read
TestReadAheadStream::test_default_part_size REMOVE 1, 6 Monkeypatches the DEFAULT_* class constants and asserts that they are used. TestReadAheadStream::test_parts_in_flight
TestReadAheadStream::test_invalid_options (3 cases) REWRITE 6, 8 Keep the two documented ValueError cases and the half-initialized finalization check; drop {"part_size": "7"}, which matches Python's comparison TypeError text. —
TestReadAheadStream::test_interrupted_wait REMOVE 1, 7 A fake future raises from exception(), assuming how the stream waits; reading after Ctrl-C is not a documented contract. —
TestReadAheadStream::test_interrupted_close REMOVE 1, 3, 7 Patches the module wait and checks a retried close from a second thread with join(0.2). TestReadAheadStream::{test_close_waits_for_running_reads, test_close_with_queued_reads}

filesystem/test_init.py has no findings.

Notes

  • Source fact outside test scope: S3FileSystem._finish_multipart_upload(..., abort=True) has no caller at master 59169c5. Its only caller, _copy_object_with_multipart_upload (pyathena/filesystem/s3.py:1706), passes abort=False and runs its own cleanup. Move S3File writes onto a multipart write session #1232 (fb54bea) leaves the method and that caller unchanged and moves the S3File commit wait into _S3WriteSession.
    • TestS3FileSystem::{test_finish_multipart_upload_aborts_on_failure, test_finish_multipart_upload_waits_for_running_parts, test_finish_multipart_upload_does_not_wait_for_cancelled_parts, test_finish_multipart_upload_abort_failure_does_not_mask_the_original_error, test_finish_multipart_upload_stops_at_failed_part} exercise only the abort=True branch. The first three are removed; the last two are rewritten to run through _copy_object_with_multipart_upload.
    • TestS3FileSystem::test_finish_multipart_upload and test_finish_multipart_upload_without_abort are removed; test_finish_multipart_upload_request_parameters, which also reaches the abort=True branch, is rewritten through fs.open().
    • After this issue no test calls _finish_multipart_upload directly, so a source change can drop the abort parameter and the branch.
  • Borderline KEEPs:
    • TestS3FileSystem::test_setxattr_omits_unset_system_metadata stays: it is the public setxattr() check through Stubber, and the async and core sub-issue removes its core twin.
    • TestS3FileSystem::test_clear_multipart_uploads_preserves_unclassified_file_not_found stays: only it requires a FileNotFoundError without a ClientError cause to propagate unchanged (raised.value is error).
    • filesystem/test_s3_writer.py::TestS3MultipartWriter::test_writer_invalid_block_size stays: S3MultipartWriter is documented public API (docs/filesystem.md), and fs.open never reaches its own block-size check.
    • filesystem/test_s3_object.py::TestS3MultipartUploadPart::test_to_api_repr_with_checksum stays with its 20 cases: it is the only test of the 10 checksum field names.
    • TestS3FileSystem::test_get_client_compatible_with_s3fs and test_get_client_compatible_with_s3fs_profile read the private fs._client because no public way exists to observe the endpoint or credentials offline.
    • TestS3FileSystem::{test_info_does_not_use_listing_of_path, test_info_version_aware_heads_listed_file, test_exists_version_ignores_cached_parent_listing, test_find_directory_without_extra_requests} assert request counts, which are the caching contract; each also asserts the public result.
    • TestS3FileSystem::{test_rm_errors, test_rm_request_error_keeps_errors} match full error strings and __notes__; the match could be loosened to the key list.
    • TestS3FileSystem::{test_copy_object_with_multipart_upload_failed_part_stops_running_parts, test_failed_read_stops_running_reads, test_chmod_recursive_failure_stops_running_requests} and TestS3File::{test_discard_stops_running_parts, test_interrupted_creation_stops_retries} assert elapsed < EVENT_TIMEOUT (10 s, one-sided).
    • TestS3FileSystem::test_copy_object_with_multipart_upload_interrupted_creation sends a real SIGINT from a helper thread; it is the one sync check that Ctrl-C during a multipart copy aborts the upload.
    • TestS3FileSystem::test_copy_object_with_multipart_upload_part_sizes (max_workers 1, 4): both cases are now identical and could be one.
    • TestS3FileSystem::test_open_multipart_with_checksum: its (None, None) case duplicates test_write_transaction[10485760] and test_pipe_cat[6-1048576].
    • TestS3FileSystem::test_chmod_validation uses the real fs fixture but sends no request; it could run offline.
    • TestS3File::{test_part_scheduling_interrupted, test_discard_waits_for_running_parts} patch executor.submit or the module wait, but assert the event order ["part 1 ended", "abort"] (at Move S3File writes onto a multipart write session #1232), which is the contract.
    • filesystem/test_init.py::test_register_s3_filesystem_overwrites_existing_registration matches the log text "will be overwritten"; asserting a WARNING record from pyathena.filesystem would be less brittle.
  • Unused after the change:
    • filesystem/test_s3.py: the S3PathPairing import (only test_copy_pairs_destination_lookup uses it; the merged assertion goes through copy()); S3Executor from the s3_executor import (only test_pipe_file_failed_write_aborts_multipart_upload before its REWRITE); FIRST_EXCEPTION (only test_finish_multipart_upload_waits_for_running_parts); the bz2 and lzma imports (only TestCompressedBuffer::test_compress).
    • filesystem/test_reader.py: _InterruptedFuture, _InterruptedWaitExecutor, and the Future and wait imports.
    • filesystem/test_s3_writer.py: the **kwargs passthrough of _make_writer after the test_writer_requests rewrite. In the source, the request_kwargs parameter of S3MultipartWriter.__init__ then has no caller.
    • Re-check each against the Move S3File writes onto a multipart write session #1232 head, which changes the imports of filesystem/test_s3.py.
  • 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