Skip to content

Flakey test fixes - #5520

Merged
redmushie merged 6 commits into
ss14Starlight:starlight-devfrom
Mackamuir:flake-test-fixes
Aug 5, 2026
Merged

Flakey test fixes#5520
redmushie merged 6 commits into
ss14Starlight:starlight-devfrom
Mackamuir:flake-test-fixes

Conversation

@Mackamuir

@Mackamuir Mackamuir commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Short description

Fixes a few tests

Why we need to add this

Dungeons needed to be updated to allow for non-standard sizes without causing errors. SovietDungeonWeh has a 17x9 room and a 17x5 room that was causing errors.

AntagGhostRoles Never removed its maps, causing massive memory usage, same with PostMapInit.

CharacterSelectionTest.cs created tests with the same names.

Doing damage to a body while it's being deleted causes an error

Checks

  • I do not require assistance to complete the PR.
  • Before posting/requesting review of a PR, I have verified that the changes work.
  • I have added screenshots/videos of the changes, or this PR does not change in-game mechanics.
  • I affirm that my changes are licensed under the MIT License and grant permission for use in this repository under its conditions.

@Mackamuir
Mackamuir requested a review from a team August 5, 2026 02:38
@github-actions github-actions Bot added S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. Changes: C# size/S S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. labels Aug 5, 2026
Updated summary comment to clarify the purpose of the MapIds method.
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Dungeon generation

Layer / File(s) Summary
Room pack prototype filtering
Content.Server/Procedural/DungeonJob/DungeonJob.DunGenPrefab.cs
Normal and rotated room-pack candidates now require matching room prototypes in at least one orientation for every room size.

Integration test maintenance

Layer / File(s) Summary
Temporary map lifecycle cleanup
Content.IntegrationTests/Tests/GameRules/AntagGhostRoleTest.cs, Content.IntegrationTests/Tests/PostMapInitTest.cs
Ghost-role tests track and delete newly loaded maps. NoSavedPostMapInitTest deletes its temporary map. Obsolete configuration and imports were removed.
Character-order test labels
Content.IntegrationTests/Tests/_Starlight/Round/CharacterSelectionTest.cs
Character selection cases now include descriptive labels for reversed and rotated permutations.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: redmushie

🚥 Pre-merge checks | ✅ 7 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (7 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No Magic Numbers ✅ Passed The PR adds no numeric literals to the four changed C# files; new logic uses named properties, existing counts, and descriptive variant labels.
No Hardcoded Ecs Parameters ✅ Passed The PR adds no ECS system or component parameters. DungeonJob uses DataField-backed room prototypes; other changes only clean maps and label tests.
Avoid Service Locator ✅ Passed The PR adds SharedMapSystem through [SidedDependency] and uses existing explicit locals; no new service-locator resolution appears in the diff. Good, focused dependency handling.
Title check ✅ Passed The title accurately identifies the primary purpose: fixing flaky tests.
Description check ✅ Passed The description clearly relates the test cleanup, duplicate-name fixes, and dungeon handling changes to the changeset.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Content.IntegrationTests/Tests/GameRules/AntagGhostRoleTest.cs (1)

99-151: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Make temporary-map cleanup failure-safe in all three tests.

The current cleanup runs only after successful assertions. Any exception before cleanup can leave temporary maps loaded and recreate the memory-growth problem this PR addresses.

  • Content.IntegrationTests/Tests/GameRules/AntagGhostRoleTest.cs#L99-L151: move rule clearing and snapshot-based map deletion into finally.
  • Content.IntegrationTests/Tests/GameRules/AntagGhostRoleTest.cs#L164-L195: apply the same finally cleanup to the sequential test.
  • Content.IntegrationTests/Tests/PostMapInitTest.cs#L257-L272: delete the explicitly created map from finally after confirming that map creation succeeded.
🤖 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 `@Content.IntegrationTests/Tests/GameRules/AntagGhostRoleTest.cs` around lines
99 - 151, Make temporary-map cleanup failure-safe in AntagGhostRoleTest.cs lines
99-151 and 164-195 by wrapping each test body’s rule clearing and snapshot-based
map deletion in a finally block. In PostMapInitTest.cs lines 257-272, place
deletion of the explicitly created map in finally after confirming creation
succeeded; preserve the existing assertions and cleanup behavior.
🤖 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.

Outside diff comments:
In `@Content.IntegrationTests/Tests/GameRules/AntagGhostRoleTest.cs`:
- Around line 99-151: Make temporary-map cleanup failure-safe in
AntagGhostRoleTest.cs lines 99-151 and 164-195 by wrapping each test body’s rule
clearing and snapshot-based map deletion in a finally block. In
PostMapInitTest.cs lines 257-272, place deletion of the explicitly created map
in finally after confirming creation succeeded; preserve the existing assertions
and cleanup behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 9eabc4c0-34d3-43b4-8ccb-430b7b976948

📥 Commits

Reviewing files that changed from the base of the PR and between 7a295c4 and 9b0b782.

📒 Files selected for processing (4)
  • Content.IntegrationTests/Tests/GameRules/AntagGhostRoleTest.cs
  • Content.IntegrationTests/Tests/PostMapInitTest.cs
  • Content.IntegrationTests/Tests/_Starlight/Round/CharacterSelectionTest.cs
  • Content.Server/Procedural/DungeonJob/DungeonJob.DunGenPrefab.cs

coderabbitai[bot]
coderabbitai Bot previously approved these changes Aug 5, 2026
@Mackamuir Mackamuir mentioned this pull request Aug 5, 2026
4 tasks
@starlightgithub starlightgithub Bot added S: Approved Status: Reviewed and approved by at least one maintainer or dev; a PR may require another approval. and removed S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. labels Aug 5, 2026
@redmushie
redmushie added this pull request to the merge queue Aug 5, 2026
@redmushie redmushie self-assigned this Aug 5, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 5, 2026
@Mackamuir

Copy link
Copy Markdown
Contributor Author

jobweighttest also needs to be fixed again I suppose

@redmushie
redmushie added this pull request to the merge queue Aug 5, 2026
Merged via the queue into ss14Starlight:starlight-dev with commit 3f58e5d Aug 5, 2026
26 checks passed
bynddark8 pushed a commit to bynddark8/space-station-14 that referenced this pull request Aug 6, 2026
## Short description
Fixes a few tests

## Why we need to add this
Dungeons needed to be updated to allow for non-standard sizes without
causing errors. SovietDungeonWeh has a 17x9 room and a 17x5 room that
was causing errors.

AntagGhostRoles Never removed its maps, causing massive memory usage,
same with PostMapInit.

CharacterSelectionTest.cs created tests with the same names.

Doing damage to a body while it's being deleted causes an error

## Checks
<!-- check boxes for faster reviewing of your PR -->

- [X] I do not require assistance to complete the PR.
- [X] Before posting/requesting review of a PR, I have verified that the
changes work.
- [X] I have added screenshots/videos of the changes, or this PR does
not change in-game mechanics.
- [X] I affirm that my changes are licensed under the [MIT
License](https://github.com/ss14Starlight/space-station-14/blob/Starlight/LICENSE.TXT)
and grant permission for use in this repository under its conditions.
Limerent-Sun pushed a commit to Blimpuf-Station/BlimpufStation that referenced this pull request Aug 7, 2026
## Short description
Fixes a few tests

## Why we need to add this
Dungeons needed to be updated to allow for non-standard sizes without
causing errors. SovietDungeonWeh has a 17x9 room and a 17x5 room that
was causing errors.

AntagGhostRoles Never removed its maps, causing massive memory usage,
same with PostMapInit.

CharacterSelectionTest.cs created tests with the same names.

Doing damage to a body while it's being deleted causes an error

## Checks
<!-- check boxes for faster reviewing of your PR -->

- [X] I do not require assistance to complete the PR.
- [X] Before posting/requesting review of a PR, I have verified that the
changes work.
- [X] I have added screenshots/videos of the changes, or this PR does
not change in-game mechanics.
- [X] I affirm that my changes are licensed under the [MIT
License](https://github.com/ss14Starlight/space-station-14/blob/Starlight/LICENSE.TXT)
and grant permission for use in this repository under its conditions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changes: C# S: Approved Status: Reviewed and approved by at least one maintainer or dev; a PR may require another approval. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants