Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #3985

Copy link

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 adds JUnit XML report generation capability to TUnit, addressing issue #3985. The implementation enables automatic JUnit XML output when running in GitLab CI or when explicitly enabled via environment variables, making TUnit compatible with CI/CD systems that consume JUnit XML format.

Key Changes

  • Adds JUnitXmlWriter for generating JUnit-compliant XML from test results with comprehensive test state handling
  • Implements JUnitReporter with environment variable-based activation and automatic GitLab CI detection
  • Provides command-line option --junit-output-path for custom output location configuration

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
TUnit.Engine/Xml/JUnitXmlWriter.cs New XML generator that converts TUnit test results to JUnit XML format with support for all test states (passed, failed, skipped, error, timeout, cancelled, in-progress)
TUnit.Engine/Reporters/JUnitReporter.cs New reporter implementing data consumer and lifecycle interfaces, with environment-based activation logic and file writing with retry mechanism
TUnit.Engine/CommandLineProviders/JUnitReporterCommandProvider.cs Command provider for --junit-output-path option with validation
TUnit.Engine/Extensions/TestApplicationBuilderExtensions.cs Registration of JUnit reporter and command provider in the test application builder
TUnit.Engine.Tests/JUnitReporterTests.cs Unit tests verifying the environment variable-based activation logic for the JUnit reporter
Comments suppressed due to low confidence (1)

TUnit.Engine.Tests/JUnitReporterTests.cs:154

  • The tests only cover the IsEnabledAsync() method but don't test the actual XML generation and file writing functionality. Consider adding tests that:
  1. Verify the XML output format with various test states (passed, failed, skipped, error, timeout, cancelled)
  2. Test the file writing with retry logic
  3. Verify the filter parameter is correctly passed through to the XML generation
  4. Test the default output path generation
  5. Test the custom output path via SetOutputPath()

This is especially important since JUnitXmlWriter has no dedicated test file.

using Microsoft.Testing.Platform.Extensions;
using TUnit.Core;
using TUnit.Engine.Reporters;

namespace TUnit.Engine.Tests;

[NotInParallel]
public class JUnitReporterTests
{
    private sealed class MockExtension : IExtension
    {
        public string Uid => "MockExtension";
        public string DisplayName => "Mock";
        public string Version => "1.0.0";
        public string Description => "Mock Extension";
        public Task<bool> IsEnabledAsync() => Task.FromResult(true);
    }

    [After(Test)]
    public void Cleanup()
    {
        // Clean up environment variables after each test
        Environment.SetEnvironmentVariable("TUNIT_DISABLE_JUNIT_REPORTER", null);
        Environment.SetEnvironmentVariable("TUNIT_ENABLE_JUNIT_REPORTER", null);
        Environment.SetEnvironmentVariable("GITLAB_CI", null);
        Environment.SetEnvironmentVariable("CI_SERVER", null);
        Environment.SetEnvironmentVariable("JUNIT_XML_OUTPUT_PATH", null);
    }

    [Test]
    public async Task IsEnabledAsync_Should_Return_False_When_TUNIT_DISABLE_JUNIT_REPORTER_Is_Set()
    {
        // Arrange
        Environment.SetEnvironmentVariable("TUNIT_DISABLE_JUNIT_REPORTER", "true");
        Environment.SetEnvironmentVariable("GITLAB_CI", "true"); // Even with GitLab CI, should be disabled
        var extension = new MockExtension();
        var reporter = new JUnitReporter(extension);

        // Act
        var isEnabled = await reporter.IsEnabledAsync();

        // Assert
        await Assert.That(isEnabled).IsFalse();
    }

    [Test]
    public async Task IsEnabledAsync_Should_Return_True_When_GITLAB_CI_Is_Set()
    {
        // Arrange
        Environment.SetEnvironmentVariable("GITLAB_CI", "true");
        var extension = new MockExtension();
        var reporter = new JUnitReporter(extension);

        // Act
        var isEnabled = await reporter.IsEnabledAsync();

        // Assert
        await Assert.That(isEnabled).IsTrue();
    }

    [Test]
    public async Task IsEnabledAsync_Should_Return_True_When_CI_SERVER_Is_Set()
    {
        // Arrange
        Environment.SetEnvironmentVariable("CI_SERVER", "yes");
        var extension = new MockExtension();
        var reporter = new JUnitReporter(extension);

        // Act
        var isEnabled = await reporter.IsEnabledAsync();

        // Assert
        await Assert.That(isEnabled).IsTrue();
    }

    [Test]
    public async Task IsEnabledAsync_Should_Return_True_When_TUNIT_ENABLE_JUNIT_REPORTER_Is_Set()
    {
        // Arrange
        Environment.SetEnvironmentVariable("TUNIT_ENABLE_JUNIT_REPORTER", "true");
        var extension = new MockExtension();
        var reporter = new JUnitReporter(extension);

        // Act
        var isEnabled = await reporter.IsEnabledAsync();

        // Assert
        await Assert.That(isEnabled).IsTrue();
    }

    [Test]
    public async Task IsEnabledAsync_Should_Return_False_When_No_Environment_Variables_Are_Set()
    {
        // Arrange
        var extension = new MockExtension();
        var reporter = new JUnitReporter(extension);

        // Act
        var isEnabled = await reporter.IsEnabledAsync();

        // Assert
        await Assert.That(isEnabled).IsFalse();
    }

    [Test]
    public async Task IsEnabledAsync_Should_Prefer_Disable_Over_Enable()
    {
        // Arrange
        Environment.SetEnvironmentVariable("TUNIT_DISABLE_JUNIT_REPORTER", "true");
        Environment.SetEnvironmentVariable("TUNIT_ENABLE_JUNIT_REPORTER", "true");
        var extension = new MockExtension();
        var reporter = new JUnitReporter(extension);

        // Act
        var isEnabled = await reporter.IsEnabledAsync();

        // Assert
        await Assert.That(isEnabled).IsFalse();
    }
}

public string? Filter { get; set; }

internal void SetOutputPath(string path)
{
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The SetOutputPath method is marked as internal and is only used for testing and command-line configuration. Consider adding validation to ensure the path is not null or empty, similar to how the method is called from TestApplicationBuilderExtensions. This would make the method more robust if called from other contexts.

Suggested change
{
{
if (string.IsNullOrEmpty(path))
{
throw new ArgumentException("Output path must not be null or empty.", nameof(path));
}

Copilot uses AI. Check for mistakes.
Comment on lines +77 to +84
writer.WriteStartElement("testsuite");
writer.WriteAttributeString("name", assemblyName);
writer.WriteAttributeString("tests", summary.Total.ToString(CultureInfo.InvariantCulture));
writer.WriteAttributeString("failures", summary.Failures.ToString(CultureInfo.InvariantCulture));
writer.WriteAttributeString("errors", summary.Errors.ToString(CultureInfo.InvariantCulture));
writer.WriteAttributeString("skipped", summary.Skipped.ToString(CultureInfo.InvariantCulture));
writer.WriteAttributeString("time", summary.TotalTime.TotalSeconds.ToString("F3", CultureInfo.InvariantCulture));
writer.WriteAttributeString("timestamp", summary.Timestamp.ToString("o", CultureInfo.InvariantCulture));
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The attributes written here (lines 77-84) are duplicates of the attributes written in the testsuites element (lines 51-58). This creates redundancy in the XML output. Consider whether both levels need the same attributes, or if some should be unique to each level according to the JUnit XML schema.

Copilot uses AI. Check for mistakes.
Comment on lines 21 to 38
if (EnvironmentVariableCache.Get("TUNIT_DISABLE_JUNIT_REPORTER") is not null)
{
return false;
}

// Check if explicitly enabled OR running in GitLab CI
var explicitlyEnabled = EnvironmentVariableCache.Get("TUNIT_ENABLE_JUNIT_REPORTER") is not null;
var runningInGitLab = EnvironmentVariableCache.Get("GITLAB_CI") is not null ||
EnvironmentVariableCache.Get("CI_SERVER") is not null;

if (!explicitlyEnabled && !runningInGitLab)
{
return false;
}

// Determine output path
_outputPath = EnvironmentVariableCache.Get("JUNIT_XML_OUTPUT_PATH")
?? GetDefaultOutputPath();
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The new JUnit reporter environment variables are not registered in EnvironmentVariableCache._tunitEnvironmentVariables array. The following variables need to be added:

  • TUNIT_DISABLE_JUNIT_REPORTER
  • DISABLE_JUNIT_REPORTER
  • TUNIT_ENABLE_JUNIT_REPORTER
  • CI_SERVER
  • JUNIT_XML_OUTPUT_PATH

This is important for performance as the cache avoids repeated system calls. Without these entries, calls to EnvironmentVariableCache.Get() for these variables will return null even when they are set.

Copilot uses AI. Check for mistakes.

// Get test state
var stateProperty = testNode.Properties.AsEnumerable()
.FirstOrDefault(p => p is TestNodeStateProperty);
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

This code searches for TestNodeStateProperty by iterating through all properties and checking type. Consider using OfType<TestNodeStateProperty>().FirstOrDefault() for consistency with how TimingProperty and TestMethodIdentifierProperty are retrieved on lines 138-147, which would be more idiomatic and potentially more efficient.

Suggested change
.FirstOrDefault(p => p is TestNodeStateProperty);
.OfType<TestNodeStateProperty>()
.FirstOrDefault();

Copilot uses AI. Check for mistakes.
summary.Total++;

var stateProperty = test.TestNode.Properties.AsEnumerable()
.FirstOrDefault(p => p is TestNodeStateProperty);
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

This code searches for TestNodeStateProperty by iterating through all properties and checking type. Consider using OfType<TestNodeStateProperty>().FirstOrDefault() for consistency with how TimingProperty is retrieved on lines 301-303, which would be more idiomatic and potentially more efficient.

Suggested change
.FirstOrDefault(p => p is TestNodeStateProperty);
.OfType<TestNodeStateProperty>()
.FirstOrDefault();

Copilot uses AI. Check for mistakes.
Comment on lines 37 to 40
if (commandOption.Name == JUnitOutputPathOption && arguments.Length != 1)
{
return ValidationResult.InvalidTask("A single output path must be provided for --junit-output-path");
}
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The validation check arguments.Length != 1 is redundant because the command option is already configured with ArgumentArity.ExactlyOne on line 28, which ensures exactly one argument is provided by the framework. This check will never be triggered.

Suggested change
if (commandOption.Name == JUnitOutputPathOption && arguments.Length != 1)
{
return ValidationResult.InvalidTask("A single output path must be provided for --junit-output-path");
}

Copilot uses AI. Check for mistakes.
{
var summary = new TestSummary
{
Timestamp = DateTimeOffset.Now
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

Using DateTimeOffset.Now for the timestamp means it captures the time when the XML is generated, not when the tests actually ran. Consider capturing the timestamp when tests start (e.g., from the first test's start time) or when results are being collected to provide more accurate test execution timing information.

Copilot uses AI. Check for mistakes.

const int maxAttempts = 5;
var random = new Random();

Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

Creating a new Random instance on each call can lead to predictable sequences if called in quick succession. Consider using Random.Shared (available in .NET 6+) for better randomness, or create a single static Random instance if targeting older frameworks. For async retry scenarios, Random.Shared is the modern best practice.

Copilot uses AI. Check for mistakes.
Comment on lines +19 to +29
[After(Test)]
public void Cleanup()
{
// Clean up environment variables after each test
Environment.SetEnvironmentVariable("TUNIT_DISABLE_JUNIT_REPORTER", null);
Environment.SetEnvironmentVariable("TUNIT_ENABLE_JUNIT_REPORTER", null);
Environment.SetEnvironmentVariable("GITLAB_CI", null);
Environment.SetEnvironmentVariable("CI_SERVER", null);
Environment.SetEnvironmentVariable("JUNIT_XML_OUTPUT_PATH", null);
}

Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The tests will not work as expected because they set environment variables directly, but the JUnitReporter uses EnvironmentVariableCache.Get() which caches environment variables on first access. Once the cache is initialized, setting environment variables with Environment.SetEnvironmentVariable() won't affect the cached values.

To fix this, you need to either:

  1. Add a method to clear/invalidate the EnvironmentVariableCache and call it in the Cleanup() method
  2. Use reflection to directly call the cache initialization or clear it
  3. Refactor tests to use dependency injection for environment variable access

Copilot uses AI. Check for mistakes.
Comment on lines 80 to 86
{
if (kvp.Value.Count > 0)
{
lastUpdates.Add(kvp.Value[kvp.Value.Count - 1]);
}
}

Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

Copilot uses AI. Check for mistakes.
@thomhurst thomhurst merged commit dd65654 into main Dec 5, 2025
10 of 13 checks passed
@thomhurst thomhurst deleted the feature/3985 branch December 5, 2025 19:55
@claude claude bot mentioned this pull request Dec 6, 2025
1 task
This was referenced Dec 12, 2025
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.

CI/CD reporting for GitLab (junit)

2 participants