chore(sonar): re-justify coverage.exclusions + surface testable services (ui-mobile#243 Phase 1) - #316
Conversation
…es (orbit-ui-mobile#243 Phase 1) Phase 1 (config only, no app code) of the pre-launch SonarCloud gate — the consumer side is thomasluizon/orbit-ui-mobile#243. Make the coverage surface honest before any burn-down. - Widen /Extensions/ServiceCollectionExtensions.cs -> ServiceCollectionExtensions*.cs: the exact-name glob matched only 1 of 4 DI partials; all 4 are DI registration, so exclude them consistently. - DROP Services/AppConfigService.cs (pure ConvertValue<T> + injectable cache/repo) and Services/UserDateService.cs (user-timezone "today" + week-start, injectable deps) from coverage.exclusions — both are unit-testable and live in Orbit.Infrastructure, which is already in the coverage run. - Every remaining exclusion re-justified inline (bootstrap/DI, EF/generated, options POCOs, external-SDK glue, hosted BackgroundServices), with Phase-2 extract targets flagged: MinimumVersionMiddleware's semver comparator, GeoLocation's IsPrivateIp classifier, GoogleToken's outcome classification. opencover paths (**/coverage.opencover.xml) span all three coverage-producing test projects (Domain/Application/Infrastructure.Tests); Analyzers.Tests runs outside the runtime coverage set by design, and there is no Api test project yet. Confirmed, unchanged. Paired PR: thomasluizon/orbit-ui-mobile#446. thomasluizon/orbit-ui-mobile#243
|
There was a problem hiding this comment.
/pr-review — PR #316
Scope: chore(sonar): re-justify coverage.exclusions + surface testable services (ui-mobile#243 Phase 1)
Recommendation: APPROVE
Prior reviews: none found (gh pr view 316 --json reviews,comments returned zero) — full diff reviewed.
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 1 |
Summary
Single-file, config-only diff to .github/workflows/sonarcloud.yml (32 additions, 1 deletion, no src/ code touched). It:
- Widens the
sonar.coverage.exclusionsDI glob from the exact filenameServiceCollectionExtensions.cstoServiceCollectionExtensions*.cs - Drops
Services/AppConfigService.csandServices/UserDateService.csfrom the exclusion list - Adds an inline WHY-comment justifying every remaining exclusion entry
Verification performed (against tree at HEAD a87d645)
- DI glob fix is a real bug fix: exactly 4 files match the widened glob (
ServiceCollectionExtensions.cs,.AiServices.cs,.Infrastructure.cs,.BackgroundJobs.cs), all confirmedpartial class ServiceCollectionExtensionsinsrc/Orbit.Api/Extensions/. Under the old exact-name glob, 3 of 4 DI partials were silently inside coverage scope. No unrelated file falls under the widened glob. - AppConfigService.cs: ctor takes
OrbitDbContext+IMemoryCache(both injectable),ConvertValue<T>is pure static logic — testable claim confirmed. - UserDateService.cs: already has
tests/Orbit.Infrastructure.Tests/Services/UserDateServiceTests.cs, corroborating "unit-testable." - Every other exclusion entry resolves to a real path — no stale entries, no dangling globs.
- Entry count: 27 → 25 (2 dropped, 1 widened in place) — matches PR body.
tests/Orbit.IntegrationTests"hollow dir" claim: confirmed absent from tree.- Coverage test invocations: target exactly
Orbit.Domain.Tests,Orbit.Application.Tests,Orbit.Infrastructure.Tests;Orbit.Analyzers.Testsis outside the coverage run, as claimed. - No new CI/security risk: no
github.event.*interpolation in anyrun:block, permissions/trigger blocks unchanged.
Findings
Critical / High / Medium: None.
Low / Info:
[INFO] Verbose but justified WHY-comment block
· location: .github/workflows/sonarcloud.yml:37-63
· issue: 27-line comment enumerating every coverage.exclusions entry with a one-line justification.
· risk: None — each line is WHY (exclusion rationale), not WHAT; unusually long for a config file but compliant.
· fix: No action needed; if it grows further, consider linking a doc instead.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | N/A — gate false, only workflow YAML changed; manually spot-checked anyway, no injection/fork-secret risk |
| contract-aligner | N/A — no DTO/Controller/packages/shared touched |
Validation
Build/Tests: skipped per this PR's separate required CI checks (Build / Unit Tests / SonarCloud).
Deferred (all N/A — no application code, no cross-repo surface in diff)
Dimensions 3,5,6,7,8,9,10,11,13,14; cross-repo packages/shared backward-compat not verifiable in this session (no ui-mobile checkout) and moot — no contract surface in diff.
What's good
- Every exclusion entry now has a falsifiable, independently-verified justification — held up under path-by-path checking with zero discrepancies.
- The DI-glob change is a genuine bug fix (3 of 4 partials were leaking into coverage scope), not cosmetic.
- Dropping the two testable services is a real coverage-honesty improvement ahead of the metric burn-down described in the PR body.



Phase 1 (step a) of orbit-ui-mobile#243 — SonarCloud coverage surface, orbit-api
Config only, no app code. Paired with orbit-ui-mobile#446. Get the coverage-exclusion set honest before the metric burn-down, so the numbers mean something. Does not flip the Sonar check to required (that's step d).
What changed (
.github/workflows/sonarcloud.yml)Widened the DI glob.
**/Extensions/ServiceCollectionExtensions.cs→**/Extensions/ServiceCollectionExtensions*.cs. The exact-name glob matched only 1 of 4 DI partials; the other three (.AiServices,.BackgroundJobs,.Infrastructure) were silently in coverage scope. All four are DI registration → all bootstrap → now consistently excluded (the glob does what its name implies).Dropped two testable services from
coverage.exclusions:Services/AppConfigService.csConvertValue<T>is pure type-parse-with-fallback logic; the cache/repo deps are injectable. Lives inOrbit.Infrastructure, which is in the coverage run — unit-testable, should count.Services/UserDateService.csGetUserTodayAsyncis real timezone→DateOnlylogic + week-start default with injectable deps. This is the mandatedIUserDateService— core, testable, wrongly hidden.Everything else re-justified inline (see the comment block above the SonarScanner Begin step): bootstrap/DI, EF/generated, options POCOs, external-SDK glue (OpenAI/Stripe/Firebase/Google/geo HTTP), and hosted BackgroundServices.
sonar.exclusions(Migrations/bin/obj/OAuthLoginPage/.claude) all still exist and remain justified — unchanged.Confirmations (task)
**/coverage.opencover.xmlcaptures the three projects the workflow runs with coverage:Orbit.Domain.Tests,Orbit.Application.Tests,Orbit.Infrastructure.Tests.Orbit.Analyzers.Testsexists but runs outside the runtime coverage set by design (analyzers arecoverage.exclusions-excluded, netstandard2.0 build-time tooling).tests/Orbit.IntegrationTestsis a hollow dir (no.csproj, no tracked files — stale local bin/obj only). There is noOrbit.Api.Testsproject, so Api-layer code (Program/Extensions/Middleware/OpenApi/OAuth) has no coverage infra — this is why the whole layer is exclusion-covered.push: [main], so main is analyzed and overall metrics are tracked, not just PR deltas. Unchanged. (The SonarCloud new-code definition is a dashboard setting outside this file — verify in the project UI at step d.)Handoff to Phase 2 (do NOT do here)
Testable logic still behind broad exclusions, flagged inline for the burn-down to extract to a testable layer + cover (rather than keep suppressed):
MinimumVersionMiddleware(Middleware/**) — pure semver comparator (IsVersionBelow/NormalizeVersion); server-authoritative version gate. Needs anOrbit.Apitest project or extraction toApplication.GeoLocationService.IsPrivateIpAddress— pure RFC1918/IPv6 classifier behind the ipapi HTTP call.GoogleTokenService— token-outcome classification (invalid_grant/unauthorized_client) testable via a fakeHttpMessageHandler.ReminderSchedulerService— pure reminder-text/decision helpers.thomasluizon/orbit-ui-mobile#243