CI Testing Upgrade - #5430
Merged
Merged
Conversation
github-actions
Bot
added
S: Untriaged
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughPurposeUpgrade CI test execution from VSTest to Microsoft Testing Platform (MTP). Tests now run from precompiled executables with retries, hang detection, JSON discovery, CTRF reporting, and timing-based sharding. User and developer impact
Notable changes
Risk areas
Validation evidence
Guideline gaps
WalkthroughThe pull request configures executable NUnit testing with Microsoft Testing Platform extensions, replaces fixed test sharding with timing-based six-shard generation, updates CI execution and reporting, and adds workflows for pull-request reports and harvested timing baselines. ChangesCI test execution
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BuildJob
participant ShardPreparation
participant ContentTests
participant IntegrationTests
participant ReportWorkflows
BuildJob->>ShardPreparation: Build binaries and generate six shard filters
ShardPreparation->>ContentTests: Provide test binaries and filters
ShardPreparation->>IntegrationTests: Provide test binaries and shard filters
ContentTests->>ReportWorkflows: Upload CTRF results
IntegrationTests->>ReportWorkflows: Upload shard CTRF results
ReportWorkflows->>ReportWorkflows: Publish comments or promote timing baseline
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
.github/workflows/build-test-debug.yml (4)
222-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused "Download shard test results" step.
Good consolidation moving reporting into one job. However, the
/tmp/shard-nunitoutput from Lines 222-227 doesn't appear to be consumed anywhere — the "Generate CTRF test report" step (Line 240) only globs/tmp/ctrf-results/**/*.json, which is already populated by the broadertest-nunit-*download at Lines 229-234. Removing the first download step saves artifact-download time on every CI run without losing anything.🧹 Proposed removal of the unused step
- - name: Download shard test results - continue-on-error: true - uses: actions/download-artifact@v8 - with: - pattern: test-nunit-shard-* - path: /tmp/shard-nunit - - name: Download CTRF test results continue-on-error: true uses: actions/download-artifact@v8🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-test-debug.yml around lines 222 - 234, Remove the "Download shard test results" step that downloads artifacts matching the pattern test-nunit-shard-* to /tmp/shard-nunit, as this directory is not consumed by any subsequent steps in the workflow. The "Generate CTRF test report" step only uses /tmp/ctrf-results populated by the second download step, making the first download redundant and safe to delete entirely.
25-26: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout steps that don't need to push. All fiveactions/checkout@v7steps in this file use default credential persistence; none of these jobs push commits or tags, so the shared root cause is the same missing hardening flag repeated five times.
.github/workflows/build-test-debug.yml#L25-L26: addpersist-credentials: falseto thebuildjob's checkout..github/workflows/build-test-debug.yml#L69-L70: addpersist-credentials: falseto thecontent-testsjob's checkout..github/workflows/build-test-debug.yml#L124-L125: addpersist-credentials: falseto theintegration-testsjob's checkout..github/workflows/build-test-debug.yml#L182-L183: addpersist-credentials: falseto theyaml-linterjob's checkout..github/workflows/build-test-debug.yml#L219-L220: addpersist-credentials: falseto theci-successjob's checkout.🔒 Proposed fix (repeat for each site)
- name: Checkout uses: actions/checkout@v7 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-test-debug.yml around lines 25 - 26, Add persist-credentials: false to all five actions/checkout@v7 steps in the file to disable credential persistence for jobs that do not perform git push operations. In .github/workflows/build-test-debug.yml at lines 25-26 (build job), lines 69-70 (content-tests job), lines 124-125 (integration-tests job), lines 182-183 (yaml-linter job), and lines 219-220 (ci-success job), add the persist-credentials: false option to each checkout action invocation to complete the security hardening across all non-push workflow jobs.Source: Linters/SAST tools
188-193: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winExtend NuGet restore caching to the test jobs, and confirm the cache is ever populated.
This cache-restore step is a nice addition for the linter job. Two things worth checking:
content-tests(Line 84) and each of the 6integration-testsshards (Line 140) rundotnet restorewithout this cache, so the PR's own goal of not increasing total runtime despite fewer shards is only partly realized — 7 of 8 test/lint jobs still pay full restore cost.- No
actions/cache/save@v4step (orsave-alwaysinput) is visible anywhere in this file. Without a save step,actions/cache/restore@v4here will always miss and never gets warmed.Consider applying the same restore-cache pattern to
content-testsandintegration-tests, and adding a matchingactions/cache/save@v4step (orsave-always: trueon the restore step) so the cache is actually populated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-test-debug.yml around lines 188 - 193, The NuGet cache restore pattern shown for the linter job needs to be applied to the content-tests job (around line 84) and each of the integration-tests shards (around line 140) so all test runs benefit from cached packages. Additionally, add an actions/cache/save@v4 step (or set save-always: true on the restore step) after the dotnet restore runs to populate the cache, otherwise the restore-cache step will always miss and never warm the cache. Copy the Restore NuGet cache step configuration to both test job types, and ensure a matching save step exists in at least one job that runs successfully to seed the cache for subsequent runs.
46-49: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUpdate the local shard helper for the new six-shard count.
Nice shard-count update in CI.
Tools/_Starlight/prepare_test_shards.pystill callspartition_tests.py generate 8, so local shard filters will differ from CI. Change line 63 togenerate 6so local failure reproduction matches CI layout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-test-debug.yml around lines 46 - 49, Update the shard count used by prepare_test_shards.py to pass 6 to partition_tests.py generate, matching the six-shard configuration in the Discover Integration Tests workflow and keeping local shard filters consistent with CI..github/workflows/yaml-linter.yml (1)
8-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deleting this workflow instead of disabling all its triggers.
Good call retiring the standalone linter workflow now that
build-test-debug.ymlruns an equivalentyaml-linterjob. Since every trigger is commented out and there's noworkflow_call, thebuildjob here (Lines 17-42) can never execute again. Keeping a fully dormant workflow file around is a bit of a trap for future maintainers — it reads as live CI configuration. Deleting the file (or repurposing it as aworkflow_call-triggered reusable workflow, if that's the actual intent) would be clearer than a permanently-disabled trigger block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/yaml-linter.yml around lines 8 - 14, The yaml-linter workflow file has all its triggers (the on: section) commented out, making the build job unreachable and the entire workflow dormant. Since the yaml-linter functionality is now handled by an equivalent job in build-test-debug.yml, either delete the entire yaml-linter.yml file to remove dormant configuration that could confuse future maintainers, or if the workflow is intended for reuse elsewhere, repurpose it by adding a workflow_call trigger instead of the commented-out push, pull_request, and workflow_dispatch triggers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-test-debug.yml:
- Line 217: Change the runs-on label for the ci-success job from ubuntu-slim to
ubuntu-latest. This enables the use of a configurable timeout-minutes setting
instead of relying on the immutable 15-minute timeout imposed by the ubuntu-slim
runner.
- Around line 17-18: Move the workflow-level pull-requests write permission into
the ci-success job that invokes ctrf-io/github-test-reporter@v1, and remove it
from the global permissions block. Keep other jobs, including build,
content-tests, integration-tests, and yaml-linter, without that permission.
In `@Tools/_Starlight/partition_tests.py`:
- Around line 383-411: Define the missing load_timings() helper and
cmd_harvest() command implementation in Tools/_Starlight/partition_tests.py
before their existing call sites, ensuring cmd_generate() can load timing data
and the harvest command dispatch resolves without NameError. Preserve the
current timing-weighted shard behavior and align cmd_harvest() with the parser’s
harvest arguments and existing command conventions.
---
Nitpick comments:
In @.github/workflows/build-test-debug.yml:
- Around line 222-234: Remove the "Download shard test results" step that
downloads artifacts matching the pattern test-nunit-shard-* to /tmp/shard-nunit,
as this directory is not consumed by any subsequent steps in the workflow. The
"Generate CTRF test report" step only uses /tmp/ctrf-results populated by the
second download step, making the first download redundant and safe to delete
entirely.
- Around line 25-26: Add persist-credentials: false to all five
actions/checkout@v7 steps in the file to disable credential persistence for jobs
that do not perform git push operations. In
.github/workflows/build-test-debug.yml at lines 25-26 (build job), lines 69-70
(content-tests job), lines 124-125 (integration-tests job), lines 182-183
(yaml-linter job), and lines 219-220 (ci-success job), add the
persist-credentials: false option to each checkout action invocation to complete
the security hardening across all non-push workflow jobs.
- Around line 188-193: The NuGet cache restore pattern shown for the linter job
needs to be applied to the content-tests job (around line 84) and each of the
integration-tests shards (around line 140) so all test runs benefit from cached
packages. Additionally, add an actions/cache/save@v4 step (or set save-always:
true on the restore step) after the dotnet restore runs to populate the cache,
otherwise the restore-cache step will always miss and never warm the cache. Copy
the Restore NuGet cache step configuration to both test job types, and ensure a
matching save step exists in at least one job that runs successfully to seed the
cache for subsequent runs.
- Around line 46-49: Update the shard count used by prepare_test_shards.py to
pass 6 to partition_tests.py generate, matching the six-shard configuration in
the Discover Integration Tests workflow and keeping local shard filters
consistent with CI.
In @.github/workflows/yaml-linter.yml:
- Around line 8-14: The yaml-linter workflow file has all its triggers (the on:
section) commented out, making the build job unreachable and the entire workflow
dormant. Since the yaml-linter functionality is now handled by an equivalent job
in build-test-debug.yml, either delete the entire yaml-linter.yml file to remove
dormant configuration that could confuse future maintainers, or if the workflow
is intended for reuse elsewhere, repurpose it by adding a workflow_call trigger
instead of the commented-out push, pull_request, and workflow_dispatch triggers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b12fbf49-7cc0-49f2-bda0-c869011835e1
📒 Files selected for processing (13)
.config/dotnet-tools.json.github/ci.runsettings.github/workflows/build-test-debug.yml.github/workflows/yaml-linter.ymlContent.IntegrationTests/Content.IntegrationTests.csprojContent.Tests/Content.Tests.csprojDirectory.Build.propsDirectory.Build.targetsDirectory.Packages.propsResources/Prototypes/_Starlight/Entities/Structures/Piping/Plumbing/plumbing_machines.ymlTools/_Starlight/partition_tests.pyTools/_Starlight/test-timings.jsonglobal.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tools/_Starlight/partition_tests.py`:
- Around line 327-335: Validate the result of json.load in the timing-loading
flow before iterating: require data to be a dict, and return None for null,
list, scalar, or other non-object values. Keep the existing timings coercion and
filtering logic unchanged for valid dictionaries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b88c6326-ef62-4144-94ec-78c0e3643644
📒 Files selected for processing (1)
Tools/_Starlight/partition_tests.py
Contributor
Author
|
PR Comments wont work until this is on the main branch |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-test-debug.yml (1)
116-169: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep generated shard count equal to executed shard count.
The reduced CI fan-out is good.
Tools/_Starlight/prepare_test_shards.py, Line 63, still generates eight partitions. This workflow executes six shard jobs. Tests assigned to generated shards 6 and 7 will not run.Pass the same shard count to
prepare_test_shards.pyand the integration-job matrix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-test-debug.yml around lines 116 - 169, Update the shard-generation step that invokes prepare_test_shards.py to use the same six-shard count as the integration-test matrix in the workflow. Ensure the generated shard files cover exactly shards 0 through 5 so every assigned test is executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test-report-comment.yml:
- Around line 16-18: Update the report-comment workflow’s PR lookup/output flow
around the trusted PR resolution and before writing number to GITHUB_OUTPUT:
fetch the PR head SHA, compare it with HEAD_SHA, and skip the comment/report
update when they differ. Keep the existing behavior for matching SHAs, and
adjust the concurrency handling only as needed to prevent stale runs from
overwriting the latest PR report.
In `@Tools/_Starlight/partition_tests.py`:
- Around line 515-520: Update the TRX parsing used by cmd_harvest to import
ElementTree from defusedxml.ElementTree instead of xml.etree.ElementTree, while
preserving the existing ET.parse and ET.ParseError handling in the trx_files
loop.
---
Outside diff comments:
In @.github/workflows/build-test-debug.yml:
- Around line 116-169: Update the shard-generation step that invokes
prepare_test_shards.py to use the same six-shard count as the integration-test
matrix in the workflow. Ensure the generated shard files cover exactly shards 0
through 5 so every assigned test is executed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 00199084-d2cd-438e-bf31-e90231d37770
📒 Files selected for processing (3)
.github/workflows/build-test-debug.yml.github/workflows/test-report-comment.ymlTools/_Starlight/partition_tests.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Directory.Packages.props`:
- Line 37: Update the file ending after the closing </Project> element to
include a final newline, ensuring Tools/check_crlf.py and the EditorConfig
checks pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b00da235-0a54-4ddd-ac8b-4b91ca821203
📒 Files selected for processing (1)
Directory.Packages.props
Mackamuir
marked this pull request as draft
August 1, 2026 06:34
Contributor
Author
|
ok opening again I need tests to run believe it or not |
Mackamuir
marked this pull request as ready for review
August 1, 2026 06:46
Mackamuir
added
S: Draft
Contributor
Author
|
@coderabbitai review |
Contributor
|
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
Author
Mackamuir
added
S: Needs Review
Move both test projects off the VSTest host and onto Microsoft.Testing Platform, so tests build into self-hosting executables that can be invoked directly. - global.json selects the Microsoft.Testing.Platform runner. - Directory.Build.props turns on EnableNUnitRunner repo-wide. - Directory.Build.targets re-enables the NUnit runner for test projects that RobustToolbox's own Directory.Build.props shadowed, and pins the adapter reference it needs. - Content.Tests and Content.IntegrationTests become Exe, pick up the NUnit3TestAdapter 6.2.0 MTP-capable adapter, and add the HangDump, Retry and CtrfReport extensions. - .github/ci.runsettings carries the NUnit MTP settings CI runs with. - LevelOfParallelism drops to 1; integration tests were peaking well past available memory at 2. - Starlight package versions are grouped together in Directory.Packages.props to keep upstream merges clean.
Contributor
Author
|
Big ass PR, ready to review, let me know if this needs to be split up |
Contributor
Author
|
Also if a test hangs it won't retry, unfortunately, but no fixes available from what I could find |
Rinary1
approved these changes
Aug 10, 2026
starlightgithub
Bot
added
S: Approved
Rinary1
enabled auto-merge
August 10, 2026 16:56
Merged
via the queue into
ss14Starlight:starlight-dev
with commit Aug 10, 2026
d194a9d
19 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short description
Makes CI Epic.
Goals:
Changes
Testing Framework Migration
Upgrades the testing framework from using VSTest to MTP. The reasoning behind this is that MTP is just flat newer, has better modules, and integrations.
Content.Tests and Content.IntegrationTests are now built as executables, and CI runs those precompiled binaries directly instead of calling dotnet test, it improves performance a
fairbit. The build artifact now only contains the test binaries instead of the entire checkout.Changed hang detection over to MTP's HangDump extension.
Reworked test sharding to use MTP's JSON test discovery.
Removed all the rider run configuration (Sorry), But now since you can easily identify testfails we shouldn't really need it.
Test Retries
The migration to MTP also allows us to natively retry specific failed tests. Each failed test can be retried twice after the original attempt, instead of using the retry action to rerun the entire test suite/shard and adding another 15 minutes. Retry results are uploaded alongside the original CTRF reports so we don't lose the failure output, and if they are real testfails we get real failure messages instead of the ambiguous stuff we got before. Retries are disabled during timing harvests so they don't mess with the results.
Timing-Based Sharding
New timing-based sharding pipeline, to automagically create weights for all of the tests. The manually dispatched Harvest Test Timings workflow runs every integration test without retries, merges the CTRF reports, and will create the baseline. It then strips it and creates a test-timings.ctrf.json artifact, which still needs to be downloaded and committed.
New tests without timing data use the median test duration as a fallback.
Reduced the amount of CI shards from 8 > X at the cost of they take around 14~ minutes to finish, if we bump it up to like 6 runners they each finish in about X minutes per shard.
Did some tests, here is a table of timings:
Reporting
Changed the reporter to this CTRF Reporter, I like it better, again less verbose but more readable. Content and Integration tests get separate summaries and downloadable report artifacts.
The reporter also leaves one combined comment on the PR and overwrites it when new results come in, I got sick of not being able to see test results on my phone. This is handled by a separate workflow so fork PRs don't get write permissions.
Reduce the amount of output in Build-Test-Debug, way more readable now, I think the trade off is worth it. Normal run has minimal logs, debug contains a lot more. The old info was interesting but not really useful as we never used it to compare, but it is still saved in the CTRF outputs.
Workflow Restructuring
Moved Yaml Linting into Build-Test-Debug as a job that reuses the already compiled binaries. This frees up another runner that previously had to restore and build the entire project independently. The old workflow is disabled via comment.
Editorconfig linting migrated to actually use editorconfig, diffing the PR to get changed files, this will give false positives if the PR is out of date with head.
Added ReviewDog reviews to catch compiler warnings that are applicable to the PR
Performance & Memory
Reduced integration-test parallelism from 2 > 1 to stop the test processes peaking at genuinely stupid amounts of memory, this also caused no extra action time. Harvest and runner-debug runs also print memory usage while the tests are running, which should hopefully help.
Example Harvest workflow:
https://github.com/Mackamuir/space-station-14/actions/runs/31050550671
Why we need to add this
Read and weep
Tests actually show native errors
If a test fails it will only retry said test.
Media (Video/Screenshots)
Checks