Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Aug 11, 2025

VSCode's C# extension was not showing "play" button controls in the editor gutter for TUnit tests in version 0.5x. This prevented users from running individual tests directly from the editor, even with C# Dev Kit installed and "Use Testing Platform Protocol" enabled.

Root Cause

TUnit uses Microsoft.Testing.Platform instead of the legacy VSTest framework, but was missing specific MSBuild properties that VSCode's C# extension requires to recognize test projects and enable editor gutter integration.

Solution

Added two essential MSBuild properties to enable VSCode test discovery:

<!-- Enable VSCode test discovery in editor gutter -->
<UseTestingPlatformProtocol>true</UseTestingPlatformProtocol>
<IsTestingPlatformEnabled>true</IsTestingPlatformEnabled>

These properties were added to:

  • TUnit.Engine/TUnit.Engine.props - Core engine properties
  • TUnit/TUnit.props - Main TUnit library properties
  • TestProject.props - Template for all test projects

Also added the Microsoft.Testing.Platform.MSBuild package reference to ensure proper MSBuild integration.

What This Fixes

After this change, VSCode users should see:

  • ▶️ Play buttons in the editor gutter next to test methods
  • Ability to run individual tests directly from the editor
  • Full test discovery in the Test Explorer
  • Proper integration with the "Use Testing Platform Protocol" setting

Impact

This is a minimal, non-breaking change that only adds the necessary properties for VSCode compatibility without affecting Visual Studio, Rider, or command-line usage.

Fixes #2856.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Visual Studio Code: the play control in the editor gutter missing in TUnit 0.5x versions Fix VSCode test discovery: Add missing MSBuild properties for editor gutter integration Aug 11, 2025
Copilot AI requested a review from thomhurst August 11, 2025 10:03
@thomhurst thomhurst marked this pull request as ready for review August 11, 2025 10:10
@claude
Copy link
Contributor

claude bot commented Aug 11, 2025

Claude finished @Copilot's task —— View job


PR Review: VSCode Test Discovery Fix

I've completed a thorough review of this PR. Here's my comprehensive analysis:

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze changed files and MSBuild properties
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage
  • Provide final recommendations

Code Quality Assessment

Strong Points

  1. Minimal, Focused Changes: The PR correctly targets only the necessary MSBuild properties for VSCode integration
  2. Consistent Application: Properties are consistently applied across all relevant .props files (TUnit.Engine.props, TUnit.props, TestProject.props)
  3. Proper MSBuild Integration: The addition of Microsoft.Testing.Platform.MSBuild package reference ensures proper build tooling

⚠️ Issues Found

  1. Misleading PR Description: The PR description claims to add EnableMSTestRunner property, but the latest commit actually removes this property along with explanatory comments. The git diff shows:

    -        <!-- Enable VSCode test discovery in editor gutter -->
    -        <EnableMSTestRunner>true</EnableMSTestRunner>
  2. Missing Test Coverage:

    • The PR removed VSCodeIntegrationTest.cs and VSCode_INTEGRATION_README.md files that could have provided validation
    • No automated tests verify that VSCode integration actually works
    • No integration tests confirm the MSBuild properties have the intended effect
  3. Documentation Gap: With the removal of documentation files and comments, there's no guidance for:

    • Users experiencing issues
    • Developers understanding which properties are required
    • Troubleshooting VSCode integration problems

Technical Analysis

MSBuild Properties Review

The remaining properties are technically sound:

  • UseTestingPlatformProtocol=true - ✅ Correct for Microsoft.Testing.Platform
  • IsTestingPlatformEnabled=true - ✅ Required for platform recognition
  • Package reference to Microsoft.Testing.Platform.MSBuild - ✅ Proper integration

Performance Considerations

  • ✅ No performance impact - these are build-time properties only
  • ✅ No runtime overhead introduced

Security Assessment

  • ✅ No security concerns - standard MSBuild properties
  • ✅ No sensitive data exposure

Recommendations

High Priority

  1. Clarify PR Description: Update to reflect what actually changed (removal of EnableMSTestRunner, not addition)
  2. Add Validation Tests: Create simple tests to verify VSCode integration works
  3. Restore Documentation: Add minimal documentation explaining the properties and troubleshooting steps

Medium Priority

  1. Version Testing: Verify this works across VSCode versions and C# extension versions
  2. Cross-IDE Compatibility: Confirm these changes don't negatively impact Visual Studio or Rider

Low Priority

  1. Consider Feature Flag: For such IDE-specific properties, consider making them configurable

Final Assessment

The core technical changes are sound and appropriate for VSCode integration. However, there are documentation and testing gaps that reduce confidence in the implementation.

Recommendation: The PR is technically acceptable but should be enhanced with proper testing and documentation before merge.


Tests with circular dependencies were causing indefinite hangs when scheduled
for execution, as each test would wait for its dependencies that could never
complete.

This fix adds proactive circular dependency detection in ExecutionPlan.Create:
- Uses depth-first search with visit state tracking to detect cycles
- Marks all tests in a cycle as Failed with descriptive error message
- Completes their TaskCompletionSource to prevent waiting
- Excludes circular tests from execution scheduling and topological sort
- Reports all pre-failed tests through message bus for proper test result output

Tests like ConflictingDependsOnTests that have circular dependencies
(Test1 -> Test2 -> Test1) now fail immediately with clear error messages
instead of hanging indefinitely.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@claude
Copy link
Contributor

claude bot commented Aug 11, 2025

Claude finished @Copilot's task —— View job


PR Review: VSCode Test Discovery Fix

I've completed a comprehensive review of this PR. Here's my detailed analysis:

Review Checklist:

  • Read repository guidelines (CLAUDE.md) - No repo guidelines found
  • Analyze MSBuild properties changes
  • Review scheduling logic changes in ExecutionPlan.cs
  • Review TestScheduler.cs changes
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage
  • Provide final recommendations

🚨 Critical Issue: Scope Creep

This PR has significantly evolved beyond its stated purpose. The title indicates "VSCode test discovery fixes" but it now includes a major refactoring of the test scheduling system.

What Was Expected (VSCode Integration):

  • Adding MSBuild properties for VSCode C# extension compatibility
  • Simple, low-risk changes to .props files

What Actually Changed:

  1. MSBuild Properties (Expected): Added VSCode integration properties
  2. ⚠️ Major Scheduling Refactor (Unexpected): Added 87+ lines of circular dependency detection logic

Technical Analysis

MSBuild Properties Changes (Good)

Files: TUnit.Engine.props, TUnit.props, TestProject.props

  • ✅ Correctly added UseTestingPlatformProtocol=true
  • ✅ Correctly added IsTestingPlatformEnabled=true
  • ✅ Added Microsoft.Testing.Platform.MSBuild package reference
  • ✅ These changes align with Microsoft.Testing.Platform requirements for VSCode

⚠️ Scheduling System Changes (Concerning)

Files: ExecutionPlan.cs (+101/-4), TestScheduler.cs (+9/-0)

Algorithm Quality: The circular dependency detection is technically sound:

  • Uses DFS with visit states (Visiting/Visited) to detect cycles
  • Proper topological sorting implementation
  • Correctly identifies all tests in circular dependency chains

Issues Found:

  1. 🔴 Brittle Internal Access (ExecutionPlan.cs:85):

    test.Context.InternalExecutableTest._taskCompletionSource?.TrySetResult();

    Direct access to internal _taskCompletionSource field is fragile and could break with refactoring.

  2. 🟡 Performance Impact:

    • Cycle detection runs on every test execution plan creation
    • Additional memory overhead from multiple dictionaries and collections
    • O(V + E) complexity for each test run
  3. 🟡 Missing Edge Cases:

    • No handling of self-dependencies (test depending on itself)
    • No validation of dependency resolution timing
  4. 🔴 Missing Unit Tests:

    • No tests for the complex circular dependency detection algorithm
    • Existing test file (CircularDependencyTestVerification.cs) only provides integration examples
    • Critical algorithm has no unit test coverage

Security Assessment

  • ✅ No security concerns identified
  • ✅ Standard MSBuild properties pose no security risks
  • ✅ No sensitive data exposure in the changes

Performance Considerations

MSBuild Properties: ✅ No runtime performance impact (build-time only)

Scheduling Changes: ⚠️ Potential Impact

  • Memory: Additional data structures created for each test run
  • CPU: O(V + E) cycle detection algorithm runs for every execution plan
  • Scalability: Could impact large test suites with complex dependency graphs

Test Coverage Assessment

Major Gap: The circular dependency detection algorithm has no dedicated unit tests.

Existing Coverage:

  • Integration test exists (CircularDependencyTestVerification.cs) with examples
  • No isolated unit tests for algorithm correctness
  • No edge case testing (empty graphs, self-dependencies, etc.)

Recommendations

🔴 High Priority (Must Fix)

  1. Split the PR: Separate VSCode integration from scheduling refactor
  2. Add Unit Tests: Create comprehensive tests for circular dependency detection
  3. Fix Internal Access: Replace direct _taskCompletionSource access with public API
  4. Update PR Description: Accurately describe the actual changes made

🟡 Medium Priority (Should Consider)

  1. Performance Testing: Measure impact on large test suites
  2. Edge Case Handling: Add validation for self-dependencies
  3. Documentation: Add inline documentation for the cycle detection algorithm

🟢 Low Priority (Nice to Have)

  1. Caching: Consider caching dependency graphs for repeated runs
  2. Metrics: Add timing metrics for cycle detection performance

Final Verdict

MSBuild Changes: ✅ Ready for merge - These correctly implement VSCode integration

Scheduling Changes: ⚠️ Needs work - While technically sound, lacks proper testing and has implementation concerns

Overall Recommendation: This PR should be split into two separate PRs to allow the VSCode integration to proceed while the scheduling refactor receives proper testing and review.


@thomhurst
Copy link
Owner

@copilot undo the code changes and just keep the .props file changes. All update the title/description to remove EnableMSTestRunner

Copy link
Contributor Author

Copilot AI commented Aug 11, 2025

@copilot undo the code changes and just keep the .props file changes. All update the title/description to remove EnableMSTestRunner

Done! Reverted all code changes and kept only the .props file changes. Also removed EnableMSTestRunner references from the title/description. The solution now only adds the two essential MSBuild properties (UseTestingPlatformProtocol and IsTestingPlatformEnabled) plus the Microsoft.Testing.Platform.MSBuild package reference for VSCode test discovery integration. (363fc8f)

@thomhurst thomhurst merged commit 9bd5640 into main Aug 11, 2025
8 of 9 checks passed
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.

Visual Studio Code: the play control in the editor gutter missing in TUnit 0.5x versions

2 participants