chore(quality): clear Sonar code smells and raise coverage to ~95.8% - #169
Merged
Conversation
Resolve the 4 open SonarQube code smells and lift coverage from 89.8% to ~95.8% (line 91.8%->97.4%, branch 84.1%->91.6%). Smell fixes (all in Lib.EntityFramework): - S1192: extract repeated "decimal" literal to a const (SqlColumnTypeMapper) - S3776: split FluentSyntax.ResolveOwningEntity (cognitive complexity 22) into ResolveFromReceiverChain / ResolveFromAncestors / ComposeOwnedKey - S107: fold GetOrCreateOwned's 8 params into an OwnedTarget record (FluentOwnedTypeWalker) - S107: fold ResolveOwnedNavigationTypesAsync's 8 params into an OwnedTypeResolutionContext record (EfModelAnalyzer) Bug fix (surfaced by new coverage): FluentOwnedTypeWalker.ResolveOwnedType seeded zero columns for an OwnsMany over a T[] array navigation -- its `is not INamedTypeSymbol` guard rejected IArrayTypeSymbol, though the file-discovery path (OwnedClrTypeName) already unwraps T[]. Now unwraps the array element type up front so both paths agree. Coverage: 15 new *CoverageTests.cs files (~207 tests) across CLI, MCP, Core, ClassDiagram and EntityFramework, targeting the worst-covered error/fallback paths (ErdCommand, CollectingOutputConsole, DiagramResourceCache, CompilationFactory, the EF fluent walkers, ...). 1118 tests passing; build clean under TreatWarningsAsErrors and EnforceCodeStyleInBuild. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving code quality and resilience across ProjGraph (notably EF Fluent API analysis and MCP/CLI robustness) by clearing SonarQube smells, fixing an owned-type array edge case, and substantially expanding automated coverage for error/fallback paths.
Changes:
- Refactors EF Fluent API ownership resolution and owned-type handling to reduce complexity/parameter counts and fix
OwnsManyover array navigations. - Adds broad “coverage tests” suites across EF, Core, ClassDiagram, CLI, and MCP to exercise guard rails and degraded/fallback behavior.
- Adds renderer and discovery edge-case coverage to ensure diagrams remain valid under unexpected model states.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ProjGraph.Tests.Unit.EntityFramework/FluentWalkerCoverageTests.cs | New unit coverage for EF Fluent API walkers’ guard/fallback paths. |
| tests/ProjGraph.Tests.Unit.EntityFramework/FluentSyntaxCoverageTests.cs | New unit coverage for FluentSyntax receiver-chain/ancestor resolution and name extraction. |
| tests/ProjGraph.Tests.Unit.EntityFramework/EfRenderingCoverageTests.cs | New unit coverage for Mermaid ERD renderer defensive fallbacks. |
| tests/ProjGraph.Tests.Unit.EntityFramework/EfDiscoveryCoverageTests.cs | New unit coverage for EF file discovery boundaries and array-owned navigation discovery. |
| tests/ProjGraph.Tests.Unit.EntityFramework/EfAnalyzerCoverageTests.cs | New unit coverage for EF Roslyn analyzers on unusual attribute/type shapes. |
| tests/ProjGraph.Tests.Unit.Core/ProjectParserCoverageTests.cs | New unit coverage for malformed MSBuild inputs and property/version fallback logic. |
| tests/ProjGraph.Tests.Unit.Core/OutputConsoleCoverageTests.cs | New unit coverage for SpectreOutputConsole stdout/stderr routing and markup escaping. |
| tests/ProjGraph.Tests.Unit.Core/CompilationFactoryCoverageTests.cs | New unit coverage for compilation options, references, and cross-tree binding behavior. |
| tests/ProjGraph.Tests.Unit.ClassDiagram/ClassDiagramCoverageTests.cs | New unit coverage for class-diagram discovery/resolution fallbacks and renderer defaults. |
| tests/ProjGraph.Tests.Integration.Mcp/WorkspaceRootServiceTests.cs | New integration coverage for MCP workspace roots negotiation, caching, and invalidation. |
| tests/ProjGraph.Tests.Integration.Mcp/Helpers/InProcessMcpSession.cs | Adds in-memory client/server harness for MCP integration testing. |
| tests/ProjGraph.Tests.Integration.Mcp/DiagramResourceCacheTests.cs | New integration coverage for publishing/eviction/update notifications of cached MCP resources. |
| tests/ProjGraph.Tests.Integration.Mcp/CollectingOutputConsoleTests.cs | New coverage for MCP stdout-safe console warning buffering and async-flow isolation. |
| tests/ProjGraph.Tests.Integration.Cli/ErdCommandCoverageTests.cs | New CLI integration coverage for ERD discovery/prompting/non-interactive behavior and snapshot branch. |
| tests/ProjGraph.Tests.Integration.Cli/CommandOptionsCoverageTests.cs | New CLI integration coverage for validation/error reporting branches and DI adapter behavior. |
| src/ProjGraph.Lib.EntityFramework/Infrastructure/SqlColumnTypeMapper.cs | Removes repeated "decimal" literal via shared constant for Sonar smell cleanup. |
| src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentSyntax.cs | Refactors ownership resolution to reduce cognitive complexity (receiver-chain vs ancestor search). |
| src/ProjGraph.Lib.EntityFramework/Infrastructure/FluentOwnedTypeWalker.cs | Refactors owned creation parameters and fixes OwnsMany array element-type seeding. |
| src/ProjGraph.Lib.EntityFramework/Infrastructure/EfModelAnalyzer.cs | Refactors owned-navigation resolution state into a context record to reduce parameter count. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…fy CI The CI's `dotnet format --verify-no-changes` whitespace check is stricter than the local `EnforceCodeStyleInBuild` build and flagged two multi-property object initializers in FluentWalkerCoverageTests. Reformat one property per line. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves the 4 open SonarQube code smells and lifts coverage from 89.8% → ~95.8% (line 91.8% → 97.4%, branch 84.1% → 91.6%; measured locally via opencover with the CI's
sonar.coverage.exclusions).Code smells (all in
Lib.EntityFramework)SqlColumnTypeMapper.cs"decimal"literal →DecimalconstFluentSyntax.csResolveOwningEntity(cognitive complexity 22) split intoResolveFromReceiverChain/ResolveFromAncestors/ComposeOwnedKeyFluentOwnedTypeWalker.csGetOrCreateOwned8 params → 5 viaOwnedTargetrecordEfModelAnalyzer.csResolveOwnedNavigationTypesAsync8 params → 3 viaOwnedTypeResolutionContextrecordAll refactors are behavior-preserving; the S3776 split keeps each
returnin the receiver-chain loop terminal (including theUsingEntity→nullcase) by signalling "the chain settled it" rather than falling through to the ancestor search.Bug fix (surfaced by new coverage)
FluentOwnedTypeWalker.ResolveOwnedTypeseeded zero columns for anOwnsManyover aT[]array navigation — itsis not INamedTypeSymbolguard rejectedIArrayTypeSymbol, even though the file-discovery path (OwnedClrTypeName) already unwrapsT[]and documents it as supported. The seeding path now unwraps the array element type up front so both paths agree. Covered by a new test.Coverage
15 new
*CoverageTests.csfiles (~207 tests) across CLI, MCP, Core, ClassDiagram and EntityFramework, targeting the worst-covered error/fallback paths (ErdCommand60%,CollectingOutputConsole44%,DiagramResourceCache62%,CompilationFactory69%, the EF fluent walkers, …).Verification
TreatWarningsAsErrors+EnforceCodeStyleInBuild.developpush after merge (thesonarqube.ymlworkflow only triggers on push todevelop, not on PRs).Note (pre-existing, out of scope)
McpTransportTests.ListTools_OverRealStdioTransport_ExposesAllFourToolsis flaky under full-suite parallel load (65s init timeout) but passes standalone in ~5s. Not touched here; worth a separate stabilization fix.🤖 Generated with Claude Code