You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Introduce helper methods and Get_Boolean_Data() that allow Tar tests to
be parameterized by a bool to exercise both sync and async code paths
from a single test method. Follows the same pattern used by ZipArchive
tests (see ZipTestHelper.cs). The bool async parameter is always last.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Merge TarReader async test files into their sync counterparts using the
bool async parameter pattern with [MemberData(nameof(Get_Boolean_Data))].
Merged and deleted:
- TarReader.Async.Tests.cs
- TarReader.GetNextEntryAsync.Tests.cs
- TarReader.File.Async.Tests.cs + Base
- TarReader.File.GlobalExtendedAttributes.Async.Tests.cs
- TarReader.TarEntry.ExtractToFileAsync.Tests.cs (+ Unix)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR restructures System.Formats.Tar tests to remove duplicated sync vs async variants by introducing shared helpers and updating existing tests to exercise both code paths within a single test definition (typically via a bool async parameter or a loop over { false, true }). It also removes now-redundant *Async* test files and updates the test project file accordingly.
Changes:
Added TarTestsBase.Helpers.cs with helper methods to abstract sync/async Tar APIs and provide boolean theory data.
Migrated many TarWriter/TarReader/TarFile/TarEntry tests to cover both sync and async paths without separate *Async* test files.
Removed a set of redundant async-only test files and updated System.Formats.Tar.Tests.csproj compile items.
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 3 comments.
The new async parameter only controls extraction here; the verification loop still calls TarReader.GetNextEntry() synchronously. As a result, the async: true case does not cover the asynchronous reader path. Use TarReaderHolder and GetNextEntry(reader, async: async) for this verification, matching the other merged sync/async tests.
This sync-only overload has no call sites in the System.Formats.Tar tests; every CreateTarReader use resolves to the TarReaderHolder overload with an async flag. Keeping it adds dead API surface to the shared test base and makes overload selection harder to understand; remove it.
The new async parameter selects ExtractToDirectory, but this verification still constructs a plain TarReader and calls synchronous GetNextEntry(). Thus the async: true case does not exercise asynchronous reading in this test. Use CreateTarReader/TarReaderHolder and GetNextEntry(reader, async: async) for the verification loop.
Async round-trip verification does not exercise GetNextEntryAsync
This test now parameterizes archive extraction and uses asynchronous stream reads when async is true, but the archive verification still uses a synchronous TarReader at line 296. The async case therefore misses GetNextEntryAsync; create the reader through TarReaderHolder with the same flag so the round-trip covers both reader paths.
CreateTarReader(Stream, bool leaveOpen = false) has no call sites; every reader construction in the updated tests uses the overload that accepts the sync/async flag. This dead overload adds another construction path without supporting the deduplication pattern, so remove it rather than retaining unused helper code.
The async cases now reopen the generated archive with File.OpenRead, which creates a synchronous FileStream, so the GetNextEntryAsync tests no longer exercise the asynchronous FileStream strategy or async disposal. The deleted async file tests used FileStreamOptions with FileOptions.Asynchronous and await using; preserve that setup for async: true (and apply it to the other readback blocks in this file) so this deduplication does not remove the async I/O coverage.
Changing this from a using declaration to manual disposal leaves the reader undisposed whenever an assertion or GetNextEntry call throws before the cleanup block. In the file-backed callers that can leave a handle open and interfere with temporary-directory cleanup on Windows. Use TarReaderHolder with await using (or a try/finally) so disposal is guaranteed on failures.
This overload is unused: every CreateTarReader call in the test tree uses the TarReaderHolder overload below, so retaining this overload adds dead helper API and makes the overload set harder to understand. Remove the unused overload.
TarTestsBase.cs references helper members defined in TarTestsBase.Helpers.cs
and other TarTestsBase.*.cs partial files, but the Manual test project
only included TarTestsBase.cs, causing CS0246/CS0103 build errors.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
<!-- -->
TAR extraction currently requires the destination directory to exist,
unlike ZIP extraction. This change allows callers to extract into a
missing destination without creating it first, while preserving lazy
directory creation for empty archives.
Remove the destination-existence checks and rely on TAR's existing
per-entry extraction helpers to create destination and parent
directories as needed. This covers all eight overloads:
synchronous/asynchronous, file/stream inputs, and `bool
overwriteFiles`/`TarExtractOptions`. Existing directories remain
accepted regardless of the overwrite setting, and file-overwrite
behavior is unchanged. Path-containment checks, link handling,
permissions, cancellation, and stream ownership continue through the
existing implementation. ZIP is unchanged.
Update XML documentation and add shared regression coverage for
existing/missing destinations, missing parents, empty archives, nested
files and directory entries, file collisions, files occupying required
directory paths, invalid paths, and traversal through file/link entries.
Cancellation tests cover both overload families. The tests use main's
consolidated sync/async infrastructure following the merge of #130407.
### Validation
- Libraries baseline and TAR builds succeeded on Windows x64 using a
short `subst` path.
- Before the fix, 212 targeted cases failed.
- After merging main at `6fe5c26c9be`, the TAR build succeeded and the
full default Windows TAR suite passed 6,956 tests with zero failures,
including all 344 destination regression cases and four cancellation
cases.
- Unix-specific tests were not run.
### Compatibility
This is a behavioral compatibility change: callers that previously
received `DirectoryNotFoundException` for a missing destination now get
successful extraction, or success without directory creation for an
empty archive. Callers requiring an existing destination must validate
that precondition explicitly.
The PR is labeled `needs-breaking-change-doc-created` for the
repository's post-merge compatibility-documentation process.
> [!NOTE]
> This PR description and the code changes were generated with GitHub
Copilot.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
rzikm
left a comment
Fixes #127379