feat: add simple overload for attaching artifacts with optional display name and description

[thomhurst]

No description provided.

[claude]

Code Review - PR #4078

Summary

This PR adds a convenient overload for attaching test artifacts with just a file path, making the API more user-friendly. The implementation is clean and follows TUnit conventions.

---

✅ Strengths

1. Good API Design: The new overload provides excellent ergonomics by reducing boilerplate for the common case of attaching a file with defaults.

2. Proper Documentation:

   • XML comments are clear and consistent with existing style (TUnit.Core/Interfaces/ITestOutput.cs:50-58)
   • Documentation updates show good before/after examples (docs/docs/test-lifecycle/artifacts.md)

3. Snapshot Testing Compliance: All public API snapshots have been correctly updated across all target frameworks (✓ .NET 8.0, 9.0, 10.0, .NET Framework 4.7)

4. Code Style:

   • Modern C# syntax with collection expressions
   • Follows TUnit naming conventions
   • Proper null-coalescing for default values (TestContext.Output.cs:40)

5. Test Coverage: Integration tests demonstrate both simple and parameterized usage patterns

---

🔍 Issues & Recommendations

Critical: Missing Input Validation

void ITestOutput.AttachArtifact(string filePath, string? displayName, string? description)
{
    var fileInfo = new FileInfo(filePath);  // ⚠️ No validation
    _artifactsBag.Add(new Artifact
    {
        File = fileInfo,
        DisplayName = displayName ?? fileInfo.Name,
        Description = description
    });
}

Problems:

1. No null check on filePath - Will throw ArgumentNullException from FileInfo constructor
2. No file existence validation - Creates FileInfo even if file doesn't exist (may be intentional for deferred validation, but should be documented)
3. No path validation - Invalid characters or malformed paths will throw exceptions

Recommendation:

void ITestOutput.AttachArtifact(string filePath, string? displayName, string? description)
{
    ArgumentException.ThrowIfNullOrWhiteSpace(filePath);  // C# 11+
    
    var fileInfo = new FileInfo(filePath);
    
    // Optional: Validate file exists (if this is a requirement)
    // if (!fileInfo.Exists)
    //     throw new FileNotFoundException($"Artifact file not found: {filePath}", filePath);
    
    _artifactsBag.Add(new Artifact
    {
        File = fileInfo,
        DisplayName = displayName ?? fileInfo.Name,
        Description = description
    });
}

---

Issue: Inconsistent API Surface

The existing AttachArtifact(Artifact) method accepts an Artifact where DisplayName is required:

public class Artifact
{
    public required FileInfo File { get; init; }
    public required string DisplayName { get; init; }  // ⚠️ Required
    public string? Description { get; init; }
}

But the new overload makes displayName optional (defaults to file name). This creates an inconsistency:

// Original API: DisplayName required (compile error if omitted)
AttachArtifact(new Artifact { File = ..., DisplayName = "..." });

// New API: DisplayName optional
AttachArtifact("path.txt");  // Uses file name

Recommendation: This is acceptable if intentional (convenience overload provides sensible default), but consider whether DisplayName should be nullable in the Artifact class for consistency.

---

Minor: Potential Performance Consideration

DisplayName = displayName ?? fileInfo.Name

FileInfo.Name performs a string allocation on every access (it's a property that extracts the file name from the full path). In hot paths, consider caching:

var fileName = fileInfo.Name;
_artifactsBag.Add(new Artifact
{
    File = fileInfo,
    DisplayName = displayName ?? fileName,
    Description = description
});

Impact: Negligible for artifact attachment (not a hot path), but good practice.

---

Testing Gap: No Unit Tests

The PR includes integration tests in TUnit.TestProject, but no unit tests specifically for the new overload behavior. Consider adding tests for:

• ✅ Default display name uses file name
• ✅ Custom display name overrides default
• ✅ Description is properly set/null
• ❌ Null/empty file path handling (currently missing - will throw)
• ❌ Non-existent file handling (behavior unclear)
• ❌ Invalid path handling

Recommendation: Add unit tests in TUnit.Engine.Tests or TUnit.Core.Tests to verify edge cases.

---

Documentation: Missing Error Behavior

The XML documentation doesn't specify what happens if:

• filePath is null/empty
• The file doesn't exist
• The path contains invalid characters

Recommendation: Update XML comments to document expected behavior:

/// <param name="filePath">The path to the file to attach</param>
/// <exception cref="ArgumentNullException">When <paramref name="filePath"/> is null</exception>
/// <exception cref="ArgumentException">When <paramref name="filePath"/> is empty or contains invalid characters</exception>

---

📊 Checklist Against CLAUDE.md

Rule	Status	Notes
Dual-Mode Implementation	✅ N/A	Runtime-only feature (doesn't require source-gen)
Snapshot Testing	✅ Pass	All .verified.txt files updated correctly
Modern C# Syntax	✅ Pass	Uses proper conventions
Performance	✅ Pass	Not a hot path, acceptable
AOT Compatibility	✅ Pass	No reflection or trimming concerns
Code Style	✅ Pass	Follows TUnit conventions

---

🎯 Verdict

Approve with Recommendations

The core implementation is solid and provides good developer experience. However, input validation is critically missing and should be added before merging to prevent runtime exceptions from common mistakes (null paths, typos, etc.).

Before merging:

1. ✅ Add null/empty validation for filePath parameter
2. ⚠️ Consider adding unit tests for edge cases
3. ⚠️ Document exception behavior in XML comments

Nice-to-have:

• Decide if file existence should be validated (and document the decision)
• Consider making Artifact.DisplayName nullable for consistency

Great work on the ergonomic improvement! 🚀

[Copilot]

Pull request overview

This PR adds a convenient overload to the AttachArtifact method that accepts a file path as a string along with optional display name and description parameters, simplifying the artifact attachment API for common use cases where users don't need to construct an Artifact object manually.

Key Changes:

• Added a new AttachArtifact overload that takes string parameters instead of requiring an Artifact object
• Updated documentation to demonstrate both the simple and full usage patterns
• Added test coverage for the new overload variations

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
TUnit.Core/Interfaces/ITestOutput.cs	Added new overload signature to ITestOutput interface with XML documentation
TUnit.Core/TestContext.Output.cs	Implemented the new overload that creates FileInfo and Artifact objects internally
docs/docs/test-lifecycle/artifacts.md	Updated documentation to show the simple overload first, then the full Artifact object approach
TUnit.TestProject/TestArtifactTests.cs	Added two new tests covering the simple overload with and without custom display name/description
TUnit.PublicAPI/*.verified.txt	Updated public API snapshots for all target frameworks to reflect the new method signature

[Copilot]

The filePath parameter should be validated for null or whitespace before creating a FileInfo object. Consider adding validation like ArgumentException.ThrowIfNullOrWhiteSpace(filePath) at the beginning of the method to provide a clear error message when an invalid path is provided, rather than allowing a potentially confusing exception from the FileInfo constructor.

Suggested change

	{
	{
	ArgumentException.ThrowIfNullOrWhiteSpace(filePath);