From f6ac7ca8a2d8bd2901cd6ae4b889897ef81899a3 Mon Sep 17 00:00:00 2001 From: kevin Date: Mon, 10 Aug 2026 12:42:12 +0200 Subject: [PATCH] Fix the real Sonar LOW issues and make the Uno exclusion actually apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All 62 open SonarCloud issues were LOW, but 61 of them were false positives that cannot be fixed in code. The cause is server-side: Automatic Analysis is still enabled (sonar.autoscan.enabled=true), so every sonarscanner end step is rejected and sonar.yml has never published an analysis. The results on SonarCloud therefore come from a mode that never builds, which is why the Uno app drew 57 bogus S2325 "make it static" findings (every read of an [ObservableProperty]-generated property, every IValueConverter member) and 4 S8970 "nullable warnings are disabled here" although Directory.Build.props sets enable. Applying any of them would not compile. The same conflict makes every /d: setting in sonar.yml inert, including the Source/Trackify/**/*.cs exclusion added in #10 — which is why those findings never closed. Mirror that exclusion into .sonarcloud.properties, the file automatic analysis actually reads, so it takes effect in the mode that is running today and stays correct after the switch is flipped. Genuine fixes, verified with SonarAnalyzer.CSharp 10.31.0.145097 run locally over the analysed scope (now clean): - S3878 in LwpAddressingMapping.ParseMacAddress. The suggested fix is a silent bug: Split(':', '-') binds to (char, int count) because '-' converts to int, so the '-' form would stop parsing. Use the non-params (char[], StringSplitOptions) overload, which satisfies the rule and keeps the behaviour, and add tests covering both separators. - S1144 unused Domain constant in LayerTrainDependencyTests. - S8969 redundant null-forgiving operator after Assert.NotNull. The last two are not reported today (Test/ is only analysed by the CI scan) but would surface the moment it starts publishing. Document the conflict as R-9 and the local analyzer procedure in arc42 7.6. Co-Authored-By: Claude Opus 5 --- .github/workflows/sonar.yml | 7 ++++ .sonarcloud.properties | 18 ++++++++-- .../Lego/LwpAddressingMapping.cs | 6 ++-- .../Application/LwpAddressingMappingTests.cs | 34 +++++++++++++++++++ .../Architecture/LayerTrainDependencyTests.cs | 3 +- .../Domain/SpeedFunctionTests.cs | 2 +- docs/arc42/07-deployment-view.md | 25 ++++++++++++++ docs/arc42/11-risks-and-technical-debt.md | 28 +++++++++++++++ 8 files changed, 116 insertions(+), 7 deletions(-) create mode 100644 Test/Trackify.Tests/Application/LwpAddressingMappingTests.cs diff --git a/.github/workflows/sonar.yml b/.github/workflows/sonar.yml index 8350b02..f3adf58 100644 --- a/.github/workflows/sonar.yml +++ b/.github/workflows/sonar.yml @@ -15,6 +15,13 @@ name: SonarCloud # https://sonarcloud.io/project/settings?id=Ktechen_Trackify -> Analysis Method # after which every run here is accepted. # +# !! THAT HAS NOT HAPPENED YET (`sonar.autoscan.enabled=true`), so every run of this workflow to date +# has failed at `Sonar end` and NONE of the /d: settings below have ever taken effect — the coverage +# report is produced and never consumed, and the scope exclusions are inert. What actually scopes the +# published results today is .sonarcloud.properties, the file automatic analysis reads. Keep its +# exclusion list identical to the one here, or a scope change silently does nothing. See R-9 in +# docs/arc42/11-risks-and-technical-debt.md. +# # Scope note: the scanner for .NET only sees projects that are built between `begin` and `end`. As in # ci.yml the Uno app (Source/Trackify, five heads) is not buildable on a Linux runner, so the # analysed scope here is the shared core + CLI + tests. diff --git a/.sonarcloud.properties b/.sonarcloud.properties index f7d12df..a407fd7 100644 --- a/.sonarcloud.properties +++ b/.sonarcloud.properties @@ -2,9 +2,21 @@ # # The CI scan in .github/workflows/sonar.yml is the real analysis (it builds the projects and uploads # coverage) and carries its own /d:sonar.* settings; automatic analysis has to be switched off for that -# scan to be accepted, at which point this file is simply ignored. It is kept so the exclusion below -# also applies while automatic analysis is still the active mode. +# scan to be accepted, at which point this file is simply ignored. It is kept because automatic +# analysis is still the active mode (`sonar.autoscan.enabled=true` on the project), so today this file +# — not the workflow — is what actually scopes the published results. Keep the two exclusion lists +# identical; anything only in the workflow has no effect until the mode is switched. # # docs/ is the vendored `lego-ble-wireless-protocol-docs` submodule: third-party LEGO protocol # documentation (HTML/JS) that we neither own nor maintain. -sonar.exclusions=docs/** +# +# Source/Trackify/**/*.cs is the Uno app. Automatic analysis never builds, so the C# analyser sees it +# without a compilation: the CommunityToolkit.Mvvm source generator has not run and the WinUI/Uno +# references are unresolved. Every member that reads an [ObservableProperty]-generated property, and +# every IValueConverter implementation, therefore looks like it uses no instance state — 57 bogus +# S2325 "make it static" findings (uncompilable if applied), plus S8970 "nullable warnings are +# disabled here" although Directory.Build.props sets enable. The app is not +# analysable in either mode — the CI scan can't build its five heads on a Linux runner either, and +# excludes the same path — so exclude it rather than leave unfixable findings open. +# Only the .cs is excluded: the WasmScripts JS and any XML still get analysed. +sonar.exclusions=docs/**,Source/Trackify/**/*.cs diff --git a/Source/Trackify.Application/Lego/LwpAddressingMapping.cs b/Source/Trackify.Application/Lego/LwpAddressingMapping.cs index ed74cfb..5168b8a 100644 --- a/Source/Trackify.Application/Lego/LwpAddressingMapping.cs +++ b/Source/Trackify.Application/Lego/LwpAddressingMapping.cs @@ -24,7 +24,9 @@ public static string FormatMacAddress(ulong address) /// Parses "AA:BB:CC:DD:EE:FF" (or '-' separated) back into a 48-bit address. public static ulong ParseMacAddress(string mac) - // Explicit separator array: Split(':', '-') also binds to the (char, int count) overload. - => mac.Split([':', '-']) + // Separator array *and* explicit options on purpose: Split(':', '-') binds to the + // (char, int count) overload instead ('-' converts to int), and only the non-params + // (char[], StringSplitOptions) overload takes the array without S3878 calling it redundant. + => mac.Split([':', '-'], StringSplitOptions.None) .Aggregate(0, (current, part) => (current << 8) | Convert.ToByte(part, 16)); } diff --git a/Test/Trackify.Tests/Application/LwpAddressingMappingTests.cs b/Test/Trackify.Tests/Application/LwpAddressingMappingTests.cs new file mode 100644 index 0000000..37d4a90 --- /dev/null +++ b/Test/Trackify.Tests/Application/LwpAddressingMappingTests.cs @@ -0,0 +1,34 @@ +using Trackify.Application.Lego; + +namespace Trackify.Tests.Application; + +public class LwpAddressingMappingTests +{ + [Theory] + [InlineData("90:84:2B:4E:5B:96")] + [InlineData("90-84-2B-4E-5B-96")] + public void Parses_both_separators_to_the_same_address(string mac) + => Assert.Equal(0x90842B4E5B96UL, LwpAddressingMapping.ParseMacAddress(mac)); + + // Guards the deliberate Split([':', '-'], StringSplitOptions.None) overload choice: the shorter + // Split(':', '-') binds to (char separator, int count) instead — '-' converts to int 45 — which + // silently splits on ':' only and leaves the '-' form as one unparsable 17-character token. + [Fact] + public void Round_trips_through_the_formatter() + { + const ulong address = 0x0011AAFF7788UL; + var formatted = LwpAddressingMapping.FormatMacAddress(address); + + Assert.Equal("00:11:AA:FF:77:88", formatted); + Assert.Equal(address, LwpAddressingMapping.ParseMacAddress(formatted)); + Assert.Equal(address, LwpAddressingMapping.ParseMacAddress(formatted.Replace(':', '-'))); + } + + [Fact] + public void Maps_the_rgb_led_port_per_hub_model() + { + Assert.Equal((byte)50, LwpAddressingMapping.RgbLedPortFor(HubType.PoweredUpHub)); + Assert.Equal((byte)17, LwpAddressingMapping.RgbLedPortFor(HubType.DuploTrainHub)); + Assert.Null(LwpAddressingMapping.RgbLedPortFor(HubType.WeDo2SmartHub)); + } +} diff --git a/Test/Trackify.Tests/Architecture/LayerTrainDependencyTests.cs b/Test/Trackify.Tests/Architecture/LayerTrainDependencyTests.cs index 4ad1e34..8b9a5c6 100644 --- a/Test/Trackify.Tests/Architecture/LayerTrainDependencyTests.cs +++ b/Test/Trackify.Tests/Architecture/LayerTrainDependencyTests.cs @@ -17,7 +17,8 @@ namespace Trackify.Tests.Architecture; /// public class LayerTrainDependencyTests { - private const string Domain = "Trackify.Domain"; + // No "Trackify.Domain" constant: Domain is the innermost layer, so every other layer is allowed + // to depend on it and no assertion below ever names it. private const string Application = "Trackify.Application"; private const string Infrastructure = "Trackify.Infrastructure"; private const string Cli = "Trackify.Cli"; diff --git a/Test/Trackify.Tests/Domain/SpeedFunctionTests.cs b/Test/Trackify.Tests/Domain/SpeedFunctionTests.cs index 52644db..ed140d7 100644 --- a/Test/Trackify.Tests/Domain/SpeedFunctionTests.cs +++ b/Test/Trackify.Tests/Domain/SpeedFunctionTests.cs @@ -16,7 +16,7 @@ public void TryCompile_accepts_a_valid_formula() { Assert.True(SpeedFunction.TryCompile("1-(1-x)^2", out var fn)); Assert.NotNull(fn); - Assert.Equal(0.0, fn!(0), 3); + Assert.Equal(0.0, fn(0), 3); Assert.Equal(1.0, fn(1), 3); } diff --git a/docs/arc42/07-deployment-view.md b/docs/arc42/07-deployment-view.md index eac8a75..9dfd945 100644 --- a/docs/arc42/07-deployment-view.md +++ b/docs/arc42/07-deployment-view.md @@ -221,3 +221,28 @@ dotnet run --project Source/Trackify/Trackify.csproj -f net10.0-desktop # laun Buildable heads outside macOS: `net10.0-android`, `net10.0-desktop`, `net10.0-browserwasm`, `net10.0-windows10.0.19041.0`. Real BLE behaviour is confirmed on a device and on a Pi — it cannot be exercised in CI. + +**Checking SonarCloud's C# rules locally.** The scan needs a `SONAR_TOKEN` and only runs in CI, so a +fix to a Sonar finding is otherwise unverifiable before pushing — and while +[R-9](11-risks-and-technical-debt.md) stands, the published findings are not trustworthy anyway. The +same rules can be run offline by injecting the analyzer package without touching any repo file: write + +```xml + + + +``` + +to a scratch file and pass it as `CustomAfterMicrosoftCommonProps`: + +```bash +dotnet build Test/Trackify.Tests/Trackify.Tests.csproj -t:Rebuild \ + -p:CustomAfterMicrosoftCommonProps= \ + -p:TreatWarningsAsErrors=false -p:EnforceCodeStyleInBuild=false +``` + +Building the **test** project covers the whole analysed scope, since it references the core libs and +the CLI. `VersionOverride` bypasses Central Package Management; the two overrides stop warnings-as-errors +from aborting the build before every rule has reported. Keep the version in step with the +`sonar.cs.analyzer.dotnet.pluginVersion` the server reports. Non-C# rules (docker, XML, YAML, +JavaScript) have no local equivalent — reason about those from the rule description. diff --git a/docs/arc42/11-risks-and-technical-debt.md b/docs/arc42/11-risks-and-technical-debt.md index 3f75c2d..1756489 100644 --- a/docs/arc42/11-risks-and-technical-debt.md +++ b/docs/arc42/11-risks-and-technical-debt.md @@ -89,6 +89,34 @@ have no hub address recorded yet will be merged into one local row. - **Realistic trigger:** trains created in the app before discovery has run, then synced from a Pi. - **Fix if it bites:** drop the name fallback and require a hub identity for matching. +### R-9: `sonar.yml` has never published an analysis (high impact, needs a one-off admin action) + +The project still has **Automatic Analysis** enabled server-side (`sonar.autoscan.enabled=true`), and +the two modes are mutually exclusive: every `sonarscanner end` step is rejected with *"You are running +CI analysis while Automatic Analysis is enabled"*, so the workflow has failed on **every** run to date +while the `begin` and test steps pass. Three consequences, all of them easy to misread: + +- **Coverage is not measured at all.** The OpenCover report is produced and never consumed, so the + coverage metric and its badge come from nothing. +- **Every `/d:sonar.*` setting in `sonar.yml` is inert**, including the scope exclusions. What actually + scopes the published results is [`.sonarcloud.properties`](../../.sonarcloud.properties), the file + Automatic Analysis reads — which is why the two exclusion lists must be kept identical. +- **The published C# findings are analysed without a compilation.** Source generators have not run and + package references are unresolved, so the results are unreliable in exactly the places that depend on + either — most visibly the Uno app, where every read of a `[ObservableProperty]`-generated property + and every `IValueConverter` member drew a bogus `S2325` "make it static", and `S8970` claimed nullable + warnings were disabled although `Directory.Build.props` sets `enable`. Applying + those "fixes" would not compile. The app is excluded for this reason. + +- **Mitigation today:** C# rules are reproduced locally against the same analyzer version by injecting + `SonarAnalyzer.CSharp` through `CustomAfterMicrosoftCommonProps` — + see [§7.6](07-deployment-view.md#76-build-and-delivery-pipeline). +- **Fix:** a project admin turns Automatic Analysis off once, at + [Project Settings → Analysis Method](https://sonarcloud.io/project/settings?id=Ktechen_Trackify). + Nothing in this repository can do it — it is a server-side setting, and a `sonar-project.properties` + file (sometimes suggested) is rejected outright by the scanner for .NET. After the switch the CI scan + is accepted, coverage starts reporting, and the workflow's own exclusions take over. + ## 11.2 Technical debt | # | Debt | Where | Cost of leaving it |