Port PR #114: Fix stall upgrade logic and test stability#115
Conversation
- Centralized stall upgrade logic to ensure UI descriptions match gameplay scaling. - Added "Boost" category to the legendary naming system for Bak Kut Teh stalls. - Improved unit test stability by explicitly managing the ViewModel's game loop (cancelling gameJob). - Resolved logical errors and timeouts in KitchelinBonusTest and BossWaveLogicTest. - Updated MilestoneBoostTest and LegendaryNamingTest to reflect current scaling rules and use deterministic upgrade methods. - Validated specificStat in MainViewModel.kt to prevent invalid state changes. Co-authored-by: candour <4670475+candour@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughPrecomputes available upgrade stats and validates specific stat choices in MainViewModel, switches non-specific stat selection to the view model’s seeded RNG, widens ChangesUpgrade Selection Refactoring & Test Adjustments
Boost Legendary Naming Extension
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/test/java/com/messark/hawker/BossWaveLogicTest.kt (1)
29-42:⚠️ Potential issue | 🟠 MajorAdd
Dispatchers.resetMain()teardown to prevent cross-test leakage.Line 31 overrides
Dispatchers.Main, but this class never restores it. That can destabilize unrelated tests executed afterward.Suggested fix
import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest import kotlinx.coroutines.test.setMain +import org.junit.After @@ class BossWaveLogicTest { @@ `@Before` fun setup() { Dispatchers.setMain(testDispatcher) @@ viewModel = MainViewModel(application, settingsRepository, gameStateRepository) viewModel.gameJob?.cancel() } + + `@After` + fun tearDown() { + Dispatchers.resetMain() + }app/src/test/java/com/messark/hawker/LegendaryNamingTest.kt (1)
22-26:⚠️ Potential issue | 🟠 MajorAdd
@Afterteardown to reset the Main dispatcher.This test sets
Dispatchers.Mainin setup without resetting it, which can bleed scheduler state into subsequent tests and cause flakiness. ImportresetMainand@After, then add a teardown method.Suggested fix
import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.setMain import org.junit.Assert.* +import org.junit.After import org.junit.Before import org.junit.Test @@ -23,6 +26,11 @@ class LegendaryNamingTest { `@Before` fun setup() { Dispatchers.setMain(testDispatcher) } + + `@After` + fun tearDown() { + Dispatchers.resetMain() + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 89078c47-739f-40e8-92ac-8baf171010a7
📒 Files selected for processing (6)
app/src/main/java/com/messark/hawker/MainViewModel.ktapp/src/main/java/com/messark/hawker/utils/LegendaryNames.ktapp/src/test/java/com/messark/hawker/BossWaveLogicTest.ktapp/src/test/java/com/messark/hawker/KitchelinBonusTest.ktapp/src/test/java/com/messark/hawker/LegendaryNamingTest.ktapp/src/test/java/com/messark/hawker/MilestoneBoostTest.kt
- Ported upgrade logic synchronization and legendary naming from PR #114. - Made RNG injectable in MainViewModel constructor with a default value. - Updated all unit tests to inject a fixed-seed Random(42) for determinism. - Changed gameJob visibility to internal and cancelled it in test setups. - Validated specificStat in applyUpgrade. - Verified all unit tests pass. Co-authored-by: candour <4670475+candour@users.noreply.github.com>
Build Successful! 🚀Note: This link will be removed when the PR is closed. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/messark/hawker/MainViewModel.kt (1)
42-42: ⚡ Quick winLimit
gameJobmutability while keeping test access.
internal varallows same-module reassignment; tests only need to read/cancel. Considerprivate setto preserve lifecycle ownership inMainViewModel.Suggested change
- internal var gameJob: Job? = null + internal var gameJob: Job? = null + private setapp/src/test/java/com/messark/hawker/BossWaveLogicTest.kt (1)
29-42: ⚡ Quick winAdd a teardown to reset
Dispatchers.Main.This test sets
Dispatchers.Mainbut doesn’t restore it, which can bleed into other tests in the same run.Suggested change
+import kotlinx.coroutines.test.resetMain +import org.junit.After @@ class BossWaveLogicTest { @@ `@Before` fun setup() { Dispatchers.setMain(testDispatcher) @@ viewModel = MainViewModel(application, settingsRepository, gameStateRepository, kotlin.random.Random(42)) viewModel.gameJob?.cancel() } + + `@After` + fun tearDown() { + Dispatchers.resetMain() + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f40fbea-853e-4ab5-ab55-f5fc12c2d6db
📒 Files selected for processing (8)
app/src/main/java/com/messark/hawker/MainViewModel.ktapp/src/test/java/com/messark/hawker/BossWaveLogicTest.ktapp/src/test/java/com/messark/hawker/KitchelinBonusTest.ktapp/src/test/java/com/messark/hawker/LegendaryNamingTest.ktapp/src/test/java/com/messark/hawker/MainViewModelTest.ktapp/src/test/java/com/messark/hawker/MilestoneBoostTest.ktapp/src/test/java/com/messark/hawker/StallStatsTest.ktapp/src/test/java/com/messark/hawker/UndeadEnemyTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/test/java/com/messark/hawker/LegendaryNamingTest.kt
- app/src/test/java/com/messark/hawker/KitchelinBonusTest.kt
- app/src/test/java/com/messark/hawker/MilestoneBoostTest.kt
1 participant
This PR ports the remaining logic and stability fixes from the original PR #114 (#114) to resolve merge conflicts and synchronize the codebase with the intended upgrade scaling and test reliability improvements.
Key changes include:
gameJobvisibility tointernalfor test control, added validation for specific upgrades, and ensured the seeded random instance is used for deterministic test results.BossWaveLogicTest,KitchelinBonusTest,LegendaryNamingTest, andMilestoneBoostTestto match the current game state and ensure deterministic, stable runs by cancelling the background game loop in setup.All tests passed successfully.
PR created automatically by Jules for task 7969685778257572278 started by @candour
Summary by CodeRabbit
New Features
Bug Fixes