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.
With async: true, this merged test still probes the exhausted DataStream only through synchronous Read. The deleted async test used ReadAsync and Assert.ThrowsAsync, so the async data-stream failure paths (both before and after reader disposal) are no longer covered. Branch these assertions on async and use the async read/throws APIs for the async case.
The async case now reaches this test, but it reads and disposes the returned entry stream with synchronous APIs. The removed async version exercised ReadAsync followed by DisposeAsync; keep those calls conditional on async so this test still covers the asynchronous data-stream lifecycle.
This parameterized async case also uses synchronous Read and Dispose on the entry stream. The deleted async test covered ReadAsync/DisposeAsync for the partially-read stream, so the async branch has lost that coverage; dispatch both operations according to async.
Although this test is now run with async: true, the returned stream is still disposed synchronously. The removed async variant used DisposeAsync, so the no-read asynchronous disposal path is no longer exercised; select DisposeAsync for the async case.
This provider duplicates the format list already centralized in GetTarEntryFormats()/GetTarEntryFormatsAndBooleanData() below. If a new TarEntryFormat is added, the tests using GetFormatBooleanData will silently omit it; delegate to the existing provider (or replace the call sites with GetTarEntryFormatsAndBooleanData) so there is only one list to maintain.
[!NOTE] This review comment was created by GitHub Copilot.
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.
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