Matrix dotnet build

[simonmarty]

Description

Why is this change being made?

1. We do not run unit tests on Linux, only on Windows.

What is changing?

1. Matrix on OS and the two most recent .NET LTS versions.

Related Links

• Issue #, if available:

---

Testing

How was this tested?

1. Tested on fork

When testing locally, provide testing artifact(s):

1. https://github.com/simonmarty/aws-secretsmanager-caching-net/actions/runs/23774021117

---

Reviewee Checklist

Update the checklist after submitting the PR

• I have reviewed, tested and understand all changes
  If not, why:
• I have filled out the Description and Testing sections above
  If not, why:
• Build and Unit tests are passing
  If not, why:
• Unit test coverage check is passing
  If not, why:
• Integration tests pass locally
  If not, why: No integ test changes
• I have updated integration tests (if needed)
  If not, why:
• I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise)
  If not, why:
• I have added explanatory comments for complex logic, new classes/methods and new tests
  If not, why:
• I have updated README/documentation (if needed)
  If not, why:
• I have clearly called out breaking changes (if any)
  If not, why:

---

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

• Reviewee checklist has been accurately filled out
• Code changes align with stated purpose in description
• Test coverage adequately validates the changes

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.92%. Comparing base (9e32e0e) to head (2427b23).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #194   +/-   ##
=======================================
  Coverage   76.92%   76.92%           
=======================================
  Files           5        5           
  Lines         234      234           
  Branches       24       24           
=======================================
  Hits          180      180           
  Misses         46       46           
  Partials        8        8           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[secrets-manager-ai-code-review]

Claude finished @simonmarty's task in 2m 16s —— View job

---

Claude Code Review

Thanks for expanding CI coverage to Linux and multiple .NET LTS versions — this is a worthwhile improvement. Overall the change is solid; I have one correctness concern about the Windows matrix legs and a few smaller notes. I've left inline comments on the specific lines.

🔴 Windows matrix legs run redundant/identical work

The test command only appends the --framework filter when not on Windows:

${{ runner.os != 'Windows' && format('--framework net{0}.0', matrix.dotnet-version) || '' }}

On Windows this evaluates to '', so dotnet test runs all target frameworks (net8.0;net10.0;net48) on both Windows legs. Consequences:

• build (windows-latest, .NET 10) and build (windows-latest, .NET 8) execute the identical test set → one Windows leg is duplicated CI time (4 jobs, but only 3 distinct test runs).
• The job name is misleading on Windows: the .NET 8 leg actually runs net8.0 + net10.0 + net48.
• setup-dotnet only installs the single matrix.dotnet-version SDK, so the Windows .NET 8 leg running net10.0 tests depends on the .NET 10 runtime being pre-installed on the hosted image. If that ever changes, the leg breaks.

Suggestion: either apply the framework filter on Windows too (each leg tests exactly one runtime, symmetric with Linux), or drop the dotnet-version axis on Windows and run all TFMs once there. Also consider explicitly installing every runtime the test set needs rather than relying on the image's pre-installed set. (Inline comment on line 43.)

🟡 msbuild-architecture: ${{ runner.arch }}

runner.arch yields uppercase (X64), while the previous literal was x64 and setup-msbuild documents lowercase inputs. Worth confirming v3 handles the case; since these runners are always x64, a plain x64 literal is safer. (Inline comment on line 35.)

🟢 Minor / nits

• net48 on Linux: The test csproj still targets net48. On Linux dotnet test --framework net10.0 correctly avoids it, so this is fine — just calling out that net48 is Windows-only and the current filtering handles it.
• Missing space in name: — build (${{ matrix.runner}}, ...) (line 15) is missing a space before the closing braces after matrix.runner. Purely cosmetic.
• Action version bumps (checkout v6, setup-dotnet v5, setup-msbuild v3, codecov v6) look reasonable and are appropriately paired with the matrix change.
• IntegTests csproj adding net10.0 is consistent with the unit test project. 👍

✅ Things done well

• fail-fast: false is the right call for a coverage matrix.
• Guarding the MSBuild install with if: runner.os == 'Windows' is correct (net48 can't build on Linux).
• Codecov coverage unchanged (73.18%), so no test regressions.

None of these are blocking except the Windows redundancy, which is more of an efficiency/correctness-of-intent issue than a failure. Nice work overall.

[secrets-manager-ai-code-review]

The --framework filter is only applied on non-Windows, so the two Windows matrix legs do redundant work.

On Windows the conditional evaluates to '', so dotnet test runs all target frameworks (net8.0;net10.0;net48) regardless of matrix.dotnet-version. That means:

• build (windows-latest, .NET 10) and build (windows-latest, .NET 8) execute the identical test set (all three TFMs). One of the two Windows legs is pure duplicated CI time.
• The job name is misleading on Windows — .NET 8 actually runs net8.0 and net10.0 and net48.
• Running net10.0 tests under the .NET 8 leg relies on the .NET 10 runtime being pre-installed on the GitHub image, since setup-dotnet only installs the single matrix.dotnet-version SDK. If the hosted image ever drops that runtime, the .NET 8 Windows leg breaks.

Consider either applying the framework filter on Windows too (so each leg tests exactly one runtime, matching the Linux behavior), or dropping the dotnet-version axis for Windows and testing all TFMs in a single Windows job. Also worth installing all runtimes the test set needs rather than depending on the image's pre-installed set — e.g.:

dotnet-version: |
  8
  10

[secrets-manager-ai-code-review]

runner.arch returns uppercase values (X86, X64, ARM, ARM64), whereas the previous hardcoded value was lowercase x64 and setup-msbuild's documented msbuild-architecture inputs are x86 / x64 / arm64. Please confirm v3 accepts X64 case-insensitively; if not, this silently mis-selects the architecture. Since the Windows runners here are always x64, a simple lowercase literal (x64) is safer than deriving from runner.arch.

[secrets-manager-ai-code-review]

Claude finished @simonmarty's task in 1m 35s —— View job

---

Claude Code Review

• Read PR diff against origin/master
• Review current workflow + csproj changes
• Post review feedback

Re-reviewed the latest state of the branch. Both issues from the previous review round have been addressed — nice work. Details below.

✅ Previously-flagged issues, now resolved

1. Windows redundant/identical test runs — Fixed. The matrix now uses an explicit include: list with a framework per leg, and the test command applies --framework ${{ matrix.framework }} on every OS (no more Windows-only conditional). Each leg now tests exactly one TFM:

   • ubuntu-latest → net8.0, net10.0
   • windows-latest → net8.0, net10.0, net48

   The job name (build (${{ matrix.runner }}, ${{ matrix.framework }})) is now accurate per leg and no longer has the missing-space cosmetic issue.

2. msbuild-architecture: ${{ runner.arch }} — Fixed. Reverted to the literal x64, avoiding the uppercase/case-sensitivity concern entirely.

3. Runtime availability — setup-dotnet@v5 now installs both 8 and 10 explicitly, so the test set no longer depends on whichever runtimes happen to be pre-installed on the hosted image.

🟢 Minor notes (non-blocking)

• dotnet build/dotnet restore still build all TFMs on Linux, including net48 (only dotnet test is filtered by --framework). This works because the .NET SDK implicitly pulls in the .NET Framework reference assemblies, so net48 compiles fine on Linux without MSBuild.exe — and your fork run confirms it passes. Just noting it's building net48 on the Linux legs even though it's never tested there; harmless, and it does give you build-time coverage of the net48 TFM on Linux.
• Codecov (v6, use_oidc: true) — each leg uploads its own TestResults directory, which is the intended behavior for a coverage matrix. fail-fast: false is the right call so one framework's failure doesn't mask the others.
• IntegTests + UnitTests csproj both adding net10.0 are consistent with each other. 👍

Summary

No blocking issues remain. The change cleanly expands CI to Linux and the two most recent .NET LTS versions, each leg does exactly the distinct work its name advertises, and the earlier correctness concerns are gone. LGTM.