Skip to content

CI: split Core tests into 3 processes, drop wasm workload and NuGet cache, drop push-to-main - #673

Merged
erikdarlingdata merged 3 commits into
devfrom
fix/ci-test-split
Nov 1, 2026
Merged

erikdarlingdata merged 3 commits into
devfrom
fix/ci-test-split

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Nov 1, 2026 •

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Four free CI speedups, plus the push-to-main removal Erik added. Draft, not merged, base dev.

What changed

  1. Core tests split into 3 processes in the one build-and-test job. New .github/scripts/run-core-tests-parallel.sh; ci.yml "Run tests" is now bash .github/scripts/run-core-tests-parallel.sh. Each process runs dotnet test ... -- --hangdump --hangdump-timeout 5m --hangdump-type none --max-threads 1 --results-directory <own dir> plus its filters. Process 1: --filter-class "PlanViewer.Core.Tests.X*" for A-N. Process 2: the same for O, P, Q, R, U, V, W, X, Y, Z. Process 3: --filter-not-class for every pattern in 1 and 2 (catch-all, so a new class, lowercase initial or new namespace cannot drop out). The script waits for all three, prints each log after it finishes (labelled, in ::group:: blocks), prints one summary line per process, and exits 1 if any process failed. The new script is added to the paths filter in ci.yml. Single job kept, so the required check name build-and-test is unchanged.
  2. Removed Install WASM workload from ci.yml.
  3. Removed the NuGet cache from ci.yml (cache: true and cache-dependency-path; dotnet-version kept).
  4. Removed Install WASM workload from dependency-submission.yml. deploy-web.yml untouched.
  5. Scope change from Erik: removed the push: branches: [main] trigger from ci.yml. Only pull_request remains; the explanatory comment still reads correctly and is unchanged. release.yml and nightly.yml untouched.
  6. README CI badge (knock-on): added &branch=dev to the shields.io URL at README.md:10, since the default branch will stop getting CI runs. Checked with curl: the URL with branch=dev returns a real status (<title>CI: PASSING</title>), so it stays.

Local proof

  • Filter semantics against the real runner: --filter-class "PlanViewer.Core.Tests.A*" is a prefix match (93 tests in A-classes alone). A multi-letter OR of --filter-class and a multi-pattern --filter-not-class both work.
  • Full suite once through the script, locally (Windows, no workload): exit 0. Process totals 516 + 659 + 332 = 1,507, which is the full suite count; 0 failed, 31 skipped (30 + 1), same as the unsplit suite.
  • Failure propagation, from a temporary uncommitted copy of the script where process 1 matched no tests: process 1 and 3 exit 8 (no tests ran), process 2 exit 0, script printed an ::error:: per failed process and exited 1.
  • git ls-files --eol shows the script is i/lf w/lf. No unintended runs of spaces in the diff (only comment alignment in the script header).

CI proof (run 41318217649, attempts 1, 2, 3 via gh run rerun)

All three attempts green: no flakes from the split, no retries, none of the two known-risk tests (GcfPipelineTests.Tool_Result_Stays_Json_When_Disabled, McpSmokeTests.GeneratedMcpServer_ExposesOnlyBoundedPlanToolsOverStdio) failed. Linux, no wasm workload: restore, build and tests pass.

Timings (job step timestamps from the jobs API)

Before (audit median, last 150 runs) After attempt 1 After attempt 2 After attempt 3
CI job wall (build-and-test) 214 s 98 s 93 s 106 s
Run tests 129 s 39 s 43 s 47 s
Install WASM workload 14 s removed removed removed
Setup .NET (incl. NuGet cache) 8 s about 1 s under 2 s under 2 s
Post Setup .NET (cache save) 11 s under 1 s under 1 s under 1 s
Restore solution 12 s 18 s 13 s 17 s
Build solution 30 s 33 s 29 s 33 s

The audit estimate was 214 s to about 130-150 s; measured 93-106 s, better than estimated, because the test step dropped from 129 s to 39-47 s (the audit's "before" figures are medians from older runs, and these three after-runs are a small sample). Restore is a few seconds slower without the cache (cache hit rate was 2 of 13), as expected. Build time is unchanged.

detect in Dependency submission (workflow_dispatch on this branch, run 41318216780 vs the latest dev push run 40886224644, commit fb2adb1):

Before After
detect job wall 467 s 153 s
Install WASM workload 253 s removed
Restore solution 68 s 60 s
Restore SSMS projects 97 s 50 s

The audit's estimate was about 200 s; measured 153 s.

Dependency snapshot comparison (Change 4)

Diffed the resolved package set (manifest + package URL) of the dependency-snapshot artifact from the branch dispatch against the one from dev run 40886224644: both 8 manifests, 529 entries, 342 distinct packages. One difference, in src/PlanViewer.Web/PlanViewer.Web.csproj:

  • branch (no workload): pkg:nuget/Microsoft.NET.Sdk.WebAssembly.Pack@10.0.13
  • dev (with workload): pkg:nuget/Microsoft.NET.Sdk.WebAssembly.Pack@10.0.14

No package name disappeared; one SDK build pack version differs. Both runs used SDK 10.0.403. My reading is that the workload install bumps the wasm manifest, which moves this pack from the SDK-bundled 10.0.13 to 10.0.14. That is a build-time SDK pack, not something referenced in Directory.Packages.props. The effect is that the snapshot reports 10.0.13 where the workload run reported 10.0.14. I kept Change 4 since nothing was lost, but if you want the snapshot to match what deploy-web.yml ships (which still installs the workload), revert the dependency-submission.yml hunk; it is self-contained.

Considered, not added

  • Retries or reruns inside the script to hide a flaky test: not added, per the brief.
  • A static-vs-computed partition check beyond the catch-all process: not needed; process 3 is --filter-not-class of everything in 1 and 2.

Notes

  • Class-initial split shares of reported test time locally: process 1 about 32 s, process 2 about 33 s, process 3 about 41 s (the process 3 wall is the critical path, so its balance could be tuned later by moving an initial between groups).
  • Two of the three after-run restore steps were slightly slower than the audit median (no NuGet cache). Net effect is still positive.

Considered, not added

  • Asserting the three processes' summed test count. Process 3 is the negation of processes 1 and 2, so a class cannot fall out of the split, and a process matching no tests exits non-zero. No concrete failure to guard against. All CI attempts reported 516 + 659 + 332 = 1,507.
  • Keeping a push trigger for the badge. branch=dev returns CI: PASSING from pull_request runs whose head branch is dev. It updates on each dev-to-main PR, the same release cadence as the old badge on main. CI never ran on pushes to dev.
  • Reverting the dependency-submission hunk over Microsoft.NET.Sdk.WebAssembly.Pack. The snapshot reports 10.0.13 and deploy-web ships 10.0.14. Kept: it is one patch version of a build-time pack, against about 310s saved per push.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M98owaifNbzAhDjG7sTKGh

erikdarlingdata and others added 2 commits November 1, 2026 09:50
…e and push-to-main trigger

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M98owaifNbzAhDjG7sTKGh
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M98owaifNbzAhDjG7sTKGh
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review November 1, 2026 14:59
@claude

claude Bot commented Nov 1, 2026

Copy link
Copy Markdown

Review of CI changes:

Medium: README badge &branch=dev won't track anything after this merges. With the push: branches: [main] trigger removed, ci.yml runs only on pull_request. Those runs have the PR head branch, not dev, so shields.io has no new run to show for dev. It shows the last pre-merge push run on dev (the 'PASSING' you checked via curl), which goes stale and stays green even if dev breaks. Either drop the badge, point it at an event that still runs, or keep a push trigger on dev and main.

Medium: nothing validates merged dev or main any more. PRs are tested against their base at PR time, but a merge-queue-less squash onto a moved base is never built or tested. Combined with the stale badge, a bad semantic merge is invisible until release.yml or nightly.yml. If that trade-off is intended, say so in the ci.yml comment. It still describes only the required-check reasoning.

Low: Microsoft.NET.Sdk.WebAssembly.Pack is 10.0.13 in the dependency snapshot but 10.0.14 in what deploy-web.yml ships. You noted this in the PR body. The snapshot now under-reports what ships, which defeats its purpose for that project. Reverting the dependency-submission.yml hunk is cheap, and the 153 s detect time isn't on any PR's critical path.

Low: run-core-tests-parallel.sh

  • grep -E 'Total:|total:' ... produces an empty summary if the runner's output format changes. The exit codes remain authoritative, so this is cosmetic.
  • If dotnet test is killed (OOM or signal), the .exit file may be missing. That is handled as 'missing' and treated as failure, which is good.
  • The A-N / O-Z / rest split is by name initial, so it will drift out of balance (process 3 is already the critical path). That's fine for now.

Nothing in the diff touches version files, NoWarn, T-SQL, or Core/Web linking.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M98owaifNbzAhDjG7sTKGh
Comment thread README.md
<a href="https://github.com/erikdarlingdata/PerformanceStudio/issues"><img src="https://img.shields.io/github/issues/erikdarlingdata/PerformanceStudio?style=for-the-badge" alt="Open Issues"></a>
<a href="https://github.com/erikdarlingdata/PerformanceStudio/commits/main"><img src="https://img.shields.io/github/last-commit/erikdarlingdata/PerformanceStudio?style=for-the-badge" alt="Last Commit"></a>
<a href="https://github.com/erikdarlingdata/PerformanceStudio/actions/workflows/ci.yml"><img src="https://img.shields.io/github/actions/workflow/status/erikdarlingdata/PerformanceStudio/ci.yml?style=for-the-badge&label=CI" alt="CI"></a>
<a href="https://github.com/erikdarlingdata/PerformanceStudio/actions/workflows/ci.yml"><img src="https://img.shields.io/github/actions/workflow/status/erikdarlingdata/PerformanceStudio/ci.yml?style=for-the-badge&label=CI&branch=dev" alt="CI"></a>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The branch=dev badge will likely never show a result. This PR removes the push trigger from ci.yml, so CI only runs on pull_request events. shields.io queries the latest run for that branch, and pull_request runs are attached to the PR merge ref, not to dev. Expect "no status" here. The old badge (default branch, main) also stops updating once push is gone. Either keep a push trigger for dev/main, or point the badge at the nightly workflow, which does test dev.

echo "::endgroup::"
if [ "$code" != "0" ]; then
echo "::error::Core tests process $name failed with exit code $code"
status=1

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low: if a filter matches zero tests in a process (for example after test classes are renamed), that process can exit non-zero. Exit code 8 is "zero tests ran" under Microsoft.Testing.Platform. That would fail CI, which is the safe direction. The reverse case is a bigger risk. The summary line only greps Total:, and nothing checks that the three processes together ran the full test count. Consider asserting the summed total against a single --list-tests count, or at least failing when any process reports 0 total. Otherwise a filter-syntax regression that silently drops tests would pass.

@claude

claude Bot commented Nov 1, 2026

Copy link
Copy Markdown

Review: CI-only change, no app code touched. Two comments inline: the README badge points at branch=dev but ci.yml no longer runs on push, so it will likely show no status; and the parallel test script has no check that the three processes together run the full test set. Removing the wasm-tools workload step looks fine, since no project in the build sets AOT or native WASM options (only deploy-web.yml still installs it). No version, NoWarn or T-SQL concerns.

@erikdarlingdata
erikdarlingdata merged commit 82130b4 into dev Nov 1, 2026
7 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/ci-test-split branch November 1, 2026 15:12
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