Sonar: fix all 8 code smells + raise coverage 61.2% → ~81%#37
Merged
Conversation
… constants (S1192)
…76) + cover all rules
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…/craft/research loaders Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rsAsync (S3776) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ested options POCOs from coverage
…esearch handlers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add hosted-service integration tests for SwitchesHostedService (5 loops), StorageMonitorsHostedService (4 loops), and AlarmsHostedService (4 loops), exercising every Consume*Async path via the real InMemoryEventBus + poll-until-received pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rage (unmockable DiscordSocketClient)
…layers hosted services Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, not per-event resilience Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving SonarQube quality metrics by refactoring high-complexity areas into smaller helpers and adding targeted unit tests to raise overall coverage while keeping runtime behavior unchanged.
Changes:
- Refactors generator and validation code to reduce cognitive complexity (method extraction, shared parsers, shared responders).
- Adds substantial new test coverage across generator sources/validation and multiple hosted services.
- Updates Sonar workflow configuration to broaden coverage exclusions for code that is difficult to unit test.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/RustPlusBot.ItemData.Generator/Validation/DatasetValidator.cs | Extracts validation rules into smaller methods to reduce cognitive complexity. |
| tools/RustPlusBot.ItemData.Generator/Sources/OfflineRustLabsSource.cs | Extracts JSON parsing helpers to reduce duplication/complexity. |
| tools/RustPlusBot.ItemData.Generator/Program.cs | Extracts argument parsing and item-building/reporting into helpers. |
| tests/RustPlusBot.ItemData.Generator.Tests/OfflineStackSourceTests.cs | Adds coverage for stack-size parsing and skip cases. |
| tests/RustPlusBot.ItemData.Generator.Tests/OfflineRustLabsSourceTests.cs | Adds coverage for recycle yields, craft recipes, and research costs parsing. |
| tests/RustPlusBot.ItemData.Generator.Tests/OfflineNamesSourceTests.cs | Adds coverage for name parsing and skip cases. |
| tests/RustPlusBot.ItemData.Generator.Tests/OfflineDespawnSourceTests.cs | Adds coverage for despawn-time parsing and skip cases. |
| tests/RustPlusBot.ItemData.Generator.Tests/DatasetValidatorTests.cs | Extends validator tests (smelter + CCTV edge cases). |
| tests/RustPlusBot.Features.Switches.Tests/Hosting/SwitchesHostedServiceTests.cs | Adds bus-routing and crash-isolation tests for switches hosted service. |
| tests/RustPlusBot.Features.StorageMonitors.Tests/Hosting/StorageMonitorsHostedServiceTests.cs | Adds bus-routing and crash-isolation tests for storage monitors hosted service. |
| tests/RustPlusBot.Features.Players.Tests/Hosting/PlayersHostedServiceTests.cs | Adds event-routing and fault isolation tests for player events hosted service. |
| tests/RustPlusBot.Features.Commands.Tests/Hosting/CommandsHostedServiceTests.cs | Adds dispatch-loop routing/robustness tests for commands hosted service. |
| tests/RustPlusBot.Features.Chat.Tests/Hosting/ChatHostedServiceTests.cs | Adds relay-loop routing/robustness tests for chat hosted service. |
| tests/RustPlusBot.Features.Alarms.Tests/Hosting/AlarmsHostedServiceTests.cs | Adds routing/refresh behavior tests for alarms hosted service. |
| src/RustPlusBot.Features.Connections/Supervisor/ConnectionSupervisor.cs | Extracts marker delta publishing into helper method. |
| src/RustPlusBot.Features.Commands/Modules/ItemCommandModule.cs | Deduplicates embed response logic and string literal keys. |
| .github/workflows/Sonar.yml | Expands sonar.coverage.exclusions patterns to exclude additional “untestable” code paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+123
to
+127
| var minIdx = argList.IndexOf("--min-items"); | ||
| if (minIdx >= 0 && minIdx + 1 < argList.Count) | ||
| { | ||
| minItems = int.Parse(argList[minIdx + 1], System.Globalization.CultureInfo.InvariantCulture); | ||
| } |
Comment on lines
+636
to
+637
| var added = current.Where(c => previous.All(p => p.Id != c.Id)).ToList(); | ||
| var removed = previous.Where(p => current.All(c => c.Id != p.Id)).ToList(); |
Comment on lines
+75
to
+82
| var deadline = DateTimeOffset.UtcNow.AddSeconds(20); | ||
| while (DateTimeOffset.UtcNow < deadline | ||
| && !h.PairingPoster.ReceivedCalls().Any(c => | ||
| c.GetMethodInfo().Name == nameof(ISwitchChannelPoster.EnsureAsync))) | ||
| { | ||
| await h.Bus.PublishAsync(new SwitchPairedEvent(10UL, serverId, 42UL)); | ||
| await Task.Delay(20); | ||
| } |
Comment on lines
+80
to
+87
| var deadline = DateTimeOffset.UtcNow.AddSeconds(20); | ||
| while (DateTimeOffset.UtcNow < deadline | ||
| && !h.PairingPoster.ReceivedCalls().Any(c => | ||
| c.GetMethodInfo().Name == nameof(IStorageMonitorChannelPoster.EnsureAsync))) | ||
| { | ||
| await h.Bus.PublishAsync(new StorageMonitorPairedEvent(Guild, serverId, 7UL)); | ||
| await Task.Delay(20); | ||
| } |
Comment on lines
+57
to
+67
| var deadline = DateTimeOffset.UtcNow.AddSeconds(20); | ||
| while (DateTimeOffset.UtcNow < deadline | ||
| && !h.Sender.ReceivedCalls().Any(c => | ||
| c.GetMethodInfo().Name == nameof(ITeamChatSender.SendAsync))) | ||
| { | ||
| await h.Bus.PublishAsync(new PlayerStateChangedEvent( | ||
| 10UL, serverId, | ||
| new MapDimensions(3000, 3000, 0), | ||
| [new PlayerTransition(PlayerTransitionKind.Connect, 1UL, "Alice", null)])); | ||
| await Task.Delay(20); | ||
| } |
Comment on lines
+63
to
+71
| var deadline = DateTimeOffset.UtcNow.AddSeconds(20); | ||
| while (DateTimeOffset.UtcNow < deadline | ||
| && !h.Sender.ReceivedCalls().Any(c => | ||
| c.GetMethodInfo().Name == nameof(ITeamChatSender.SendAsync))) | ||
| { | ||
| await h.Bus.PublishAsync( | ||
| new TeamMessageReceivedEvent(10UL, serverId, 7UL, "alice", "!pop", FromActivePlayer: false)); | ||
| await Task.Delay(20); | ||
| } |
Comment on lines
+55
to
+63
| var deadline = DateTimeOffset.UtcNow.AddSeconds(20); | ||
| while (DateTimeOffset.UtcNow < deadline | ||
| && !poster.ReceivedCalls().Any(c => | ||
| c.GetMethodInfo().Name == nameof(ITeamChatWebhookPoster.PostAsync))) | ||
| { | ||
| await bus.PublishAsync( | ||
| new TeamMessageReceivedEvent(10UL, Guid.NewGuid(), 1UL, "Alice", "hello", FromActivePlayer: false)); | ||
| await Task.Delay(20); | ||
| } |
Comment on lines
+91
to
+98
| var deadline = DateTimeOffset.UtcNow.AddSeconds(20); | ||
| while (DateTimeOffset.UtcNow < deadline | ||
| && !h.PairingPoster.ReceivedCalls().Any(c => | ||
| c.GetMethodInfo().Name == nameof(IAlarmChannelPoster.EnsureAsync))) | ||
| { | ||
| await h.Bus.PublishAsync(new AlarmPairedEvent(10UL, serverId, 42UL)); | ||
| await Task.Delay(20); | ||
| } |
…value instead of crashing Addresses Copilot review comment on PR #37.
Owner
Author
|
Thanks @copilot — dispositions below. ✅ Fixed —
|
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
Improves the SonarQube quality metrics on
develop.Smell fixes (all behavior-preserving)
S1192(repeated string literals) —ItemCommandModule: extracted constants and collapsed the four near-identicalRespondFor*Asyncresponders into one shared embed helper + describe-delegates.S3776(cognitive complexity) — extracted methods inDatasetValidator(63→per-rule), generatorProgram.cs(ParseArgs/BuildItems/ReportOrphans),OfflineRustLabsSource(×2 parsers), andConnectionSupervisor.PollMarkersAsync(PublishMarkerDeltaAsync).Coverage (exclude untestable boilerplate + write real tests)
sonar.coverage.exclusionsnow also drops genuinely-untestable code: composition roots (Program.cs), DI*ServiceCollectionExtensions, DiscordModules/**and the Discord-gateway I/O layer (posters/messenger/webhook/gateway/bot-service — all need a liveDiscordSocketClient),DesignTimeDbContextFactory, and 3 untested*OptionsPOCOs. Tested Options (CommandOptions/ConnectionOptions/PairingOptions) and the Rust WebSocket sources are kept in the metric.Verification
-maxcpucount:1: 0 warnings / 0 errors (warnings-as-errors; noS1192/S3776emitted).jb cleanupcode --profile=ReformatAndReorder: idempotent-clean (format gate green).Notes
developon push, so the published numbers update after merge. The ~81% figure is a local proxy computed with the same exclusion globs; the smell fixes are confirmed by the clean warnings-as-errors build.🤖 Generated with Claude Code