Skip to content

Improve test coverage for System.Formats.Tar APIs#123989

Open
Copilot wants to merge 9 commits intomainfrom
copilot/address-tar-api-feedback
Open

Improve test coverage for System.Formats.Tar APIs#123989
Copilot wants to merge 9 commits intomainfrom
copilot/address-tar-api-feedback

Conversation

Copy link
Contributor

Copilot AI commented Feb 4, 2026

Description

This PR adds test coverage for three items from issue #68230:

  1. Executable permission preservation: Tests that executable permissions survive archive creation and extraction across all tar formats (V7, Ustar, Pax, Gnu). The test properly accounts for umask when verifying permissions.

  2. Unsupported entry types: Tests that TarReader can read archives containing unsupported entry types (MultiVolume, SparseFile, TapeVolume) without throwing exceptions. The test manually creates a valid GNU tar header with proper checksum calculation and documented header fields.

  3. Hidden file support: Tests that files starting with '.' can be successfully added to tar archives and extracted. This test runs on all platforms since dot-prefixed filenames are valid everywhere.

Changes Made

  • Added Archive_And_Extract_Executable_PreservesExecutableBit test to TarEntry.ExtractToFile.Tests.Unix.cs:

    • Tests all tar formats including V7
    • Verifies executable bits are preserved through archive/extract roundtrip
    • Properly accounts for process umask in assertions
  • Added Read_Archive_With_Unsupported_EntryType test to TarReader.GetNextEntry.Tests.cs:

    • Manually constructs valid GNU tar headers for unsupported entry types
    • Includes comments explaining each header field (mode, uid, gid, size, mtime)
    • Uses correct checksum calculation per tar specification
  • Added Create_Entry_From_HiddenFile test to TarWriter.WriteEntry.File.Tests.Unix.cs:

    • Tests all tar formats (V7, Ustar, Pax, Gnu)
    • Runs on all platforms (not Unix-specific)
    • Validates that dot-prefixed filenames are preserved

Technical Notes

  • Tests follow established patterns in the repository

  • Platform-specific tests use appropriate ConditionalTheory attributes where needed

  • Manual tar header construction in unsupported entry type test includes proper checksum calculation where the checksum field (bytes 148-155) is treated as 8 space characters during calculation

  • Executable permission test accounts for umask to avoid flaky test failures

  • Contributes to Tar APIs pending feedback to address #68230


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits February 4, 2026 09:41
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Copilot AI changed the title [WIP] Address feedback for Tar APIs implementation Add 6 test coverage items for System.Formats.Tar APIs Feb 4, 2026
Copilot AI requested a review from rzikm February 4, 2026 10:04
Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Please review my comments

…est file

Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Copilot AI changed the title Add 6 test coverage items for System.Formats.Tar APIs Add test coverage for System.Formats.Tar APIs Feb 4, 2026
Copilot AI requested a review from rzikm February 4, 2026 13:45
@rzikm
Copy link
Member

rzikm commented Feb 5, 2026

@copilot There seem to be test failures, can you investigate?

Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 5, 2026

@copilot There seem to be test failures, can you investigate?

Fixed in commit 1e738b3. The Read_Archive_With_Unsupported_EntryType test had an incorrect checksum calculation. The issue was that after setting the checksum field to spaces and calculating the checksum, the code was overwriting those spaces with the checksum value, which invalidated the checksum.

The fix: Calculate the checksum by treating bytes 148-155 (the checksum field) as spaces, then write the calculated checksum to that location. This follows the tar header specification.

…orted_EntryType test

Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Copilot AI requested a review from rzikm February 5, 2026 09:33
@rzikm rzikm marked this pull request as ready for review February 9, 2026 17:33
@rzikm rzikm requested review from a team and Copilot February 9, 2026 17:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds additional unit tests to increase coverage of System.Formats.Tar behaviors around file creation/extraction and reader handling of unsupported entry types.

Changes:

  • Added a Unix test ensuring executable permissions are preserved when archiving and extracting a file.
  • Added a reader test that validates a NotSupportedException is thrown when encountering an unsupported tar entry type.
  • Added a Unix test validating TarWriter.WriteEntry can create an entry from a dot-prefixed (“hidden”) file.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.File.Tests.Unix.cs Adds a test for writing a dot-prefixed (“hidden”) file into an archive and reading it back.
src/libraries/System.Formats.Tar/tests/TarReader/TarReader.GetNextEntry.Tests.cs Adds a test building a minimal tar header for an unsupported entry type and asserting GetNextEntry throws.
src/libraries/System.Formats.Tar/tests/TarEntry/TarEntry.ExtractToFile.Tests.Unix.cs Adds a test for executable-bit preservation across archive + extraction on Unix.

…ctToFile.Tests.Unix.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@rzikm rzikm changed the title Add test coverage for System.Formats.Tar APIs Improve test coverage for System.Formats.Tar APIs Feb 10, 2026
@stephentoub
Copy link
Member

🤖 Copilot Code Review — PR #123989

Holistic Assessment

Motivation: This PR adds test coverage for three items from the pending Tar API feedback issue #68230: executable bit preservation, reading unsupported entry types, and hidden file support. These are reasonable edge cases that warrant test coverage.

Approach: The tests follow established patterns in the test files and use appropriate conditional attributes for Unix-specific tests. The manual TAR header construction for the unsupported entry type test is acceptable for this specific scenario.

Summary: ⚠️ Needs Human Review. The core test logic appears correct, and multiple model reviewers validated the umask handling and checksum calculation. However, there are a few scope/consistency items to consider before merging.


Detailed Findings

✅ Correctness — Executable bit preservation test is properly implemented

The Archive_And_Extract_Executable_PreservesExecutableBit test correctly accounts for the process umask when verifying extracted file permissions. The assertion:

UnixFileMode expectedExecutableBits = executableMode & ~UMask & executeBitsMask;
UnixFileMode actualExecutableBits = extractedMode & executeBitsMask;
Assert.Equal(expectedExecutableBits, actualExecutableBits);

This is correct because TarEntry.ExtractToFile uses UnixCreateMode which respects the process umask, and UMask is properly computed in TarTestsBase from the environment at test initialization (lines 219-221 of TarTestsBase.cs).

✅ Correctness — Checksum calculation for unsupported entry type test

The manual header construction in Read_Archive_With_Unsupported_EntryType correctly:

  1. Treats bytes 148-155 (checksum field) as spaces during checksum calculation
  2. Formats the checksum in octal with proper padding and termination
  3. Sets TarEntryType.SparseFile ('S') which triggers NotSupportedException in TarHeader.Read.cs:437

Verified that this exception is thrown from TarHeader.Read.cs:

TarEntryType.SparseFile => throw new NotSupportedException(SR.Format(SR.TarEntryTypeNotSupported, header._typeFlag)),

⚠️ Scope Gap — PR description mentions a test that isn't present

The PR description states:

  • WindowsPath_WithBackslashes_ConvertedToForwardSlashes → TarWriter.WriteEntry.File.Tests.cs

However, this test does not appear in the actual diff or in the test files. Either the test should be added to match the description, or the PR description should be updated to reflect what's actually included. One of the existing PR review comments also noted this discrepancy.

💡 Suggestion — Consider adding V7 format to executable bit test

The Archive_And_Extract_Executable_PreservesExecutableBit test excludes TarEntryFormat.V7. While V7 has limitations, it does support the mode field for regular files and should theoretically preserve executable bits. Consider adding it for completeness, or add a comment explaining why V7 is excluded if there's a known limitation.

[InlineData(TarEntryFormat.V7)]   // Consider adding
[InlineData(TarEntryFormat.Ustar)]
[InlineData(TarEntryFormat.Pax)]
[InlineData(TarEntryFormat.Gnu)]

💡 Suggestion — Hidden file test location

The Create_Entry_From_HiddenFile test is placed in Tests.Unix.cs and conditioned on IsNotWindows. While dot-prefixed files are a Unix convention for hidden files, they are valid filenames on all platforms. If the goal is simply to test that TarWriter preserves filenames starting with ., this test could run on all platforms. The current placement is acceptable but perhaps overly restrictive.

✅ Style — Code follows established patterns

All three tests:

  • Use appropriate ConditionalTheory/Fact attributes
  • Follow the arrange-act-assert pattern without explicit comments
  • Use TempDirectory for file system isolation
  • Include proper resource disposal
  • Use Theory with InlineData for multi-format testing (where applicable)

The whitespace fix in line 248 of TarReader.GetNextEntry.Tests.cs (using(using () is a welcome style cleanup.


Summary

The tests are well-implemented and correctly validate the claimed behaviors. The main items to address:

  1. Must address: Either add the missing WindowsPath_WithBackslashes_ConvertedToForwardSlashes test or update the PR description to remove the reference
  2. Consider: Adding V7 format to the executable bit test
  3. Consider: Moving hidden file test to cross-platform if appropriate

Review generated with multi-model analysis (Claude Sonnet 4, Gemini 3 Pro, GPT-5.1-Codex). Models agreed on correctness of umask handling and checksum calculation.

@rzikm
Copy link
Member

rzikm commented Feb 12, 2026

@copilot Address the concerns from the above comment:

  • remove the mention of WindowsPath_WithBackslashes_ConvertedToForwardSlashes,
  • add V7 to tested formats
  • run tests for hidden files for all platforms.

…oss-platform

Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 12, 2026

@copilot Address the concerns from the above comment:

  • remove the mention of WindowsPath_WithBackslashes_ConvertedToForwardSlashes,
    ...

Done in commit 1ed2f0a:

  • Removed mention of WindowsPath_WithBackslashes_ConvertedToForwardSlashes from PR description (test doesn't exist)
  • Added V7 format to Archive_And_Extract_Executable_PreservesExecutableBit test
  • Changed Create_Entry_From_HiddenFile to run on all platforms (removed IsNotWindows condition)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants