test: Moved over to analyser based accessibility enforcement #40
Conversation
Signed-off-by: James Hughes <james@pyrosoftsolutions.co.uk>
…e and then fixed all the warnings that had been silenced erroneously
WalkthroughThis PR removes two architecture test projects (Lazarus.Tests.Architecture and Lazarus.Extensions.HealthChecks.Tests.Architecture) and their associated tests, updates the CI pipeline to skip those test jobs, removes unused imports across multiple files, adds Archon package dependency, and comprehensively restructures the EditorConfig file with updated formatting rules and file-type groupings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.editorconfig.github/workflows/ci-pipeline.ymlsrc/Lazarus.Extensions.HealthChecks.Tests.Architecture/AccessibilityTests.cssrc/Lazarus.Extensions.HealthChecks.Tests.Architecture/Lazarus.Extensions.HealthChecks.Tests.Architecture.csprojsrc/Lazarus.Extensions.HealthChecks.Tests.Architecture/TypeDiscoveryFixture.cssrc/Lazarus.Extensions.HealthChecks.Tests.Integration/Fixture/TestWebApplicationFactory.cssrc/Lazarus.Extensions.HealthChecks.Tests.Unit/LazarusServiceHealthCheckTests.cssrc/Lazarus.Extensions.HealthChecks/Lazarus.Extensions.HealthChecks.csprojsrc/Lazarus.Extensions.HealthChecks/Public/HealthCheckExtensions.cssrc/Lazarus.Tests.Architecture/AccessibilityTests.cssrc/Lazarus.Tests.Architecture/Lazarus.Tests.Architecture.csprojsrc/Lazarus.Tests.Architecture/TypeDiscoveryFixture.cssrc/Lazarus.Tests.Unit/LazarusServiceTests.cssrc/Lazarus.Tests.Unit/ServiceCollectionExtensionsTests.cssrc/Lazarus.slnxsrc/Lazarus/Lazarus.csprojsrc/Lazarus/Public/Configuration/ServiceCollectionExtensions.cs
💤 Files with no reviewable changes (12)
- src/Lazarus.Tests.Unit/ServiceCollectionExtensionsTests.cs
- src/Lazarus.Extensions.HealthChecks.Tests.Integration/Fixture/TestWebApplicationFactory.cs
- .github/workflows/ci-pipeline.yml
- src/Lazarus.Extensions.HealthChecks.Tests.Architecture/Lazarus.Extensions.HealthChecks.Tests.Architecture.csproj
- src/Lazarus.Extensions.HealthChecks.Tests.Architecture/TypeDiscoveryFixture.cs
- src/Lazarus.slnx
- src/Lazarus.Extensions.HealthChecks.Tests.Unit/LazarusServiceHealthCheckTests.cs
- src/Lazarus.Tests.Architecture/TypeDiscoveryFixture.cs
- src/Lazarus.Extensions.HealthChecks/Public/HealthCheckExtensions.cs
- src/Lazarus.Extensions.HealthChecks.Tests.Architecture/AccessibilityTests.cs
- src/Lazarus.Tests.Architecture/Lazarus.Tests.Architecture.csproj
- src/Lazarus.Tests.Architecture/AccessibilityTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/Lazarus/Public/Configuration/ServiceCollectionExtensions.cs (1)
src/Lazarus/Public/Configuration/LazarusConfigurationException.cs (2)
LazarusConfigurationException(10-18)LazarusConfigurationException(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Package Lazarus Pre-release
- GitHub Check: Package HealthChecks Pre-release
🔇 Additional comments (10)
src/Lazarus/Public/Configuration/ServiceCollectionExtensions.cs (1)
15-15: LGTM! Constant naming follows C# conventions.The rename from
DUPE_SERVICE_ERROR_MESSAGEtoDupeServiceErrorMessagecorrectly adopts PascalCase, aligning with C# naming conventions for constants and the updated.editorconfigrules.Also applies to: 54-54
.editorconfig (7)
1-14: LGTM! Well-structured global settings.The simplified header and global settings provide clear defaults. The
max_line_length = 180is generous but acceptable as a project preference.
16-23: LGTM! Consolidated code file settings.The unified glob pattern for
.cs,.csx,.vb, and.vbxfiles with 4-space indentation is clean and follows standard practices.
24-50: LGTM! Well-organised file-type sections.The new XML/Config, JSON/YAML, and Web Files sections are well-structured with appropriate 2-space indentation for these file types.
61-114: Note the severity reductions for .NET style rules.Many rules have been downgraded to
silentorsuggestionseverity (e.g.,this.preferences, parentheses preferences, object initialisers). This more permissive approach will reduce IDE noise but may allow style inconsistencies. Ensure this aligns with your team's preferences.
115-173: LGTM! Modern C# style with strict var policy.The configuration enforces file-scoped namespaces (line 169) and completely disallows
varusage (lines 122-124) with error severity. This ensures explicit type declarations throughout the codebase, which aligns with the project's style preferences.
225-290: Excellent comprehensive naming conventions.The naming rules are well-defined and cover all symbol types appropriately:
- Private fields:
_camelCase(lines 278-283)- Constants:
PascalCase(lines 264-269)- Interfaces:
IPascalCase(lines 245-249)This aligns perfectly with the constant rename in
ServiceCollectionExtensions.cs.
291-329: LGTM! Excellent analyzer diagnostic configuration.The new diagnostics section enforces important code quality rules:
- Nullable reference types as errors (lines 300-306)
- Async without await as error (line 315)
- Unused code as warnings (lines 308-312)
This strengthens code safety and maintainability.
src/Lazarus.Extensions.HealthChecks/Lazarus.Extensions.HealthChecks.csproj (1)
33-36: Configuration is consistent with the main project.The Archon package reference uses the same configuration as in
src/Lazarus/Lazarus.csproj, which ensures consistency across the projects. The verification script provided in the review of the main project file will confirm the package version validity for both projects.src/Lazarus/Lazarus.csproj (1)
34-37: The Archon package reference is valid and correctly configured as a Roslyn analyser. The package exists on NuGet, the configuration withPrivateAssets="all"andIncludeAssetsincluding "analyzers" is correct, and no security advisories have been reported. Version 0.1.3 is available as a newer release, though the package metadata between versions is identical.
| ServiceCollection services = new(); | ||
| services.AddLogging(); | ||
| services.AddSingleton<IWatchdogService<TestService>>(_watchdog); | ||
| services.AddSingleton(_watchdog); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the actual code at the lines mentioned
cat -n src/Lazarus.Tests.Unit/LazarusServiceTests.cs | sed -n '15,35p'Repository: hughesjs/Lazarus
Length of output: 986
🏁 Script executed:
# Search for WatchdogScopeFactory to understand what it does
rg -n "class WatchdogScopeFactory" --type=csRepository: hughesjs/Lazarus
Length of output: 151
🏁 Script executed:
# Search for any GetService or GetRequiredService calls involving IWatchdogService
rg -n -C3 "GetService.*IWatchdogService|GetRequiredService.*IWatchdogService" --type=csRepository: hughesjs/Lazarus
Length of output: 3783
🏁 Script executed:
# Search more broadly for resolution patterns that might involve IWatchdogService
rg -n "serviceProvider\.GetService\|serviceProvider\.GetRequiredService\|services\.BuildServiceProvider" --type=cs -A2 -B2Repository: hughesjs/Lazarus
Length of output: 42
🏁 Script executed:
# Search for WatchdogScopeFactory constructor to see if it uses service resolution
rg -n -A10 "WatchdogScopeFactory\s*\(" --type=csRepository: hughesjs/Lazarus
Length of output: 1138
🏁 Script executed:
# Check if CreateScope is called in LazarusServiceTests
rg -n "CreateScope\|watchdogScopeFactory" src/Lazarus.Tests.Unit/LazarusServiceTests.csRepository: hughesjs/Lazarus
Length of output: 42
🏁 Script executed:
# Check the entire LazarusServiceTests.cs to see all usage of watchdogScopeFactory
cat -n src/Lazarus.Tests.Unit/LazarusServiceTests.cs | head -100Repository: hughesjs/Lazarus
Length of output: 3839
🏁 Script executed:
# Check if there are any test methods that would use the watchdogScopeFactory
rg -n "\[Test\]\|public.*void\|public.*async.*Task" src/Lazarus.Tests.Unit/LazarusServiceTests.csRepository: hughesjs/Lazarus
Length of output: 42
🏁 Script executed:
# Search for where CreateScope is called in LazarusService
rg -n "CreateScope" src/Lazarus/Internal/Service/LazarusService.cs -A3 -B3Repository: hughesjs/Lazarus
Length of output: 636
🏁 Script executed:
# Look at the LazarusService implementation to understand the flow
cat -n src/Lazarus/Internal/Service/LazarusService.cs | head -80Repository: hughesjs/Lazarus
Length of output: 2683
🏁 Script executed:
# Verify if the registration change actually broke the tests
# by checking when services.AddSingleton is called with IWatchdogService interface
rg -n "AddSingleton.*IWatchdogService" --type=csRepository: hughesjs/Lazarus
Length of output: 634
Fix DI registration: the interface type must be registered.
The change to services.AddSingleton(_watchdog) registers only the concrete runtime type InMemoryWatchdogService<TestService>, not the interface IWatchdogService<TestService>. However, WatchdogScopeFactory.CreateScope() calls GetRequiredService<IWatchdogService<TService>>() during each test iteration, which will fail at runtime with InvalidOperationException.
Change line 29 to: services.AddSingleton<IWatchdogService<TestService>>(_watchdog) to match the production code pattern in ServiceCollectionExtensions.cs and other tests.
🤖 Prompt for AI Agents
In src/Lazarus.Tests.Unit/LazarusServiceTests.cs around line 29, the test
currently registers the concrete instance with services.AddSingleton(_watchdog)
which leaves the interface IWatchdogService<TestService> unregistered and will
cause GetRequiredService<IWatchdogService<TService>>() to fail; change the
registration so the service is registered under the interface type (register
IWatchdogService<TestService> with the existing _watchdog instance) to match the
production pattern and other tests.
Summary by CodeRabbit
Release Notes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.