Skip to content

Proper test file clean up#1245

Merged
adamhathcock merged 1 commit intomasterfrom
adam/clean-up-tests
Mar 6, 2026
Merged

Proper test file clean up#1245
adamhathcock merged 1 commit intomasterfrom
adam/clean-up-tests

Conversation

@adamhathcock
Copy link
Owner

This pull request refactors the way temporary scratch directories are handled in the TestBase class for the test suite. Instead of using static, shared directories for scratch files, each test now uses its own unique temporary directory, which improves test isolation and cleanup reliability. The code for creating, resetting, and deleting these directories has also been made more robust.

Improvements to test directory management:

  • Each test now creates its own unique temporary directory under the system temp path, rather than using shared static directories. This reduces the risk of test interference and improves parallel test execution.
  • The scratch directory creation and cleanup logic has been moved from static fields to instance fields, and the static directory creation in the class constructor has been removed. [1] [2]

Robustness and cleanup enhancements:

  • The DisposeAsync method now calls a new DeleteScratchDirectory helper, which attempts to delete the test's temp directory and throws a clear exception if cleanup fails.
  • The CleanScratch method now uses a new ResetScratchDirectory helper to safely reset both scratch directories for the test.
  • Error handling has been added to directory deletion to provide informative exceptions if the directory cannot be deleted, improving debuggability of test failures related to file system issues.

Copilot AI review requested due to automatic review settings March 6, 2026 14:58
@kilo-code-bot
Copy link
Contributor

kilo-code-bot bot commented Mar 6, 2026

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 0

Changes Reviewed

This PR improves test isolation by:

  • Using per-test-instance temp directories instead of shared static directories
  • Creating unique scratch paths for each test run using GUIDs
  • Adding robust error handling for directory cleanup with descriptive exceptions

Files Reviewed (1 file)

  • tests/SharpCompress.Test/TestBase.cs - Improved test isolation with temp directories

Notes

The DeleteScratchDirectory method catches IOException and UnauthorizedAccessException during cleanup, which are the most common failure modes. Other exceptions from Directory.Delete (such as ArgumentException or PathTooLongException) would propagate unhandled, which is reasonable behavior since these indicate programming errors rather than test environment issues.

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

This PR refactors TestBase scratch-directory handling to improve test isolation by moving from shared, solution-relative scratch folders to per-test unique temp directories under the system temp path, and centralizes cleanup/reset logic.

Changes:

  • Replace static shared Scratch/Scratch2 directories with per-test directories under Path.GetTempPath().
  • Add helper methods to reset scratch directories and to delete the entire per-test temp root during teardown.
  • Improve teardown debuggability by throwing a clearer exception when temp cleanup fails.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants