refactor(ships): extract TestHelpers to different namespaces, remove reflection, add proper ship factories#70
Merged
Conversation
…reflection, add proper ship factories
📝 WalkthroughWalkthroughThis PR consolidates test infrastructure by introducing a centralized, fluent ChangesTest Infrastructure Consolidation and Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
Assets/Scripts/Ships/Tests/ModuleJointLifecycleTests.cs (1)
48-52: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove debug logging artifacts.
Non-descriptive debug logs ("mkay0", "mkay", "mkay2") appear to be leftover debugging artifacts that should be removed before merging.
🧹 Proposed cleanup
- Debug.Log("mkay0"); Object.Destroy(other.gameObject); - Debug.Log("mkay"); yield return WaitForLifecycle(); - Debug.Log("mkay2");🤖 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 `@Assets/Scripts/Ships/Tests/ModuleJointLifecycleTests.cs` around lines 48 - 52, In ModuleJointLifecycleTests remove the non-descriptive Debug.Log calls ("mkay0", "mkay", "mkay2") that were left in the test; locate the test method in the ModuleJointLifecycleTests class where Object.Destroy(other.gameObject) and yield return WaitForLifecycle() are used and delete those Debug.Log statements so the test output is clean and only contains meaningful logs or assertions.Assets/Scripts/Ships/Tests/ShipDisconnectionTests.cs (1)
137-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing assertion makes test ineffective.
The test computes a boolean expression (lines 137-139) but discards the result with
_and never asserts on it. Lines 142-147 only log the outcome. Without an assertion, this test cannot fail and provides no verification of the alternate-path behavior.🔍 Proposed fix to add missing assertion
// Assert: If alternate path exists through C, moduleA should stay connected // This depends on the actual topology created - verify the graph state - _ = ship.ModuleGraph.ContainsNode(moduleA) && - ship.ModuleGraph.ContainsNode(moduleB) && - ship.ModuleGraph.ContainsNode(moduleC); + var aConnected = ship.ModuleGraph.ContainsNode(moduleA); + var bConnected = ship.ModuleGraph.ContainsNode(moduleB); + var cConnected = ship.ModuleGraph.ContainsNode(moduleC); // Log the result for diagnostic purposes - Debug.Log($"After removing A-Command connection: A={ship.ModuleGraph.ContainsNode(moduleA)}, " + - $"B={ship.ModuleGraph.ContainsNode(moduleB)}, C={ship.ModuleGraph.ContainsNode(moduleC)}"); + Debug.Log($"After removing A-Command connection: A={aConnected}, B={bConnected}, C={cConnected}"); - // The actual assertion depends on whether an alternate path exists - // If C connects A to B and B connects to Command, A should stay + // All modules should remain connected via alternate paths + Assert.IsTrue(aConnected, "Module A should stay connected via alternate path through C"); + Assert.IsTrue(bConnected, "Module B should stay connected to Command"); + Assert.IsTrue(cConnected, "Module C should stay connected");🤖 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 `@Assets/Scripts/Ships/Tests/ShipDisconnectionTests.cs` around lines 137 - 147, Replace the discarded boolean with an actual assertion: instead of "_ = ship.ModuleGraph.ContainsNode(moduleA) && ...", call an NUnit/Unity assertion such as Assert.IsTrue(ship.ModuleGraph.ContainsNode(moduleA) && ship.ModuleGraph.ContainsNode(moduleB) && ship.ModuleGraph.ContainsNode(moduleC), "After removing A-Command connection expected nodes A, B, C to be present (alternate-path behavior)"); reference the same symbols ship.ModuleGraph.ContainsNode, moduleA, moduleB, moduleC and keep or adjust the Debug.Log message for diagnostics as needed.
🧹 Nitpick comments (3)
Assets/Scripts/Ships/Tests/TestHelpers/Factories/ShipTestBuilder.cs (1)
91-99: ⚡ Quick winFragile child-index assumption in engine retrieval.
Line 96 retrieves the engine by assuming it's the last child added (
GetChild(childCount - 1)). This works currently becauseCreateEngineModulewas just called, but breaks if the creation order changes or if methods are composed differently.Consider having
CreateEngineModulereturn the Engine component so it can be captured directly, or store a reference before adding other children.🤖 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 `@Assets/Scripts/Ships/Tests/TestHelpers/Factories/ShipTestBuilder.cs` around lines 91 - 99, The code in ShipTestBuilder.WithEngineModule relies on a fragile GetChild(childCount - 1) to locate the newly created Engine; instead change the creation API or capture the created object: modify ModuleFactory.CreateEngineModule to return the created Engine (or the created GameObject) and update WithEngineModule to use that returned Engine instance to add to _engines (or alternatively have CreateEngineModule accept an out/ref parameter to provide the Engine). Update references in WithEngineModule so it no longer uses _shipGo.transform.GetChild(...), but directly uses the returned Engine object when calling _engines.Add(engine).Assets/Scripts/Ships/Tests/TestHelpers/Mocks/TestModuleCatalog.cs (1)
16-19: 💤 Low valueConsider explicit handling of duplicate keys.
The
Addmethod silently overwrites existing entries via the dictionary indexer. While this can be useful for test flexibility, it may hide mistakes if the same archetype ID is accidentally registered twice. Consider either:
- Documenting the overwrite behavior, or
- Throwing when a duplicate key is added (if overwriting is unintended), or
- Renaming to
AddOrUpdateto signal the overwrite semantics🤖 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 `@Assets/Scripts/Ships/Tests/TestHelpers/Mocks/TestModuleCatalog.cs` around lines 16 - 19, The Add method in TestModuleCatalog currently overwrites existing entries in the _idToPrefab dictionary; change it to explicitly handle duplicates by checking _idToPrefab.ContainsKey(id) and throwing an ArgumentException (or InvalidOperationException) with a clear message including the id when a duplicate is added, so accidental double-registration fails fast; if overwriting is intended instead, rename the method to AddOrUpdate and document the behavior and update any tests that rely on the current overwrite semantics.Assets/Scripts/Ships/Tests/TestHelpers/Mocks/TestContentCatalog.cs (1)
34-44: ⚡ Quick winConsider adding validation for test helper robustness.
The
AddPrefabandAddSpritemethods use dictionary indexer assignment, which:
- Silently overwrites if the same
idis registered twice- Accepts null parameters without validation
While this may provide flexibility in test setup, it can hide test bugs if a test accidentally registers duplicate IDs or passes null values.
🛡️ Optional defensive guards
public void AddPrefab(string id, GameObject prefab) { + if (string.IsNullOrEmpty(id)) throw new System.ArgumentNullException(nameof(id)); + if (prefab == null) throw new System.ArgumentNullException(nameof(prefab)); + if (_idToPrefab.ContainsKey(id)) throw new System.InvalidOperationException($"Prefab with id '{id}' already registered"); _idToPrefab[id] = prefab; _prefabToId[prefab] = id; } public void AddSprite(string id, Sprite sprite) { + if (string.IsNullOrEmpty(id)) throw new System.ArgumentNullException(nameof(id)); + if (sprite == null) throw new System.ArgumentNullException(nameof(sprite)); + if (_idToSprite.ContainsKey(id)) throw new System.InvalidOperationException($"Sprite with id '{id}' already registered"); _idToSprite[id] = sprite; _spriteToId[sprite] = id; }🤖 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 `@Assets/Scripts/Ships/Tests/TestHelpers/Mocks/TestContentCatalog.cs` around lines 34 - 44, Add defensive validation to AddPrefab and AddSprite: check parameters for null and ensure you don't silently overwrite existing mappings by verifying _idToPrefab/_idToSprite and _prefabToId/_spriteToId don't already contain the given id or object; if null, throw ArgumentNullException, and if the id or object is already registered, throw an ArgumentException (or use TryAdd and fail loudly) so duplicate registrations are caught in tests. This applies to the methods AddPrefab and AddSprite and the backing dictionaries _idToPrefab, _prefabToId, _idToSprite, and _spriteToId.
🤖 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 `@Assets/Scripts/Ships/Tests/TestHelpers/Factories/ShipTestBuilder.cs`:
- Around line 210-221: The if-check in WireShip<T> uses inverted logic and is
redundant: it calls _shipGo.SetActive(false) only when !_shipGo.activeSelf
(already inactive), while ModuleFactory.WireShip later forcefully toggles active
state; either remove the entire if-block (the lines referencing _parent,
_deactivateBeforeWire and _shipGo.SetActive(false)) or correct the condition to
"if (_parent != null && _deactivateBeforeWire && _shipGo.activeSelf)
_shipGo.SetActive(false)" so the GameObject is deactivated only when currently
active; update WireShip<T> accordingly and keep ModuleFactory.WireShip behavior
consistent.
In `@Assets/Scripts/UI/MainGame/PauseMenuController.cs`:
- Around line 52-53: The current guard in PauseMenuController checks both
uiDocument and uiDocument.rootVisualElement but throws a message that only
mentions the UI Document; update the check to distinguish the two failure cases
by either splitting into two ifs or by throwing a clearer message. Specifically,
in the method containing the guard (where uiDocument is validated), add one
check that throws e.g. UnityException("[PauseMenuController] uiDocument is
null.") when uiDocument == null, and a separate check that throws
UnityException("[PauseMenuController] uiDocument.rootVisualElement is null.")
when uiDocument != null && uiDocument.rootVisualElement == null so runtime
errors point to the exact missing dependency.
---
Outside diff comments:
In `@Assets/Scripts/Ships/Tests/ModuleJointLifecycleTests.cs`:
- Around line 48-52: In ModuleJointLifecycleTests remove the non-descriptive
Debug.Log calls ("mkay0", "mkay", "mkay2") that were left in the test; locate
the test method in the ModuleJointLifecycleTests class where
Object.Destroy(other.gameObject) and yield return WaitForLifecycle() are used
and delete those Debug.Log statements so the test output is clean and only
contains meaningful logs or assertions.
In `@Assets/Scripts/Ships/Tests/ShipDisconnectionTests.cs`:
- Around line 137-147: Replace the discarded boolean with an actual assertion:
instead of "_ = ship.ModuleGraph.ContainsNode(moduleA) && ...", call an
NUnit/Unity assertion such as
Assert.IsTrue(ship.ModuleGraph.ContainsNode(moduleA) &&
ship.ModuleGraph.ContainsNode(moduleB) &&
ship.ModuleGraph.ContainsNode(moduleC), "After removing A-Command connection
expected nodes A, B, C to be present (alternate-path behavior)"); reference the
same symbols ship.ModuleGraph.ContainsNode, moduleA, moduleB, moduleC and keep
or adjust the Debug.Log message for diagnostics as needed.
---
Nitpick comments:
In `@Assets/Scripts/Ships/Tests/TestHelpers/Factories/ShipTestBuilder.cs`:
- Around line 91-99: The code in ShipTestBuilder.WithEngineModule relies on a
fragile GetChild(childCount - 1) to locate the newly created Engine; instead
change the creation API or capture the created object: modify
ModuleFactory.CreateEngineModule to return the created Engine (or the created
GameObject) and update WithEngineModule to use that returned Engine instance to
add to _engines (or alternatively have CreateEngineModule accept an out/ref
parameter to provide the Engine). Update references in WithEngineModule so it no
longer uses _shipGo.transform.GetChild(...), but directly uses the returned
Engine object when calling _engines.Add(engine).
In `@Assets/Scripts/Ships/Tests/TestHelpers/Mocks/TestContentCatalog.cs`:
- Around line 34-44: Add defensive validation to AddPrefab and AddSprite: check
parameters for null and ensure you don't silently overwrite existing mappings by
verifying _idToPrefab/_idToSprite and _prefabToId/_spriteToId don't already
contain the given id or object; if null, throw ArgumentNullException, and if the
id or object is already registered, throw an ArgumentException (or use TryAdd
and fail loudly) so duplicate registrations are caught in tests. This applies to
the methods AddPrefab and AddSprite and the backing dictionaries _idToPrefab,
_prefabToId, _idToSprite, and _spriteToId.
In `@Assets/Scripts/Ships/Tests/TestHelpers/Mocks/TestModuleCatalog.cs`:
- Around line 16-19: The Add method in TestModuleCatalog currently overwrites
existing entries in the _idToPrefab dictionary; change it to explicitly handle
duplicates by checking _idToPrefab.ContainsKey(id) and throwing an
ArgumentException (or InvalidOperationException) with a clear message including
the id when a duplicate is added, so accidental double-registration fails fast;
if overwriting is intended instead, rename the method to AddOrUpdate and
document the behavior and update any tests that rely on the current overwrite
semantics.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 30ab7a9d-7fe6-4eb0-82c1-70b90c251a37
📒 Files selected for processing (54)
Assets/Scripts/Core/Ship/CrewMember.csAssets/Scripts/E2E/E2ETestBase.csAssets/Scripts/ShipFactory/ShipModuleSO.csAssets/Scripts/ShipFactory/Tests/LegalPositionCalculator/CalculatorTests.csAssets/Scripts/Ships/Modules/Cannon.csAssets/Scripts/Ships/Tests/CrewModuleTests.csAssets/Scripts/Ships/Tests/ModuleConnectionTests.csAssets/Scripts/Ships/Tests/ModuleDestructionThresholdTests.csAssets/Scripts/Ships/Tests/ModuleJointLifecycleTests.csAssets/Scripts/Ships/Tests/ShipControlAllocatorThrustTests.csAssets/Scripts/Ships/Tests/ShipCrewAssignmentTests.csAssets/Scripts/Ships/Tests/ShipDestroyAllModulesTests.csAssets/Scripts/Ships/Tests/ShipDestructionJunkTests.csAssets/Scripts/Ships/Tests/ShipDisconnectionTests.csAssets/Scripts/Ships/Tests/ShipGameplayMovementTests.csAssets/Scripts/Ships/Tests/ShipSnapshotServiceTests.csAssets/Scripts/Ships/Tests/TestHelpers/Factories.metaAssets/Scripts/Ships/Tests/TestHelpers/Factories/ModuleFactory.csAssets/Scripts/Ships/Tests/TestHelpers/Factories/ModuleFactory.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Factories/ShipTestBuilder.csAssets/Scripts/Ships/Tests/TestHelpers/Factories/ShipTestBuilder.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Factories/ShipTestFactory.csAssets/Scripts/Ships/Tests/TestHelpers/Factories/ShipTestFactory.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Fixtures.metaAssets/Scripts/Ships/Tests/TestHelpers/Fixtures/ShipTestBase.csAssets/Scripts/Ships/Tests/TestHelpers/Fixtures/ShipTestBase.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Fixtures/TestContainerFactory.csAssets/Scripts/Ships/Tests/TestHelpers/Fixtures/TestContainerFactory.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Fixtures/Utils.csAssets/Scripts/Ships/Tests/TestHelpers/Fixtures/Utils.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Mocks.metaAssets/Scripts/Ships/Tests/TestHelpers/Mocks/TestContentCatalog.csAssets/Scripts/Ships/Tests/TestHelpers/Mocks/TestContentCatalog.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Mocks/TestDebrisSpawner.csAssets/Scripts/Ships/Tests/TestHelpers/Mocks/TestDebrisSpawner.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Mocks/TestMapInfo.csAssets/Scripts/Ships/Tests/TestHelpers/Mocks/TestMapInfo.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Mocks/TestModuleCatalog.csAssets/Scripts/Ships/Tests/TestHelpers/Mocks/TestModuleCatalog.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Modules.metaAssets/Scripts/Ships/Tests/TestHelpers/Modules/TestModule.csAssets/Scripts/Ships/Tests/TestHelpers/Modules/TestModule.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Modules/TestPowerModule.csAssets/Scripts/Ships/Tests/TestHelpers/Modules/TestPowerModule.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Proxies.metaAssets/Scripts/Ships/Tests/TestHelpers/Proxies/MovableShipTestProxy.csAssets/Scripts/Ships/Tests/TestHelpers/Proxies/MovableShipTestProxy.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/Proxies/ShipTestProxy.csAssets/Scripts/Ships/Tests/TestHelpers/Proxies/ShipTestProxy.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/SmallMovableShipTestFactory.csAssets/Scripts/Ships/Tests/TestHelpers/SmallMovableShipTestFactory.cs.metaAssets/Scripts/Ships/Tests/TestHelpers/TestContentCatalog.csAssets/Scripts/Ships/Tests/TestHelpers/TestModuleCatalog.csAssets/Scripts/UI/MainGame/PauseMenuController.cs
💤 Files with no reviewable changes (4)
- Assets/Scripts/Ships/Tests/TestHelpers/SmallMovableShipTestFactory.cs.meta
- Assets/Scripts/Ships/Tests/TestHelpers/TestModuleCatalog.cs
- Assets/Scripts/Ships/Tests/TestHelpers/TestContentCatalog.cs
- Assets/Scripts/Ships/Tests/TestHelpers/SmallMovableShipTestFactory.cs
lmProgramming
changed the title
This was referenced Jun 11, 2026
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
1 participant
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.
resolves #68
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests