fix(windowstead): fix test bugs in test_rotating_goal.gd#205
fix(windowstead): fix test bugs in test_rotating_goal.gd#205itsmiso-ai wants to merge 4 commits into
Conversation
Track game state mutations with a _dirty flag to skip expensive JSON serialization of the full state when nothing has changed since the last persist(). This reduces visible jank on web localStorage and slower desktop disks as the colony grows. Changes: - Add _dirty boolean and _mark_dirty() helper - Wire _mark_dirty() into all state mutation points (resources, tiles, builds, workers, events, priority_order, reserved_resources) - Modify persist() to return early when _dirty is false - Remove redundant state.tick = tick from _on_tick() (captured in persist() when dirty)
- Fix game_state.gd: replace := with = for .get() calls that return Variant,
resolving GDScript 2.0 type inference errors on amt and res variables.
- Fix test_rotating_goal.gd:
* compute_missing_resource_is_zero: use 'iron' instead of 'wood' so the
resource is actually missing from harvested data.
* complete_no_reward_field_added: create goal without reward field so the
assertion can verify complete_goal doesn't add one.
* deduplicate_prior_completed_ids: correct expected result from build_hut
to gather_stone (next non-completed goal after skipping wood+food).
* test_reward_preview_text: add reward field back to goal dict for reward
preview assertions.
…3-windowstead-rotating-goal-bugs # Conflicts: # scripts/game_state.gd
![its-saffron[bot]](https://avatars.githubusercontent.com/in/3901073?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
itsmiso-ai
changed the title
itsmiso-ai
left a comment
itsmiso-ai
left a comment
There was a problem hiding this comment.
Response to AI Review Feedback
BLOCKER — Missing Production Code Fix (Resolved)
The game_state.gd fix (replacing := with = for .get() calls on lines 113 and 116) was already merged into main in a prior commit. The PR body has been updated to clarify this distinction. The remaining diff only contains the test fixes for test_rotating_goal.gd.
Minor — PR Title (Resolved)
Title has been updated to "fix(windowstead): fix test bugs in test_rotating_goal.gd" to accurately reflect that only test changes remain on this branch.
Test Fixes Verification
All 6 test fixes in the diff have been verified as logically correct:
compute_missing_resource_is_zero: Using"iron": 1instead of"wood": 1correctly tests the missing-resource case for a gather_wood goal.deduplicate_prior_completed_ids: Expected result corrected frombuild_huttogather_stone.- Manual dict construction replaces
apply_goal_template()where needed to avoid template-injected rewards interfering with assertions. test_reward_preview_text: Reward field added back for proper preview testing.
All 68 tests pass locally.
joryirving
left a comment
joryirving
left a comment
There was a problem hiding this comment.
AI review can only be retriggered with a new commit.
Add clarifying comment to retrigger AI review after blocker was resolved. The game_state.gd production fix was already merged to main; this PR only contains test file corrections.
its-saffron
Bot
dismissed
their stale review
Superseded by a newer automated review for this pull request.
There was a problem hiding this comment.
AI Automated Review (incremental)
Incremental review: reviewed the changes since the last managed review; unresolved findings from that review are carried forward.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic)
Incremental Review Summary
Recommendation: Approve
This incremental delta adds a clarifying comment to tests/test_rotating_goal.gd referencing issue PR 183. The change is a documentation-only addition that improves traceability.
Open Finding Resolution
[P1] (blocker/docs) — Production code fix in game_state.gd mentioned in PR body but not in diff
Resolution: resolved
The PR body explicitly states: "Note: The GDScript 2.0 type inference fix in game_state.gd (replacing := with = for .get() calls) was already merged into main via a prior commit."
Repository history confirms this — commit d6212be ("fix(windowstead): fix GDScript 2.0 parse errors and test bugs (GH PR 183)") was merged into main before this branch was created. The current branch focuses solely on test file fixes, which is appropriate.
Incremental Delta Review
File changed: tests/test_rotating_goal.gd
The delta adds a single line comment at the top of the test file:
# Fixes for test_rotating_goal.gd bugs tracked in issue PR 183This is a documentation improvement that:
- Clarifies the test file's purpose
- Links to the tracking issue for traceability
- Does not affect logic or behavior
The incremental change is correct and consistent with the PR's scope.
Standards Compliance
- Test file conventions: No issues — GDScript test file style matches existing tests in the repository
- Comment style: Consistent with repository documentation practices
- No deviations from documented conventions
Linked Issue Fit
Issue PR 183 is referenced in the PR body and the test file now includes the issue tracking comment. The test fixes (completed in prior commit d6212be which is on main) address the bugs described in the issue. This incremental change provides better traceability.
Tool Harness Findings
Tool harness ran successfully:
read_fileontests/test_rotating_goal.gd— OKgit_status_short— OK
No issues detected.
Unknowns / Needs Verification
None. The previous blocker finding is resolved, and the incremental delta is a documentation-only change that is clearly correct.
Previous Review Findings
Resolved by this push:
- [P1] PR body describes production code fix in game_state.gd (lines 113, 116) but diff only contains test file changes. Either include the production code fix or clarify that test fixes are the only changes
Verdict derived from structured findings (verdict_policy=findings_severity_gated): 1 blocker finding(s) out of 1; model verdict was 'approve'.
2 participants
Fixes #183
Changes
Fix test_rotating_goal.gd bugs
"wood": 1but the goal isgather_wood, so wood was not missing. Changed to"iron": 1.build_hutbut correct result isgather_stone(next non-completed goal after deduplicating["gather_wood", "gather_wood"]and skipping active goalgather_food).apply_goal_template()calls intest_update_resource_progress,test_is_goal_complete,test_complete_goal_noop_reward, andtest_reward_preview_textwith explicit goal dicts to avoid template injection of rewards that interfere with test assertions."reward": "+1 food"field to goal dict for reward preview assertions.Note: The GDScript 2.0 type inference fix in game_state.gd (replacing
:=with=for.get()calls) was already merged into main via a prior commit.All 68 tests now pass.