Skip to content

Conversation

@thomhurst
Copy link
Owner

Summary

  • Adds [NotDiscoverable] attribute that hides tests from test explorer/discovery while still allowing them to execute when run via filters
  • Useful for infrastructure tests, internal helpers, or tests that should only run as dependencies

Changes

  • TUnit.Core: Added IsNotDiscoverable property to TestContext and ITestExecution interface
  • TUnit.Core: Created NotDiscoverableAttribute with optional reason and conditional ShouldHide() override
  • TUnit.Engine: Modified TUnitMessageBus.Discovered() to skip publishing for hidden tests
  • Tests: Added integration tests for method-level, class-level, and conditional hiding

Usage

// Simple usage
[Test]
[NotDiscoverable]
public void InfrastructureTest() { }

// With reason
[Test]
[NotDiscoverable("Internal helper test")]
public void HelperTest() { }

// Conditional (via inheritance)
public class HideOnCIAttribute : NotDiscoverableAttribute
{
    public override Task<bool> ShouldHide(TestRegisteredContext ctx) 
        => Task.FromResult(Environment.GetEnvironmentVariable("CI") == "true");
}

Test plan

  • Integration tests pass for method-level [NotDiscoverable]
  • Integration tests pass for class-level [NotDiscoverable]
  • Conditional attribute extensibility verified
  • Public API snapshots updated
  • Full test suite verification

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings December 23, 2025 23:31
@claude
Copy link
Contributor

claude bot commented Dec 23, 2025

Pull Request Review: NotDiscoverableAttribute Implementation

Overview

This PR adds a [NotDiscoverable] attribute to hide tests from test discovery while still allowing execution via filters. The implementation follows TUnit's existing patterns (similar to SkipAttribute) and is well-structured.


Strengths

  1. Consistent API Design: The implementation mirrors SkipAttribute perfectly - same pattern with ShouldHide() method, same order (int.MinValue), and same extensibility model.

  2. Good Documentation: Excellent XML documentation with clear examples showing simple usage, with reason, and conditional hiding via inheritance.

  3. Clean Implementation: The check in TUnitMessageBus.Discovered() (lines 26-29) is simple and effective - short-circuits discovery notification when the flag is set.

  4. Proper Snapshots: Public API snapshots are correctly updated for all target frameworks (DotNet8_0, DotNet9_0, DotNet10_0, Net4_7).

  5. Test Coverage: Good test cases covering method-level, class-level, and conditional attribute scenarios.

  6. Extensibility: The virtual ShouldHide() method enables conditional hiding patterns, which is powerful for CI/environment-specific scenarios.


Observations & Considerations

1. Dual-Mode Implementation

According to CLAUDE.md Rule 1, features must work in both source-generated and reflection modes.

Analysis: This feature is correctly implemented:

  • The attribute uses ITestRegisteredEventReceiver which fires during test registration
  • Both source-gen and reflection paths call the same event receivers in TestBuilder.cs
  • The IsNotDiscoverable property is set on TestContext which is shared across both modes
  • The message bus check happens after test registration, so both modes are affected

Verdict: No additional source generator changes needed - the implementation is inherently dual-mode compatible.

2. Reason Property Inconsistency

Issue: NotDiscoverableAttribute.Reason is nullable and optional, but SkipAttribute.Reason is required.

Question: Is the Reason property actually used anywhere? If for documentation only, the nullable optional approach makes sense. If consumed by tooling/reporting, consistency with SkipAttribute might be better.

Current behavior: The reason is stored but never logged or reported (unlike Skip which shows in test results).

3. Test Execution Behavior

The PR description states: "hides tests from test explorer/discovery while still allowing them to execute when run via filters"

Question: Are there tests verifying that NotDiscoverable tests CAN still execute when targeted by filter? The current tests don't appear to verify execution behavior - they just exist as integration tests.

4. Performance

  • The IsNotDiscoverable check is a simple boolean field access (O(1))
  • No allocations in the hot path
  • Order = int.MinValue ensures early execution (before other receivers)
  • Uses ValueTask appropriately

Verdict: No performance concerns.


Testing Gaps

Missing Test Coverage:

  1. Assembly-level attribute: AttributeUsage allows AttributeTargets.Assembly but no test for this
  2. Execution verification: No test confirms NotDiscoverable tests can execute when targeted
  3. Inheritance chain: No test for class inheritance scenarios

Recommendations

Required:

  1. Complete Test Coverage: Check the "Full test suite verification" checkbox and ensure all tests pass

Suggested:

  1. Add Execution Test: Verify NotDiscoverable tests execute when targeted by filter
  2. Assembly-Level Test: Add test for assembly-level [NotDiscoverable]
  3. Documentation: Consider adding a note explaining that Reason is for documentation only

Consider:

  1. Reason Usage: Decide if Reason should be logged anywhere
  2. Discovery Logging: Consider adding a log message when tests are hidden (for debugging)

Final Verdict

Status: Approved with minor suggestions

This is a well-implemented feature that follows TUnit's conventions and patterns. The implementation is clean, performant, and properly integrated with the existing event system.

Code Quality: 5/5
Test Coverage: 4/5
Documentation: 5/5
Performance: 5/5

Great work!

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 introduces a [NotDiscoverable] attribute that allows tests to be hidden from test discovery/exploration while remaining executable when invoked directly or via filters. This is useful for infrastructure tests, internal test helpers, or tests that should only run as dependencies.

Key Changes:

  • Added NotDiscoverableAttribute with optional reason parameter and extensible conditional hiding via ShouldHide() override
  • Modified test discovery pipeline to skip publishing hidden tests to test explorers
  • Extended ITestExecution interface with IsNotDiscoverable property for runtime test visibility control

Reviewed changes

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

Show a summary per file
File Description
TUnit.Core/Attributes/TestMetadata/NotDiscoverableAttribute.cs Implements the new attribute with support for conditional hiding and documentation
TUnit.Core/Interfaces/ITestExecution.cs Adds IsNotDiscoverable property to the test execution interface
TUnit.Core/TestContext.Execution.cs Implements the IsNotDiscoverable property in TestContext
TUnit.Engine/TUnitMessageBus.cs Modifies discovery logic to skip publishing tests marked as not discoverable
TUnit.TestProject/NotDiscoverableTests.cs Adds integration tests demonstrating method-level, class-level, and conditional usage
TUnit.PublicAPI/*.verified.txt Updates public API snapshots across .NET 4.7, 8.0, 9.0, and 10.0 to reflect new API surface

/// {
/// public NotDiscoverableOnCIAttribute() : base("Hidden on CI") { }
///
/// public override Task&lt;bool&gt; ShouldHide(TestRegisteredContext context)
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

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

HTML entity '<' should be '<' in the code documentation example.

Suggested change
/// public override Task&lt;bool&gt; ShouldHide(TestRegisteredContext context)
/// public override Task<bool> ShouldHide(TestRegisteredContext context)

Copilot uses AI. Check for mistakes.
This was referenced Dec 24, 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.

2 participants