Skip to content

Conversation

@natemcmaster
Copy link
Owner

Summary

  • Added [Collection("MetadataRegistry")] attribute to test classes that use CommandMetadataRegistry
  • Added explanatory comments for future maintainers

Problem

Tests using CommandMetadataRegistry share static state and were failing intermittently when xUnit ran them in parallel with other test classes. One test would call Clear() while another was mid-execution.

Solution

All test classes that interact with the registry now use the same xUnit collection with DisableParallelization = true, ensuring they run sequentially.

Affected test classes

  • CommandMetadataRegistryTests
  • ConventionAotPathTests
  • MetadataProviderTests
  • RemainingArgsPropertyConventionTests

Test plan

  • Ran full build (./build.ps1)
  • Ran affected tests 20 times in a loop with 0 failures

🤖 Generated with Claude Code

… tests

Tests using CommandMetadataRegistry share static state and were failing
intermittently when run in parallel. Added [Collection("MetadataRegistry")]
attribute to all test classes that interact with the registry to ensure
they run sequentially.

Affected test classes:
- CommandMetadataRegistryTests
- ConventionAotPathTests
- MetadataProviderTests
- RemainingArgsPropertyConventionTests

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@natemcmaster natemcmaster merged commit 190ac01 into main Jan 12, 2026
8 checks passed
@natemcmaster natemcmaster deleted the fix/flaky-registry-tests branch January 12, 2026 02:28
@codecov
Copy link

codecov bot commented Jan 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.93%. Comparing base (94acbc8) to head (4166fbe).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #584   +/-   ##
=======================================
  Coverage   79.93%   79.93%           
=======================================
  Files         120      120           
  Lines        4017     4017           
  Branches      872      872           
=======================================
  Hits         3211     3211           
  Misses        618      618           
  Partials      188      188           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@claude
Copy link

claude bot commented Jan 12, 2026

Code Review - PR 584

This PR effectively addresses flaky test failures caused by parallel execution of tests sharing the static CommandMetadataRegistry. The approach is sound and follows xUnit best practices.

Strengths:

  • Root cause correctly identified: Tests were racing on shared static state
  • Appropriate solution: Using xUnit collections with DisableParallelization = true is the standard pattern
  • Complete coverage: All 5 test classes using CommandMetadataRegistry are now properly handled (4 updated in this PR, 1 already had the attribute)
  • Helpful comments added to each test class for future maintainers
  • Collection definition exists at DefaultMetadataResolverTests.cs:14-15
  • Well-tested: Full build passed and stress-tested 20 iterations with 0 failures

Areas for Improvement:

  1. Missing cleanup in some test classes - MetadataProviderTests.cs and RemainingArgsPropertyConventionTests.cs lack constructor/dispose cleanup that other test classes have. Consider adding constructors with CommandMetadataRegistry.Clear() for consistent state isolation.

  2. Consider adding IDisposable to all registry test classes for symmetry, matching the pattern in ConventionAotPathTests and DefaultMetadataResolverTests.

Verdict: Approve with minor suggestions. The PR solves the immediate flakiness problem effectively. The cleanup suggestions are optional improvements - current implementation is acceptable given sequential execution.

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