Skip to content

Refactor UnitTests.cs: remove placeholder, split parser tests, add ParseDirectory coverage, extract fixture#43

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

Refactor UnitTests.cs: remove placeholder, split parser tests, add ParseDirectory coverage, extract fixture#43
Copilot wants to merge 2 commits intomainfrom
copilot/xunit-expert-improve-test-quality

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 14, 2026

UnitTests.cs contained a Truth_IsTrue() placeholder, a monolithic parser test doing too much, duplicated solution-root discovery boilerplate, and zero coverage for VektisCsvParser.ParseDirectory.

Changes

  • Removed Truth_IsTrue() false-green placeholder
  • Extracted solution-root walk-up logic into IntegrationTestFixture : IClassFixture<T>, exposing SolutionRoot, VektisDataPath, and Postcode3DataPath; shared across both test classes
  • Split Parser_CanParseRealVektisFile into two focused tests inside a new VektisCsvParserTests class:
    • ParseFile_CsvHeaders_HasExpectedStructure — asserts column count and fixed column names
    • ParseFile_DataRows_AreProperlyParsed — asserts decimal parsing + ParseFile output records
  • Added ParseDirectory coverage:
    • ParseDirectory_ReturnsRecordsFromMultipleFiles — verifies records span ≥2 years
    • ParseDirectory_EmptyDirectory_ReturnsEmptyList — edge case with a temp dir
  • Added [Theory]/[InlineData] test Service_LoadedData_ContainsExpectedYear for years 2011 and 2023 in VektisDataServiceIntegrationTests
public class IntegrationTestFixture
{
    public string SolutionRoot { get; }
    public string VektisDataPath { get; }
    public string Postcode3DataPath { get; }
    // walks up from AppContext.BaseDirectory once; shared by all consuming test classes
}

public class VektisCsvParserTests : IClassFixture<IntegrationTestFixture>
{
    [Fact] public void ParseFile_CsvHeaders_HasExpectedStructure() {}
    [Fact] public void ParseFile_DataRows_AreProperlyParsed() {}
    [Fact] public void ParseDirectory_ReturnsRecordsFromMultipleFiles() {}
    [Fact] public void ParseDirectory_EmptyDirectory_ReturnsEmptyList() {}
}
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 xUnit Test Quality Expert. This issue provides specific, actionable recommendations to enhance test quality, coverage, and maintainability using xUnit best practices for .NET 10.

Current State

Property Value
Test File CareMetrics.Tests/UnitTests.cs
Source File(s) CareMetrics.API/Services/VektisCsvParser.cs, CareMetrics.API/Services/VektisDataService.cs
Test Methods 3 [Fact] / 0 [Theory] methods
Lines of Code 110 lines
Framework xUnit 2.x on .NET 10

Test Quality Analysis

Strengths ✅

  1. Integration tests provide real-world validationParser_CanParseRealVektisFile and Service_CanLoadSampleCsv_FromConfiguration exercise the actual CSV files on disk, catching real parsing regressions.
  2. Failure messages include diagnostic context — Use of $"Expected >{count} records, got {actual}" and hex-encoded byte output aids debugging when tests fail in CI.

Areas for Improvement 🎯

1. Remove Placeholder Test (HIGH PRIORITY)

Current Issue:
UnitTests.cs contains a placeholder test that always passes and asserts nothing:

[Fact]
public void Truth_IsTrue()
{
    Assert.True(true);
}

This is the classic xUnit anti-pattern. It inflates the test count, provides zero coverage signal, and can mask missing tests when developers see "all tests pass."

Recommended Change:
Delete this test entirely, or replace it with a meaningful test (e.g. testing VektisCsvParser with a known-good string or testing a boundary condition).

// ❌ REMOVE
[Fact]
public void Truth_IsTrue()
{
    Assert.True(true);
}

Why this matters: Placeholder tests erode trust in the test suite. When every test passes but one of them is Assert.True(true), the green badge means less.

2. Extract Duplicated Solution-Root Discovery (MEDIUM PRIORITY)

Current Issue:
Both Parser_CanParseRealVektisFile and Service_CanLoadSampleCsv_FromConfiguration repeat the same 5-line boilerplate to walk up the directory tree:

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 Change:
Extract to a shared helper in the test class, or introduce an IClassFixture(T) that resolves the solution root once:

public class IntegrationTestFixture
{
    public string SolutionRoot { get; }

    public IntegrationTestFixture()
    {
        var dir = new DirectoryInfo(AppContext.BaseDirectory);
        while (dir != null && !File.Exists(Path.Combine(dir.FullName, "CareMetrics.slnx")))
            dir = dir.Parent;
        SolutionRoot = dir?.FullName
            ?? throw new InvalidOperationException("Solution root not found");
    }

    public string VektisDataPath =>
        Path.Combine(SolutionRoot, "CareMetrics.API", "Data", "vektis");
}

public class UnitTests : IClassFixture(IntegrationTestFixture)
{
    private readonly IntegrationTestFixture _fixture;
    public UnitTests(IntegrationTestFixture fixture) => _fixture = fixture;

    [Fact]
    public void Parser_CanParseRealVektisFile()
    {
        var csvDir = Path.Combine(_fixture.VektisDataPath, "postcode3");
        // ... rest of test, no more root-finding boilerplate
    }
}

Why this matters: Follows the DRY principle, and IClassFixture(T) is the xUnit-idiomatic pattern for shared setup — the fixture is instantiated once per test class.

3. Split Overly-Large Test Method (MEDIUM PRIORITY)

Current Issue:
Parser_CanParseRealVektisFile tests multiple independent concerns in a single method:

  1. CSV file existence
  2. CSV header count and column names
  3. Decimal parsing of the aantal_verzekerdejaren column
  4. The actual parser output (record count)

If the header count assertion fails, you never see whether the decimal parsing works, making diagnosis harder.

Recommended Refactor:
Split into focused, named tests:

[Fact]
public void VektisCsvFile_HasExpectedHeaderStructure()
{
    var csvDir = Path.Combine(_fixture.VektisDataPath, "postcode3");
    var file = Directory.GetFiles(csvDir, "*.csv").First();

    using var reader = new StreamReader(file);
    var headers = reader.ReadLine()!.Split(';')
                        .Select(h => h.Trim().ToLowerInvariant())
                        .ToArray();

    Assert.Equal(31, headers.Length);
    Assert.Equal("geslacht", headers[0]);
    Assert.Equal("aantal_verzekerdejaren", headers...

</details>



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

- Fixes rowantervelde/agentic-development#42

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

📍 Connect Copilot coding agent with [Jira](https://gh.io/cca-jira-docs), [Azure Boards](https://gh.io/cca-azure-boards-docs) or [Linear](https://gh.io/cca-linear-docs) to delegate work to Copilot in one click without leaving your project management tool.

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