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 |