perf: eliminate redundant builds in CI pipeline#5405
Conversation
- Add --no-build to dotnet publish AOT steps (managed code already built) - Add --no-build to dotnet pack in PackTUnitFilesModule (15 projects) - Add --no-build to execute-pipeline action (TUnit.Pipeline already built) - Move mock tests AOT publish to YAML (parallel, before pipeline starts) - Rename PublishAndRunMockTestsAOTModule to RunMockTestsAOTModule
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
Code Review
This is a well-focused performance improvement to the CI pipeline. The changes are logically sound and the PR description maps clearly to each diff hunk. Here are my findings:
What works well
- The
--no-buildadditions todotnet publish/dotnet pack/dotnet runare correct given that the precedingBuildstep already compiles the solution. This avoids redundant MSBuild invocations and is a legitimate speed-up. - Renaming
PublishAndRunMockTestsAOTModule→RunMockTestsAOTModuleaccurately reflects the reduced responsibility of the class (it only runs, no longer publishes). - Deleting the C# publish logic in favour of a YAML
forloop with background jobs (&/wait) is a net simplification — the publish step has no pipeline-module dependencies, so there's no value in keeping it inside the ModularPipelines graph.
Concerns
1. Silent failure in the parallel publish loop
- name: Publish Mock Tests AOT
if: matrix.os == 'ubuntu-latest'
shell: bash
run: |
for fw in net8.0 net9.0 net10.0; do
dotnet publish TUnit.Mocks.Tests/TUnit.Mocks.Tests.csproj -c Release --no-build \
--use-current-runtime -p:Aot=true -o "MOCKTESTS_AOT_${fw}" --framework "${fw}" &
done
waitwait with no arguments waits for all background jobs but returns exit code 0 even if any of the child processes failed on Bash versions prior to 4.1 (and certain edge cases on newer versions). The standard pattern to propagate failures is:
pids=()
for fw in net8.0 net9.0 net10.0; do
dotnet publish ... --framework "${fw}" &
pids+=($!)
done
fail=0
for pid in "${pids[@]}"; do
wait "$pid" || fail=1
done
exit $failOr, simpler: set set -e (already implicit in GitHub Actions bash shell with set -eo pipefail) and use wait -n (Bash 4.3+) or capture exit codes manually. Without this fix, an AOT publish failure for one framework will not fail the CI step, yet the subsequent RunMockTestsAOTModule will try to run a non-existent binary and produce a confusing error at a later stage.
2. RunMockTestsAOTModule runs frameworks sequentially
The old PublishAndRunMockTestsAOTModule also ran frameworks sequentially, so this isn't a regression, but it's worth flagging now that the publish step has been parallelised. The three SubModule calls are awaited one at a time inside a foreach. Since each framework's binary is independent, these could run in parallel using Task.WhenAll to keep them consistent with the parallelised publish step:
var tasks = new[] { "net8.0", "net9.0", "net10.0" }.Select(framework =>
context.SubModule<CommandResult>($"Run-AOT-{framework}", () =>
context.Shell.Bash.Command(
new BashCommandOptions($"DISABLE_GITHUB_REPORTER=true \"{Path.Combine(rootDir, $"MOCKTESTS_AOT_{framework}", "TUnit.Mocks.Tests")}\""),
cancellationToken)));
var results = await Task.WhenAll(tasks);
return results;This is a suggestion rather than a blocker — sequential execution is safe and may be preferred for log readability.
3. Hard-coded executable name assumes Linux
var exePath = Path.Combine(rootDir, $"MOCKTESTS_AOT_{framework}", "TUnit.Mocks.Tests");The ModuleConfiguration skips execution on non-Linux platforms, so the Linux-only name is never wrong in practice. However, the Configure() guard is now the only thing preventing a crash on Windows (no .exe extension). The old code explicitly chose the filename based on RuntimeInformation.IsOSPlatform(OSPlatform.Windows), which was self-documenting. A brief comment explaining the assumption would help future readers:
// Guard in Configure() ensures this only runs on Linux, so no .exe suffix needed.
var exePath = Path.Combine(rootDir, $"MOCKTESTS_AOT_{framework}", "TUnit.Mocks.Tests");4. --no-build on TUnit.TestProject AOT publish spans a version property mismatch risk
dotnet publish TUnit.TestProject/TUnit.TestProject.csproj -c Release --no-build ...
"-p:Version=${{ steps.gitversion.outputs.semVer }}"
...With --no-build, the version properties passed here are not baked into the already-built assembly — the assembly was compiled in the Build step without those -p: overrides (the Build step does pass the same version flags, so this is fine). Just confirming: since the Build step already stamps the version into the binary, passing them again to dotnet publish --no-build is harmless but redundant. This isn't a bug, but it's worth noting for future cleanup.
Summary
The overall approach is correct and the changes are a meaningful improvement. The one item worth fixing before merge is concern #1 (silent failure on parallel publish). The rest are suggestions.
AOT publish uses --use-current-runtime which requires RID-specific assets that aren't produced by the regular solution build.
There was a problem hiding this comment.
Follow-up review (after latest commits)
The third commit (d0385124) removes --no-build from the AOT publish steps, correctly recognising that --use-current-runtime requires a RID-specific build that the general solution Build step does not produce. That is the right fix and addresses the concern about publish correctness.
One open issue: wait still does not propagate child failures
The parallel publish loop in dotnet.yml still has the same pattern:
- name: Publish Mock Tests AOT
if: matrix.os == 'ubuntu-latest'
shell: bash
run: |
for fw in net8.0 net9.0 net10.0; do
dotnet publish TUnit.Mocks.Tests/TUnit.Mocks.Tests.csproj -c Release \
--use-current-runtime -p:Aot=true -o "MOCKTESTS_AOT_${fw}" --framework "${fw}" &
done
waitGitHub Actions sets set -eo pipefail on bash shells, but that flag only applies to the foreground shell. Background jobs (&) run in a subshell where the pipefail flag is not inherited, and wait (with no PID argument) returns exit code 0 regardless of what the background jobs returned, on Bash versions before the fix in Bash 5.1+ (and even on newer versions this is environment-dependent). In practice this means a failed AOT publish for one framework silently passes this step, and the failure surfaces later as a confusing "binary not found" error in RunMockTestsAOTModule.
The safe, portable fix is to capture PIDs and wait on them individually:
run: |
pids=()
for fw in net8.0 net9.0 net10.0; do
dotnet publish TUnit.Mocks.Tests/TUnit.Mocks.Tests.csproj -c Release \
--use-current-runtime -p:Aot=true -o "MOCKTESTS_AOT_${fw}" --framework "${fw}" &
pids+=($!)
done
exit_code=0
for pid in "${pids[@]}"; do
wait "$pid" || exit_code=$?
done
exit $exit_codewait <pid> returns the exit code of that specific background process, making failure detection reliable across all Bash versions present in GitHub-hosted runners.
Everything else looks good
NoBuild = trueondotnet packinPackTUnitFilesModuleis correct — the solution is already built before packing.--no-buildondotnet runinexecute-pipeline/action.ymlis correct —TUnit.Pipelineis compiled as part of the solution build.- The rename from
PublishAndRunMockTestsAOTModuletoRunMockTestsAOTModuleis accurate and clean. - The
[DependsOn<RunMockTestsModule>]dependency is preserved, keeping ordering correct.
Updated [TUnit.Core](https://github.com/thomhurst/TUnit) from 1.23.7 to 1.28.7. <details> <summary>Release notes</summary> _Sourced from [TUnit.Core's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.28.7 <!-- Release notes generated using configuration in .github/release.yml at v1.28.7 --> ## What's Changed ### Other Changes * fix: prevent StringBuilder race in console interceptor during parallel tests by @thomhurst in thomhurst/TUnit#5414 ### Dependencies * chore(deps): update tunit to 1.28.5 by @thomhurst in thomhurst/TUnit#5415 **Full Changelog**: thomhurst/TUnit@v1.28.5...v1.28.7 ## 1.28.5 <!-- Release notes generated using configuration in .github/release.yml at v1.28.5 --> ## What's Changed ### Other Changes * perf: eliminate redundant builds in CI pipeline by @thomhurst in thomhurst/TUnit#5405 * perf: eliminate store.ToArray() allocation on mock behavior execution hot path by @thomhurst in thomhurst/TUnit#5409 * fix: omit non-class/struct constraints on explicit interface mock implementations by @thomhurst in thomhurst/TUnit#5413 ### Dependencies * chore(deps): update tunit to 1.28.0 by @thomhurst in thomhurst/TUnit#5406 **Full Changelog**: thomhurst/TUnit@v1.28.0...v1.28.5 ## 1.28.0 <!-- Release notes generated using configuration in .github/release.yml at v1.28.0 --> ## What's Changed ### Other Changes * fix: resolve build warnings in solution by @thomhurst in thomhurst/TUnit#5386 * Perf: Optimize MockEngine hot paths (~30-42% faster) by @thomhurst in thomhurst/TUnit#5391 * Move Playwright install into pipeline module by @thomhurst in thomhurst/TUnit#5390 * perf: optimize solution build performance by @thomhurst in thomhurst/TUnit#5393 * perf: defer per-class JIT via lazy test registration + parallel resolution by @thomhurst in thomhurst/TUnit#5395 * Perf: Generate typed HandleCall<T1,...> overloads to eliminate argument boxing by @thomhurst in thomhurst/TUnit#5399 * perf: filter generated attributes to TUnit-related types only by @thomhurst in thomhurst/TUnit#5402 * fix: generate valid mock class names for generic interfaces with non-built-in type args by @thomhurst in thomhurst/TUnit#5404 ### Dependencies * chore(deps): update tunit to 1.27.0 by @thomhurst in thomhurst/TUnit#5392 * chore(deps): update dependency path-to-regexp to v8 by @thomhurst in thomhurst/TUnit#5378 **Full Changelog**: thomhurst/TUnit@v1.27.0...v1.28.0 ## 1.27.0 <!-- Release notes generated using configuration in .github/release.yml at v1.27.0 --> ## What's Changed ### Other Changes * Fix Dependabot security vulnerabilities in docs site by @thomhurst in thomhurst/TUnit#5372 * fix: use 0.0.0-scrubbed sentinel version in snapshot scrubber to avoid false Dependabot alerts by @thomhurst in thomhurst/TUnit#5374 * Speed up Engine.Tests by removing ProcessorCount parallelism cap by @thomhurst in thomhurst/TUnit#5379 * ci: add concurrency groups to cancel redundant workflow runs by @thomhurst in thomhurst/TUnit#5373 * Add scope-aware initialization and disposal OpenTelemetry spans to trace timeline and HTML report by @Copilot in thomhurst/TUnit#5339 * Add WithInnerExceptions() for fluent AggregateException assertion chaining by @thomhurst in thomhurst/TUnit#5380 * Drop net6.0 and net7.0 TFMs, keep net8.0+ and netstandard2.x by @thomhurst in thomhurst/TUnit#5387 * Remove all [Obsolete] members and migrate callers by @thomhurst in thomhurst/TUnit#5384 * Add AssertionResult.Failed overload that accepts an Exception by @thomhurst in thomhurst/TUnit#5388 ### Dependencies * chore(deps): update dependency mockolate to 2.3.0 by @thomhurst in thomhurst/TUnit#5370 * chore(deps): update tunit to 1.25.0 by @thomhurst in thomhurst/TUnit#5371 * chore(deps): update dependency minimatch to v9.0.9 by @thomhurst in thomhurst/TUnit#5375 * chore(deps): update dependency path-to-regexp to v0.2.5 by @thomhurst in thomhurst/TUnit#5376 * chore(deps): update dependency minimatch to v10 by @thomhurst in thomhurst/TUnit#5377 * chore(deps): update dependency picomatch to v4 by @thomhurst in thomhurst/TUnit#5382 * chore(deps): update dependency svgo to v4 by @thomhurst in thomhurst/TUnit#5383 * chore(deps): update dependency path-to-regexp to v1 [security] by @thomhurst in thomhurst/TUnit#5385 **Full Changelog**: thomhurst/TUnit@v1.25.0...v1.27.0 ## 1.25.0 <!-- Release notes generated using configuration in .github/release.yml at v1.25.0 --> ## What's Changed ### Other Changes * Fix missing `default` constraint on explicit interface implementations with unconstrained generics by @thomhurst in thomhurst/TUnit#5363 * feat(mocks): add ReturnsAsync typed factory overload with method parameters by @thomhurst in thomhurst/TUnit#5367 * Fix Arg.IsNull<T> and Arg.IsNotNull<T> to support nullable value types by @thomhurst in thomhurst/TUnit#5366 * refactor(mocks): use file-scoped types for generated implementation details by @thomhurst in thomhurst/TUnit#5369 * Compress HTML report JSON data and minify CSS by @thomhurst in thomhurst/TUnit#5368 ### Dependencies * chore(deps): update tunit to 1.24.31 by @thomhurst in thomhurst/TUnit#5356 * chore(deps): update dependency mockolate to 2.2.0 by @thomhurst in thomhurst/TUnit#5357 * chore(deps): update dependency polyfill to 9.24.1 by @thomhurst in thomhurst/TUnit#5365 * chore(deps): update dependency polyfill to 9.24.1 by @thomhurst in thomhurst/TUnit#5364 **Full Changelog**: thomhurst/TUnit@v1.24.31...v1.25.0 ## 1.24.31 <!-- Release notes generated using configuration in .github/release.yml at v1.24.31 --> ## What's Changed ### Other Changes * Fix Aspire 13.2.0+ timeout caused by ProjectRebuilderResource being awaited by @Copilot in thomhurst/TUnit#5335 * chore(deps): update dependency polyfill to 9.24.0 by @thomhurst in thomhurst/TUnit#5349 * Fix nullable IParsable type recognition in source generator and analyzer by @Copilot in thomhurst/TUnit#5354 * fix: resolve race condition in HookExecutionOrderTests by @thomhurst in thomhurst/TUnit#5355 * Fix MaxExternalSpansPerTest cap bypass when Activity.Parent chain is broken by @Copilot in thomhurst/TUnit#5352 ### Dependencies * chore(deps): update tunit to 1.24.18 by @thomhurst in thomhurst/TUnit#5340 * chore(deps): update dependency stackexchange.redis to 2.12.14 by @thomhurst in thomhurst/TUnit#5343 * chore(deps): update verify to 31.15.0 by @thomhurst in thomhurst/TUnit#5346 * chore(deps): update dependency polyfill to 9.24.0 by @thomhurst in thomhurst/TUnit#5348 **Full Changelog**: thomhurst/TUnit@v1.24.18...v1.24.31 ## 1.24.18 <!-- Release notes generated using configuration in .github/release.yml at v1.24.18 --> ## What's Changed ### Other Changes * feat(mocks): shorter, more readable generated mock type names by @thomhurst in thomhurst/TUnit#5334 * Fix DisposeAsync() ordering for nested property injection by @Copilot in thomhurst/TUnit#5337 ### Dependencies * chore(deps): update tunit to 1.24.13 by @thomhurst in thomhurst/TUnit#5331 **Full Changelog**: thomhurst/TUnit@v1.24.13...v1.24.18 ## 1.24.13 <!-- Release notes generated using configuration in .github/release.yml at v1.24.13 --> ## What's Changed ### Other Changes * perf(mocks): optimize MockEngine for lower allocation and faster verification by @thomhurst in thomhurst/TUnit#5319 * Remove defunct `UseTestingPlatformProtocol` reference for vscode by @erwinkramer in thomhurst/TUnit#5328 * perf(aspnetcore): prevent thread pool starvation during parallel WebApplicationTest server init by @thomhurst in thomhurst/TUnit#5329 * fix TUnit0073 for when type from from another assembly by @SimonCropp in thomhurst/TUnit#5322 * Fix implicit conversion operators bypassed in property injection casts by @Copilot in thomhurst/TUnit#5317 * fix(mocks): skip non-virtual 'new' methods when discovering mockable members by @thomhurst in thomhurst/TUnit#5330 * feat(mocks): IFoo.Mock() discovery with generic fallback and ORP resolution by @thomhurst in thomhurst/TUnit#5327 ### Dependencies * chore(deps): update tunit to 1.24.0 by @thomhurst in thomhurst/TUnit#5315 * chore(deps): update aspire to 13.2.1 by @thomhurst in thomhurst/TUnit#5323 * chore(deps): update verify to 31.14.0 by @thomhurst in thomhurst/TUnit#5325 ## New Contributors * @erwinkramer made their first contribution in thomhurst/TUnit#5328 **Full Changelog**: thomhurst/TUnit@v1.24.0...v1.24.13 ## 1.24.0 <!-- Release notes generated using configuration in .github/release.yml at v1.24.0 --> ## What's Changed ### Other Changes * perf: optimize TUnit.Mocks hot paths by @thomhurst in thomhurst/TUnit#5304 * fix: resolve System.Memory version conflict on .NET Framework (net462) by @thomhurst in thomhurst/TUnit#5303 * fix: resolve CS0460/CS0122/CS0115 when mocking concrete classes from external assemblies by @thomhurst in thomhurst/TUnit#5310 * feat(mocks): parameterless Returns() and ReturnsAsync() for async methods by @thomhurst in thomhurst/TUnit#5309 * Fix typo in NUnit manual migration guide by @aa-ko in thomhurst/TUnit#5312 * refactor(mocks): unify Mock.Of<T>() and Mock.OfPartial<T>() into single API by @thomhurst in thomhurst/TUnit#5311 * refactor(mocks): clean up Mock API surface by @thomhurst in thomhurst/TUnit#5314 * refactor(mocks): remove generic/untyped overloads from public API by @thomhurst in thomhurst/TUnit#5313 ### Dependencies * chore(deps): update tunit to 1.23.7 by @thomhurst in thomhurst/TUnit#5305 * chore(deps): update dependency mockolate to 2.1.1 by @thomhurst in thomhurst/TUnit#5307 ## New Contributors * @aa-ko made their first contribution in thomhurst/TUnit#5312 **Full Changelog**: thomhurst/TUnit@v1.23.7...v1.24.0 Commits viewable in [compare view](thomhurst/TUnit@v1.23.7...v1.28.7). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
--no-buildto all AOT publish steps in YAML — managed code is already built by the Build step, so only native ILC compilation runs--no-buildtodotnet packinPackTUnitFilesModule— avoids rebuilding all 15 packable projects during pack--no-buildto theexecute-pipelineaction —TUnit.Pipelineis already built as part of the solutionPublishAndRunMockTestsAOTModule→RunMockTestsAOTModule(only runs pre-published binaries now)Test plan