review: fix test context chaining and Environment CoreRegistry#5323
Open
agent-refr wants to merge 10 commits into
Open
review: fix test context chaining and Environment CoreRegistry#5323agent-refr wants to merge 10 commits into
agent-refr wants to merge 10 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughEnvironment.reset now sets the final ContextImpl into CoreRegistry; multiple tests introduce a preserved static baseContext used to recreate per-test ContextImpls and call CoreRegistry.setContext after recreation. A sandbox test switched to JUnit Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Environment
participant ServiceRegistry
participant ContextImpl
participant CoreRegistry
Test->>Environment: setupClass / setup
Environment->>ServiceRegistry: register and populate services/modules
ServiceRegistry->>ContextImpl: used to construct new ContextImpl
Environment->>ContextImpl: new ContextImpl(...populated...)
ContextImpl->>CoreRegistry: CoreRegistry.setContext(context)
Test->>ContextImpl: create per-test ContextImpl(baseContext, serviceRegistry)
ContextImpl->>CoreRegistry: CoreRegistry.setContext(per-test context)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
engine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.java (1)
106-106: Duplicate CoreRegistry assignment in setup.
CoreRegistry.setContext(context)is called at both Line 106 and Line 119 in the same setup path; the second call is redundant.♻️ Cleanup
- CoreRegistry.setContext(context);Also applies to: 119-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.java` at line 106, The setup calls CoreRegistry.setContext(context) twice in TerasologyTestingEnvironment; remove the redundant second invocation so CoreRegistry.setContext(context) is only called once in the setup path (leave the initial call in the setup/initialization code and delete the later duplicate), ensuring no behavioral change to context initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@engine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.java`:
- Around line 86-90: NetworkSystemImpl is being constructed with a stale
context; move the NetworkSystemImpl creation and its registration into the
serviceRegistry so it happens after the test context is rebuilt (i.e., create
new NetworkSystemImpl using the freshly rebuilt context and then register it
with serviceRegistry as a Singleton). Specifically, ensure you instantiate
NetworkSystemImpl after the code that recreates context and game (the
Game/GameManifest setup) and then call
serviceRegistry.with(NetworkSystem.class)...use(() -> networkSystem) so
NetworkSystemImpl holds the current test context rather than the previous one.
---
Nitpick comments:
In
`@engine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.java`:
- Line 106: The setup calls CoreRegistry.setContext(context) twice in
TerasologyTestingEnvironment; remove the redundant second invocation so
CoreRegistry.setContext(context) is only called once in the setup path (leave
the initial call in the setup/initialization code and delete the later
duplicate), ensuring no behavioral change to context initialization.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af3b4828-3638-4d3f-b8e3-a4eba1f87e90
📒 Files selected for processing (6)
engine-tests/src/main/java/org/terasology/engine/Environment.javaengine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.javaengine-tests/src/test/java/org/terasology/engine/entitySystem/BaseEntityRefTest.javaengine-tests/src/test/java/org/terasology/engine/entitySystem/PojoEntityManagerTest.javaengine-tests/src/test/java/org/terasology/engine/entitySystem/PojoEntityPoolTest.javaengine-tests/src/test/java/org/terasology/engine/persistence/EntitySerializerTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/reflection/TestModuleEnvironmentSandbox.java (1)
30-42: Consider consistency with existing test patterns for PathManager singleton state.
PathManager.getInstance().useDefaultHomePath()exists and could be used in an@AfterEachto reset the singleton state. However, no other test in the codebase (includingTerasologyTestingEnvironment,AutoConfigManagerTest, andModuleEnvironmentTest) implements this cleanup, despite using the sameuseOverrideHomePathpattern. Since tests pass in Jenkins with this pattern, adding cleanup here would diverge from the established convention. If singleton leakage is a concern, consider addressing it consistently across all affected tests rather than in this one class alone.Additionally, the
protectedvisibility on the@BeforeEachmethod is unusual for a non-abstract test class—void(package-private) would suffice unless this class is intended to be extended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/reflection/TestModuleEnvironmentSandbox.java` around lines 30 - 42, The test leaves PathManager overridden and uses a protected `@BeforeEach`; change the setup to package-private void and add an `@AfterEach` that calls PathManager.getInstance().useDefaultHomePath() to reset singleton state, and keep the existing useOverrideHomePath(tempHome) call in provideSandbox (referencing PathManager.getInstance().useOverrideHomePath(tempHome)) and the new cleanup should call PathManager.getInstance().useDefaultHomePath(); this keeps the test self-contained and prevents singleton leakage if other tests change in future.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/reflection/TestModuleEnvironmentSandbox.java`:
- Around line 30-42: The test leaves PathManager overridden and uses a protected
`@BeforeEach`; change the setup to package-private void and add an `@AfterEach` that
calls PathManager.getInstance().useDefaultHomePath() to reset singleton state,
and keep the existing useOverrideHomePath(tempHome) call in provideSandbox
(referencing PathManager.getInstance().useOverrideHomePath(tempHome)) and the
new cleanup should call PathManager.getInstance().useDefaultHomePath(); this
keeps the test self-contained and prevents singleton leakage if other tests
change in future.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16246089-c87e-4a2c-8fd1-6dc28feb5653
📒 Files selected for processing (1)
engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/reflection/TestModuleEnvironmentSandbox.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine-tests/src/test/java/org/terasology/engine/core/PathManagerTest.java`:
- Around line 24-28: The test mutates the PathManager singleton via
PathManager.getInstance() and useOverrideHomePath in setup(), but doesn't
restore singleton state; add an `@AfterEach` teardown that resets the PathManager
home path to its original value (capture the original PathManager or original
home Path in setup before calling useOverrideHomePath) and call the appropriate
restoration method or reinitialize the singleton so subsequent tests don't see
leaked state; reference the setup() method, PathManager.getInstance(),
useOverrideHomePath, and add a new teardown method annotated `@AfterEach` to
perform the restore.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 427835ef-c0a9-4d5e-a253-a9a610dff1c1
📒 Files selected for processing (1)
engine-tests/src/test/java/org/terasology/engine/core/PathManagerTest.java
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 6, 2026
…Manager in tests - TerasologyTestingEnvironment: construct NetworkSystemImpl with baseContext explicitly, not the context variable which could hold stale state from a prior test if ordering changes. - PathManagerTest: add @AfterEach to restore the original PathManager singleton, preventing state leakage to subsequent test classes. CodeRabbit review feedback on PR MovingBlocks#5323. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
engine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.java (1)
86-89: Close the staleCoreRegistrywindow at the start ofsetup().From the second test onward,
CoreRegistrystill points at the previous test's child context until Line 106. Any setup-time code above that line that consults it will therefore observe stale state. Resetting it tobaseContextat the top ofsetup()would make the isolation boundary explicit.♻️ Proposed fix
`@BeforeEach` public void setup() throws Exception { + CoreRegistry.setContext(baseContext); ServiceRegistry serviceRegistry = new ServiceRegistry(); serviceRegistry.with(ModuleManager.class).lifetime(Lifetime.Singleton).use(() -> moduleManager);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.java` around lines 86 - 89, In TerasologyTestingEnvironment.setup(), reset the global CoreRegistry to the new baseContext at the start of the method so tests don't see the previous child context; specifically, at the top of setup() call the CoreRegistry reset method to point CoreRegistry to baseContext (the same baseContext used to build serviceRegistry and NetworkSystemImpl/Game registration) so subsequent setup-time code observes the fresh baseContext rather than stale state from the prior test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@engine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.java`:
- Around line 86-89: In TerasologyTestingEnvironment.setup(), reset the global
CoreRegistry to the new baseContext at the start of the method so tests don't
see the previous child context; specifically, at the top of setup() call the
CoreRegistry reset method to point CoreRegistry to baseContext (the same
baseContext used to build serviceRegistry and NetworkSystemImpl/Game
registration) so subsequent setup-time code observes the fresh baseContext
rather than stale state from the prior test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70e620df-2030-49ba-8d6c-28f75baef8dc
📒 Files selected for processing (2)
engine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.javaengine-tests/src/test/java/org/terasology/engine/core/PathManagerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- engine-tests/src/test/java/org/terasology/engine/core/PathManagerTest.java
There was a problem hiding this comment.
Pull request overview
This PR addresses test isolation issues introduced during the gestalt-di migration follow-ups by preventing runaway Context parent-chaining across tests, ensuring CoreRegistry points at the real constructed context, and improving PathManager-related test stability/coverage.
Changes:
- Introduces a stable
baseContext(per class) and rebuilds per-test child contexts from it to prevent context/service leakage between tests. - Updates
CoreRegistrywhen the “real” context is created so static lookups don’t point at an earlier/empty context. - Fixes
TestModuleEnvironmentSandboxordering sensitivity by using a realPathManagerwith@TempDir, and adds newPathManagerunit tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| engine-tests/src/test/java/org/terasology/engine/TerasologyTestingEnvironment.java | Creates a stable base context and recreates per-test child contexts; registers an initialized Game instance; refreshes CoreRegistry. |
| engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/reflection/TestModuleEnvironmentSandbox.java | Switches from mocked PathManager injection to @TempDir + real PathManager home override. |
| engine-tests/src/test/java/org/terasology/engine/persistence/EntitySerializerTest.java | Uses baseContext + per-test child contexts; refreshes CoreRegistry. |
| engine-tests/src/test/java/org/terasology/engine/entitySystem/PojoEntityPoolTest.java | Uses baseContext + per-test child contexts; refreshes CoreRegistry. |
| engine-tests/src/test/java/org/terasology/engine/entitySystem/PojoEntityManagerTest.java | Uses baseContext + per-test child contexts; refreshes CoreRegistry. |
| engine-tests/src/test/java/org/terasology/engine/entitySystem/BaseEntityRefTest.java | Uses baseContext + per-test child contexts; refreshes CoreRegistry. |
| engine-tests/src/test/java/org/terasology/engine/core/PathManagerTest.java | Adds initial unit tests for PathManager behavior (override/default home, sanitization, directory hierarchy). |
| engine-tests/src/main/java/org/terasology/engine/Environment.java | Updates CoreRegistry after building the real context in reset(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
agent-refr
pushed a commit
to SiliconSaga/Terasology
that referenced
this pull request
Apr 6, 2026
…ecated API - PathManagerTest + TestModuleEnvironmentSandbox: restore original home path in @AfterEach instead of trying to restore the singleton instance (which is the same object). Captures homePath before override and restores it after each test. - PojoEntityPoolTest: use non-deprecated addEntityManagementRelatedClasses overload (the Context parameter was ignored). Copilot review feedback on PR MovingBlocks#5323. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Each @beforeeach was wrapping the previous test's context with new ContextImpl(context, serviceRegistry), creating a growing parent chain and leaking services between tests. Introduce a stable baseContext from @BeforeAll and create per-test child contexts from that instead. Also register the loaded Game instance in TerasologyTestingEnvironment instead of an uninitialized Game::new. CodeRabbit flagged this cluster on PR MovingBlocks#5299. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…et() CoreRegistry was set to the initial empty context at line 55 but never updated after the real context was created from the serviceRegistry at line 89. Any CoreRegistry lookup after reset() resolved from the stale empty context. CodeRabbit flagged this on PR MovingBlocks#5299 (outside-diff-range). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…olation The test used PathManagerProvider to mock PathManager, but the mock didn't stub getInstallPath() which ModuleManager needs. The test only passed when run after other tests that initialized the real PathManager singleton. Replaced with @tempdir + real PathManager, matching the pattern used by other test classes. The test verifies ModuleEnvironmentSandbox type resolution, not PathManager behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PathManager had no test coverage. Add tests verifying: - useOverrideHomePath sets all directory paths and creates them - getSavePath/getRecordingPath sanitize titles (strip special chars) - getSavePath preserves valid titles - Save paths are under the saves directory Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Manager in tests - TerasologyTestingEnvironment: construct NetworkSystemImpl with baseContext explicitly, not the context variable which could hold stale state from a prior test if ordering changes. - PathManagerTest: add @AfterEach to restore the original PathManager singleton, preventing state leakage to subsequent test classes. CodeRabbit review feedback on PR MovingBlocks#5323. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix CheckStyle: move private static final logger before other static fields in TerasologyTestingEnvironment. - Add tests from Gemini review: empty/special-char title handling, path traversal sanitization, and platform-specific useDefaultHomePath tests (gated by @EnabledOnOs). Windows test has a known side effect of creating a real Terasology directory under Saved Games. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ecated API - PathManagerTest + TestModuleEnvironmentSandbox: restore original home path in @AfterEach instead of trying to restore the singleton instance (which is the same object). Captures homePath before override and restores it after each test. - PojoEntityPoolTest: use non-deprecated addEntityManagementRelatedClasses overload (the Context parameter was ignored). Copilot review feedback on PR MovingBlocks#5323. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t CoreRegistry in setup - Remove @AfterEach PathManager restore from PathManagerTest and TestModuleEnvironmentSandbox. The "original" home path was often a @tempdir from a previous test class that's already been deleted, causing NoSuchFileException. Each test sets its own @tempdir in @beforeeach, so restoration is unnecessary. - Reset CoreRegistry to baseContext at the top of TerasologyTestingEnvironment.setup() to close the stale context window between tests (CodeRabbit nitpick). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix DeclarationOrder: protected static before private static fields. - Add inline comments on all four CoreRegistry.setContext calls explaining their distinct purposes: initial setup, stale context cleanup, per-test child switch, and reassertion after prefab loading. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cervator
force-pushed
the
review/post-di-test-env
branch
from
e6d1ec1 to
4a1167a
Compare
…-class pollution PathManagerTest mutates the PathManager singleton via useOverrideHomePath but never restored it. When JUnit runs ServerConnectListManagerTest in the same JVM afterward, PathManager.getHomePath() returns the now-deleted @tempdir, causing NoSuchFileException. Add @AfterEach that restores the original home path with a guard for cases where the original path was itself a temp dir that's already gone. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
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.
Summary
Seventh review follow-up to #5299 (gestalt-di migration). Test context isolation, new PathManager tests, and a pre-existing test ordering bug fix.
Changes
baseContextpattern —@BeforeAllcreates a stable root,@BeforeEachcreates child contexts from that root instead of chaining from the previous test's context. Also registers a loadedGameinstance instead ofGame::new.addEntityManagementRelatedClassesoverload.TestModuleEnvironmentSandbox — test ordering bug
These 3 tests used
PathManagerProviderto mock thePathManagersingleton, but the mock didn't stubgetInstallPath(). TheModuleManagerconstructor callsJvm.logClasspath()which needs that method, so the tests NPE'd — unless they ran after another test class that had initialized the real PathManager singleton, giving the mock a real instance to fall back on.Jenkins happened to run classes in a favorable order. Local Gradle didn't.
Fixed by replacing the incomplete mock with
@TempDir+ the realPathManager, matching the pattern used by other test classes. The test verifiesModuleEnvironmentSandboxtype resolution, not PathManager behavior.PathManager tests (new)
PathManager had zero test coverage. Added tests for:
useOverrideHomePath— verifies all directory paths are set and createdgetSavePath/getRecordingPath— title sanitization (special chars stripped, path traversal neutralized)useDefaultHomePathgated by@EnabledOnOs(Linux, Mac, Windows)Note: the Windows
useDefaultHomePathtest creates a realTerasologydirectory under the user's Saved Games folder via JNA. Not destructive but a known side effect — future candidate for a@Tag("filesystem-side-effects")group.Additional tests contributed via Gemini review.
Test Plan
🤖 Generated with Claude Code