Repository navigation
CI: split Core tests into 3 processes, drop wasm workload and NuGet cache, drop push-to-main - #673
Conversation
…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
|
Review of CI changes: Medium: README badge Medium: nothing validates merged Low: Low:
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
| <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> |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
|
Review: CI-only change, no app code touched. Two comments inline: the README badge points at |
Claude posting for Erik Darling.
Four free CI speedups, plus the push-to-main removal Erik added. Draft, not merged, base
dev.What changed
build-and-testjob. New.github/scripts/run-core-tests-parallel.sh;ci.yml"Run tests" is nowbash .github/scripts/run-core-tests-parallel.sh. Each process runsdotnet 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-classfor 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 inci.yml. Single job kept, so the required check namebuild-and-testis unchanged.Install WASM workloadfromci.yml.ci.yml(cache: trueandcache-dependency-path;dotnet-versionkept).Install WASM workloadfromdependency-submission.yml.deploy-web.ymluntouched.push: branches: [main]trigger fromci.yml. Onlypull_requestremains; the explanatory comment still reads correctly and is unchanged.release.ymlandnightly.ymluntouched.&branch=devto the shields.io URL at README.md:10, since the default branch will stop getting CI runs. Checked with curl: the URL withbranch=devreturns a real status (<title>CI: PASSING</title>), so it stays.Local proof
--filter-class "PlanViewer.Core.Tests.A*"is a prefix match (93 tests in A-classes alone). A multi-letter OR of--filter-classand a multi-pattern--filter-not-classboth work.::error::per failed process and exited 1.git ls-files --eolshows the script isi/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)
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.
detectin Dependency submission (workflow_dispatch on this branch, run 41318216780 vs the latest dev push run 40886224644, commit fb2adb1):detectjob wallThe 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-snapshotartifact from the branch dispatch against the one from dev run 40886224644: both 8 manifests, 529 entries, 342 distinct packages. One difference, insrc/PlanViewer.Web/PlanViewer.Web.csproj:pkg:nuget/Microsoft.NET.Sdk.WebAssembly.Pack@10.0.13pkg:nuget/Microsoft.NET.Sdk.WebAssembly.Pack@10.0.14No 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 whatdeploy-web.ymlships (which still installs the workload), revert thedependency-submission.ymlhunk; it is self-contained.Considered, not added
--filter-not-classof everything in 1 and 2.Notes
Considered, not added
branch=devreturnsCI: PASSINGfrompull_requestruns whose head branch isdev. It updates on each dev-to-main PR, the same release cadence as the old badge onmain. CI never ran on pushes todev.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