test(metrics): eliminate coverage theater with exact value assertions#147
test(metrics): eliminate coverage theater with exact value assertions#147
Conversation
- Extract MetricCollectionHarness to shared file (remove 2 duplicate copies) - Create OTelTestHelper for InMemoryExporter-based unit tests - Strengthen MultipleNamedCaches: assert exact per-cache hit/miss counts - Strengthen WithCacheName: assert ALL measurements have correct tag - Strengthen EstimatedSize: assert exact value (42) not just non-empty - Strengthen EvictionAfterDisposal: assert counter NOT incremented post-dispose - Strengthen MeterFactory: verify metrics emitted through factory meter - Strengthen AdditionalTags: assert tag contents (whitespace excluded, duplicates excluded) - Add OpenTelemetry.Exporter.InMemory to Unit.csproj - Net -417 lines (deduplication + stronger assertions)
Summary of ChangesHello @rjmurillo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the reliability and accuracy of metric-related unit tests by moving from "coverage theater" to exact value assertions. It refactors the test infrastructure by centralizing metric collection utilities and introducing OpenTelemetry-specific test helpers, leading to more robust validation of metric emissions and a cleaner codebase. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the quality and reliability of the metric tests. Replacing "coverage theater" tests with assertions for exact values is a fantastic step towards ensuring correctness. The refactoring to deduplicate MetricCollectionHarness and introduce OTelTestHelper also greatly improves the test infrastructure's maintainability.
I've found a few areas in the new helper classes where minor improvements can be made for efficiency and correctness. My comments are focused on these new shared components.
There was a problem hiding this comment.
Pull request overview
This PR significantly improves test quality by replacing weak "coverage theater" assertions with exact value assertions. Coverage theater refers to tests that verify something was recorded but not that the right thing was recorded - for example, checking that metrics are non-empty rather than verifying exact hit/miss counts. The PR also eliminates code duplication by extracting MetricCollectionHarness to a shared location.
Changes:
- Strengthened 8 metric tests with exact value assertions instead of existence checks
- Extracted duplicated
MetricCollectionHarnessclass totests/Unit/Shared/ - Added
OTelTestHelper.cswith OpenTelemetry InMemoryExporter utilities (unused in this PR) - Added OpenTelemetry NuGet packages to support future metric testing patterns
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Unit.csproj | Added OpenTelemetry packages for InMemoryExporter-based testing |
| tests/Unit/Shared/OTelTestHelper.cs | New helper class for OpenTelemetry metric assertions (not used yet) |
| tests/Unit/Shared/MetricCollectionHarness.cs | Extracted from duplicated implementations, now shared |
| tests/Unit/Shared/MeteredCacheTestBase.cs | Removed duplicated harness, strengthened assertions in WithCacheName and MultipleNamedCaches tests |
| tests/Unit/MetricEmissionAccuracyTests.cs | Removed duplicated harness, now uses shared version |
| tests/Unit/CoverageGapTests.cs | Strengthened assertions in 4 tests: EstimatedSize (exact value), EvictionAfterDisposal (counter not incremented), AdditionalTags tests (verify tag contents) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Delete dead OTelTestHelper.cs (zero callers, YAGNI) - Remove 3 unused OTel packages from Unit.csproj - Fix double-lookup in MetricCollectionHarness (TryGetValue) - Fix unnecessary allocation in GetMeasurements (Array.Empty) - Make Collect() atomic (lock spans clear+record) - Replace Task.Yield() with Task.Delay(10, CancellationToken.None) - Remove duplicate assertions in 3 test methods - Add using to all MeteredMemoryCache instances (12 total) - Fix typo in XML doc comment All 226 tests pass (208 unit + 18 integration).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
- Delete unused OTelTestHelper.cs class (dead code with no callers)
- Remove unnecessary OpenTelemetry NuGet packages that supported it
- Fix MeteredMemoryCache_EvictionCallback_AfterDisposal_NoMetricUpdate test:
- Use CancellationChangeToken to trigger TokenExpired eviction
- TokenExpired IS counted by _evictionCount, unlike Remove
- This properly tests the disposal guard (the original test using
inner.Remove triggered EvictionReason.Removed which is excluded
from the counter regardless of disposal guard)
This comment has been minimized.
This comment has been minimized.
Fix vacuous assertion that would pass when no measurements exist. The Assert.All call passes trivially on an empty collection, allowing a broken implementation emitting zero 'cache.requests' measurements to pass the test. Added Assert.NotEmpty(measurements) guard before Assert.All to ensure the test fails if no measurements are emitted, consistent with the pattern used in CoverageGapTests.cs for AdditionalTags tests.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
- Eliminate coverage theater: replace Assert.Contains(true,...) with AssertAllMeasurementsHaveTags in 3 shared test methods - Fix double-lookup in GetMeasurementsWithTags (TryGetValue pattern) - Remove DRY violation: AssertMeasurementCount delegates to AssertAggregatedCount - Add AggregateException catch in Collect() for post-disposal safety - Improve comment accuracy for type-mismatch and eviction scenarios 226 tests pass (208 unit + 18 integration), 0 warnings.
…teredMemoryCacheTests Boy Scout Rule: 6 instances of Assert.Contains(true, ...Select(m => m.Tags.Any(...))) verified 'any measurement has tag' instead of 'ALL measurements have tag'. Added AssertAllMeasurementsHaveTag to TestListener with NotEmpty guard. Replaced all 6 instances across 4 tests: - WithCacheName_EmitsCacheNameTag - OptionsConstructor_WithAdditionalTags_EmitsAllTagsOnMetrics - TryGetValue_Generic_WithNamedCache_RecordsMetricsWithCacheName - GetOrCreate_WithNamedCache_RecordsMetricsWithCacheName Zero Assert.Contains(true,...) patterns remain in the test suite.
| private readonly List<MetricMeasurement> _measurements = new(); | ||
| private readonly Dictionary<string, List<MetricMeasurement>> _measurementsByInstrument = new(); | ||
| private readonly Dictionary<string, long> _aggregatedCounters = new(); | ||
| private readonly string[] _instrumentNames; |
There was a problem hiding this comment.
Unused _instrumentNames field in shared harness
Low Severity
The _instrumentNames field is assigned at construction but never read. The lambda on line 83 captures the constructor parameter instrumentNames directly instead of the field _instrumentNames. This is inconsistent with the TestListener class in MeteredMemoryCacheTests.cs, which correctly references _instrumentNames in its lambda. The behavior is identical today since both reference the same array, but the dead field is confusing.
Additional Locations (2)
2 participants
Summary
Replaces coverage theater tests (tests that verify something was recorded, not that the right thing was recorded) with exact value assertions. A broken implementation that counts hits as misses, leaks tags between caches, or increments on Set instead of TryGetValue would have passed the previous test suite.
Changes
Strengthened Assertions (8 tests)
Assert.NotEmptyAssert.Contains(true, ...)Assert.NotEmptyInfrastructure
MetricCollectionHarnesstoTests/Unit/Shared/MetricCollectionHarness.cs(was duplicated in 2 files)OTelTestHelper.csfor InMemoryExporter-based unit test assertionsImpact