Skip to content

Fix SonarCloud issues and unblock the build - #38

Merged
matt-edmondson merged 3 commits into
mainfrom
chore/sonarcloud-cleanup
Aug 14, 2026
Merged

Fix SonarCloud issues and unblock the build#38
matt-edmondson merged 3 commits into
mainfrom
chore/sonarcloud-cleanup

Conversation

@matt-edmondson

@matt-edmondson matt-edmondson commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Fixes all 90 open SonarCloud issues on ktsu-dev_Containers — 57 by code change, 33 by excluding the benchmark harness from analysis — and unblocks the build along the way.

Build fixes (were failing dotnet build)

main does not currently build locally against the pinned ktsu.Sdk 2.26.1, which added two analyzers:

  • KTSU0007Polyfill now sets PrivateAssets="all" so this build-time-only package stops leaking into every consumer's dependency graph.
  • KTSU0002 — added Containers/AssemblyInfo.cs exposing internals to ktsu.Containers.Test.

SonarCloud fixes — 57 of 90 open issues

Tests — 46

Rule Count Change
MSTEST0068 34 CollectionAssert.AreEqualAssert.AreSequenceEqual
MSTEST0037 11 Assert.AreEqual(n, x.Length)Assert.HasCount(n, x); Assert.IsTrue(a >= b)Assert.IsGreaterThanOrEqualTo(b, a)
MSTEST0049 1 pass TestContext.CancellationToken to the Task.Run calls in ConcurrentProducerConsumer_TransfersAllItemsInOrder

Assert.AreSequenceEqual takes IEnumerable<T>, so the accompanying .ToArray() calls became redundant and tripped IDE0305 (warnings-as-errors) — they were dropped.

Library — 9

S1939 (8) — remove interfaces already implied by another entry in the base list.

ContiguousCollection, InsertionOrderCollection, OrderedCollection drop IReadOnlyCollection<T> (implied by IReadOnlyList<T>); RingBuffer drops both IEnumerable<T> and IReadOnlyCollection<T>.

⚠️ The three Set types needed care. Sonar reports IReadOnlySet<T> implies IReadOnlyCollection<T> — true, but IReadOnlySet<T> only exists under NET5_0_OR_GREATER, and this library still targets netstandard2.0/netstandard2.1. Removing IReadOnlyCollection<T> outright would have dropped the interface from those targets — a breaking API change. It was moved into an #else branch instead:

public class OrderedSet<T> : ISet<T>
#if NET5_0_OR_GREATER
		, IReadOnlySet<T>
#else
		, IReadOnlyCollection<T>
#endif

S4136 (1)ContiguousMap.Entry: moved Equals(Entry) next to Equals(object?).

Benchmarks — 2

  • S4487 (HIGH) — removed the unread keyValuePairs field in OrderedMapBenchmarks and its [GlobalSetup] assignment.
  • S1481 — discard the intentional indexer read in CacheUsagePatternRingBuffer rather than binding an unused local.

Benchmarks excluded from analysis — retires 33 findings

Containers.Benchmarks now sets <SonarQubeExclude>true</SonarQubeExclude>. The remaining 33 open issues were all benchmark-only and all artifacts of benchmark style rather than defects:

  • S3267 (29) — "loops should be simplified using Where". Every one is inside a [Benchmark] method, and they come in paired comparisons (ContiguousSet_Contains vs HashSet_Contains vs SortedSet_Contains, …). The foreach + if + counter shape is what makes those comparisons fair; rewriting with LINQ injects delegate and iterator overhead into the measured region and invalidates the benchmark.
  • S4158 (3) — "collection is known to be empty here". Two are the Contains calls in EmptyCollectionOperations(), a benchmark that exists precisely to measure operations on empty collections. The third is a foreach over a RingBuffer immediately after PushBack; Sonar's dataflow doesn't model the mutation.
  • S2583 (1)cache.Count > 10 && random.Next(5) == 0 reported as always false. It is plainly reachable once 11 items have been pushed.

Benchmark code is also never executed by the test suite, so it can only ever report 0% coverage and drag down the new-code coverage gate. It is not shipped in the package.

Coverage

The first CI run failed the quality gate on new_coverage (0% vs 80% required) from exactly two uncovered new lines. One was benchmark code, handled by the exclusion above. The other was a genuine gap: ContiguousMap<TKey,TValue>.Entry is a public struct with a full equality surface — Equals(Entry), Equals(object?), GetHashCode(), ==, != — and had no tests at all. Added four covering equal/unequal entries, comparison against null and an unrelated type, and the Key/Value properties.

Docs

Refreshed the stale CollectionAssert.AreEqual guidance in CLAUDE.md, which now contradicted MSTEST0068.

Verification

  • dotnet build — clean, 0 errors / 0 warnings across all 8 TFMs
  • dotnet test355/355 pass (351 baseline + 4 new)

Build fixes (ktsu.Sdk 2.26.1 analyzers, were failing the build):
- KTSU0007: Polyfill PackageReference now sets PrivateAssets="all" so the
  build-time-only package stops leaking into consumers dependency graphs.
- KTSU0002: add Containers/AssemblyInfo.cs exposing internals to
  ktsu.Containers.Test.

SonarCloud fixes (46 issues):
- MSTEST0068 (34): CollectionAssert.AreEqual -> Assert.AreSequenceEqual.
  AreSequenceEqual takes IEnumerable<T>, so the accompanying .ToArray()
  calls became redundant (IDE0305) and were dropped.
- MSTEST0037 (11): Assert.AreEqual(n, x.Length) -> Assert.HasCount(n, x)
  for the span assertions, and Assert.IsTrue(a >= b) ->
  Assert.IsGreaterThanOrEqualTo(b, a) in SpscRingBufferTests.
- MSTEST0049 (1): pass TestContext.CancellationToken to the Task.Run calls
  in ConcurrentProducerConsumer_TransfersAllItemsInOrder.

Also refreshed the outdated CollectionAssert guidance in CLAUDE.md.

Build clean, 351/351 tests pass.
Library (9 issues):
- S1939 (8): drop interfaces already implied by another interface in the
  base list. ContiguousCollection/InsertionOrderCollection/
  OrderedCollection drop IReadOnlyCollection<T> (implied by
  IReadOnlyList<T>); RingBuffer drops IEnumerable<T> and
  IReadOnlyCollection<T> for the same reason.
  For the three Set types IReadOnlySet<T> only exists on
  NET5_0_OR_GREATER, and the library still targets netstandard2.0/2.1,
  so IReadOnlyCollection<T> moved into an #else branch rather than being
  removed outright - removing it would have dropped the interface from
  the lower targets.
- S4136 (1): ContiguousMap.Entry - move Equals(Entry) next to
  Equals(object?) so the overloads are adjacent.

Benchmarks (2 issues):
- S4487 (1, HIGH): remove the unread keyValuePairs field in
  OrderedMapBenchmarks and its GlobalSetup assignment.
- S1481 (1): discard the intentional indexer read in
  CacheUsagePatternRingBuffer instead of binding an unused local.

Build clean on all 8 TFMs, 351/351 tests pass.
…m Sonar

The SonarCloud quality gate failed on new_coverage (0% vs 80% required),
from exactly two uncovered new lines:

- Containers/ContiguousMap.cs - ContiguousMap<TKey,TValue>.Entry is a
  public struct with a full equality surface (Equals(Entry),
  Equals(object?), GetHashCode, == and !=) and had no tests at all.
  Added four tests covering equal/unequal entries, comparison against
  null and an unrelated type, and the Key/Value properties.

- Containers.Benchmarks - benchmark code is never executed by the test
  suite, so it can only ever report 0% coverage. Set SonarQubeExclude on
  the benchmark project.

Excluding the benchmark project also retires the 33 benchmark-only
findings that are artifacts of benchmark style rather than defects:
29x S3267 (identical foreach/counter loops kept uniform across competing
collections so the comparisons stay fair), 3x S4158 (operations on
intentionally-empty collections in EmptyCollectionOperations, plus a
foreach immediately after PushBack) and 1x S2583.

Build clean on all 8 TFMs, 355/355 tests pass.
@sonarqubecloud

Copy link
Copy Markdown

@matt-edmondson
matt-edmondson merged commit 6aa9ccd into main Aug 14, 2026
5 checks passed
@matt-edmondson
matt-edmondson deleted the chore/sonarcloud-cleanup branch August 14, 2026 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant