Skip to content

Improve test quality: CareMetrics.Tests/UnitTests.cs#48

Draft
Copilot wants to merge 2 commits intomainfrom
copilot/xunit-expert-improve-test-quality-again
Draft

Improve test quality: CareMetrics.Tests/UnitTests.cs#48
Copilot wants to merge 2 commits intomainfrom
copilot/xunit-expert-improve-test-quality-again

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 17, 2026

UnitTests.cs had a placeholder test, duplicated solution-root discovery logic, a monolithic integration test, and no coverage for ParseDirectory or ParseFile edge cases.

Changes

  • Removed Truth_IsTrue() placeholder
  • Extracted SolutionRootFixture — solution-root walking moved from two test methods into a shared IClassFixture<SolutionRootFixture>, injected via constructor
  • Split Parser_CanParseRealVektisFile into three focused tests: HasExpectedHeaders, InsuredColumnParsesAsPositiveDecimal, ProducesRecords
  • Added [Theory]/[InlineData] edge cases for ParseFile — no year in filename, empty file content, header-only CSV; each runs in an isolated temp directory
  • Added ParseDirectory tests — happy path against real data dir, and empty directory → empty list
  • Removed redundant reader.Close() (superseded by using)
  • Converted to file-scoped namespace to match UnitTest1.cs
[Theory]
[InlineData("no_year_here.csv", "")]          // no year in filename → year=0 → empty
[InlineData("data_2023.csv", "")]             // empty file → no header → empty
[InlineData("data_2023.csv", "col1;col2\n")] // header only, no data rows → empty
public void ParseFile_EdgeCases_ReturnsEmptyList(string fileName, string content)
{
    var tempDir = Path.Combine(Path.GetTempPath(), $"vektis_test_{Guid.NewGuid():N}");
    Directory.CreateDirectory(tempDir);
    try
    {
        File.WriteAllText(Path.Combine(tempDir, fileName), content);
        Assert.Empty(VektisCsvParser.ParseFile(Path.Combine(tempDir, fileName)));
    }
    finally { Directory.Delete(tempDir, recursive: true); }
}

Test count: 22 → 28, all passing.

Original prompt

This section details on the original issue you should resolve

<issue_title>[xunit-expert] Improve Test Quality: CareMetrics.Tests/UnitTests.cs</issue_title>
<issue_description>The test file CareMetrics.Tests/UnitTests.cs has been selected for quality improvement by the daily xUnit Test Quality Expert. This issue provides specific, actionable recommendations to enhance test quality, coverage, and maintainability using xUnit best practices for .NET.

Current State

Attribute Value
Test File CareMetrics.Tests/UnitTests.cs
Source File(s) CareMetrics.API/Services/VektisCsvParser.cs, CareMetrics.API/Services/VektisDataService.cs
Test Methods 3 [Fact] methods (0 [Theory])
Lines of Code 110 lines
Last Modified 2026-03-17

Test Quality Analysis

Strengths ✅

  1. Real-data integration testingParser_CanParseRealVektisFile validates actual CSV file structure (header columns, column count, decimal parsing) against a real Vektis data file, making it a high-value regression guard.
  2. DI container integrationService_CanLoadSampleCsv_FromConfiguration exercises the full ServiceCollectionIVektisDataService wiring, ensuring the service can be resolved with real configuration.
  3. Diagnostic assertion messages — Both integration tests include useful failure context (e.g., $"Expected >10k records, got {all.Count}") which aids debugging in CI.

Areas for Improvement 🎯

1. Placeholder Test — Remove Truth_IsTrue

Current Issue:

[Fact]
public void Truth_IsTrue()
{
    Assert.True(true);  // ❌ placeholder — zero test value
}

Assert.True(true) always passes regardless of any code change. It adds noise to test output and masks the true coverage picture.

Recommended Fix:

  • Delete this method entirely. It provides no test coverage and inflates the test count.

Why this matters: Placeholder tests are a common smell that erodes trust in the test suite. Test runners show a false "green" even when real tests are broken.

2. Duplicated Solution-Root Discovery Logic

Current Issue:

Both Parser_CanParseRealVektisFile and Service_CanLoadSampleCsv_FromConfiguration contain an identical 4-line loop to walk up the directory tree:

// Duplicated in BOTH test methods ❌
var dir = new DirectoryInfo(AppContext.BaseDirectory);
while (dir != null && !File.Exists(Path.Combine(dir.FullName, "CareMetrics.slnx")))
    dir = dir.Parent;
Assert.NotNull(dir);
var solutionRoot = dir!.FullName;

Recommended Fix:

Extract to a shared fixture or a static helper method:

// ✅ In a shared fixture class
public sealed class SolutionRootFixture
{
    public string SolutionRoot { get; }

    public SolutionRootFixture()
    {
        var dir = new DirectoryInfo(AppContext.BaseDirectory);
        while (dir != null && !File.Exists(Path.Combine(dir.FullName, "CareMetrics.slnx")))
            dir = dir.Parent;

        if (dir == null)
            throw new InvalidOperationException("Could not locate solution root (CareMetrics.slnx).");

        SolutionRoot = dir.FullName;
    }
}

// ✅ Use in test class
public class UnitTests : IClassFixture(SolutionRootFixture)
{
    private readonly string _solutionRoot;

    public UnitTests(SolutionRootFixture fixture)
        => _solutionRoot = fixture.SolutionRoot;
}

Why this matters: DRY principle — if the detection logic ever needs to change (e.g., a different solution file name), it currently needs updating in two places.

3. Missing Tests for VektisCsvParser.ParseDirectory()

Current Issue:

VektisCsvParser exposes two public static methods:

  • ParseFile(string path) — ✅ tested (indirectly) in Parser_CanParseRealVektisFile
  • ParseDirectory(string directory) — ❌ zero test coverage

ParseDirectory has its own logic (iterates files, swallows individual exceptions) that is not exercised at all.

Recommended Tests:

[Fact]
public void ParseDirectory_WithVektisDataDir_ReturnsAllRecords()
{
    // Arrange
    var csvDir = Path.Combine(_solutionRoot, "CareMetrics.API", "Data", "vektis");
    Assert.True(Directory.Exists(csvDir));

    // Act
    var records = VektisCsvParser.ParseDirectory(csvDir);

    // Assert
    Assert.NotEmpty(records);
    Assert.All(records, r => Assert.False(string.IsNullOrEmpty(r.CareType)));
}

[Fact]
public void ParseDirectory_EmptyDirectory_ReturnsEmptyList()
{
    // Arrange
    var emptyDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
    Directory.CreateDirectory(emptyDir);

    try
    {
        // Act
        var records = VektisCsvParser.ParseDirectory(emptyDir);

        // Assert
        Assert.Empty(records);
    }
    finally
    {
        Directory.Delete(emptyDir);
   ...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

- Fixes rowantervelde/agentic-development#47

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 Send tasks to Copilot coding agent from [Slack](https://gh.io/cca-slack-docs) and [Teams](https://gh.io/cca-teams-docs) to turn conversations into code. Copilot posts an update in your thread when it's finished.

Co-authored-by: rowantervelde <22254598+rowantervelde@users.noreply.github.com>
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