fix(SwitchOnmyoji): give both switch_role loops a bounded exit - #1842
Enough1122 wants to merge 1 commit into
Conversation
Both while True loops in switch_role could spin indefinitely. When the game is left on a sub-page of the onmyoji screen (e.g. the hero-exchange page), I_ONMYOJI_SWITCH never opens the role list, so the loop kept clicking until the global ~2min GameStuckError watchdog fired, losing the actual cause. Each loop now raises a ScriptError naming the failure after 20s.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
嘿——我发现了 2 个问题
AI 代理提示
请处理本次代码审查中的评论：
## 单独的评论
### 评论 1
<location path="tests/test_switch_onmyoji.py" line_range="15" />
<code_context>
from module.atom.image import RuleImage
+from module.base.timer import Timer
+from module.exception import ScriptError
from tasks.Component.SwitchOnmyoji.assets import SwitchOnmyojiAssets
</code_context>
<issue_to_address>
**nitpick:** 新增的 `Timer` 导入未被使用，因此测试模块包含冗余导入；如果 lint 配置将未使用的导入视为错误，检查将失败。
**建议修复：** 删除未使用的 `from module.base.timer import Timer` 导入。
</issue_to_address>
### 评论 2
<location path="tests/test_switch_onmyoji.py" line_range="100-105" />
<code_context>
+ assert task.ui_clicks == 1
+
+
+def test_timeouts_are_not_shared_between_instances():
+ a = ScriptedSwitch(appear_results=[])
+ b = ScriptedSwitch(appear_results=[])
+
+ assert a.SWITCH_TAB_TIMEOUT is not None
+ assert b.SWITCH_TAB_TIMEOUT is not None
</code_context>
<issue_to_address>
**nitpick (testing):** `test_timeouts_are_not_shared_between_instances` 没有测试计时器隔离性：它只断言两个类属性不为 `None`，既没有启动并发的 `switch_role` 调用，也没有检查它们的 `Timer` 实例。即使超时状态被意外共享，该测试也会通过。
**建议修复：** 让两个实例并发执行，或以其他方式验证每次调用都会创建并推进一个独立的计时器。
</issue_to_address>Sourcery 评估
已批准。
Original comment in English
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_switch_onmyoji.py" line_range="15" />
<code_context>
from module.atom.image import RuleImage
+from module.base.timer import Timer
+from module.exception import ScriptError
from tasks.Component.SwitchOnmyoji.assets import SwitchOnmyojiAssets
</code_context>
<issue_to_address>
**nitpick:** The added `Timer` import is unused, so the test module contains a redundant import and will fail lint configurations that treat unused imports as errors.
**Suggested fix:** Remove the unused `from module.base.timer import Timer` import.
</issue_to_address>
### Comment 2
<location path="tests/test_switch_onmyoji.py" line_range="100-105" />
<code_context>
+ assert task.ui_clicks == 1
+
+
+def test_timeouts_are_not_shared_between_instances():
+ a = ScriptedSwitch(appear_results=[])
+ b = ScriptedSwitch(appear_results=[])
+
+ assert a.SWITCH_TAB_TIMEOUT is not None
+ assert b.SWITCH_TAB_TIMEOUT is not None
</code_context>
<issue_to_address>
**nitpick (testing):** `test_timeouts_are_not_shared_between_instances` does not test timer isolation: it only asserts that two class attributes are non-`None`, and never starts concurrent `switch_role` calls or inspects their `Timer` instances. The test passes even if timeout state were accidentally shared.
**Suggested fix:** Exercise both instances concurrently or otherwise verify that each invocation creates and advances an independent timer.
</issue_to_address>Sourcery assessment
Approved.
| """ | ||
| import pytest | ||
|
|
||
| from module.base.timer import Timer |
There was a problem hiding this comment.
nitpick: 新增的 Timer 导入未被使用，因此测试模块包含冗余导入；如果 lint 配置将未使用的导入视为错误，检查将失败。
建议修复： 删除未使用的 from module.base.timer import Timer 导入。
Original comment in English
nitpick: The added Timer import is unused, so the test module contains a redundant import and will fail lint configurations that treat unused imports as errors.
Suggested fix: Remove the unused from module.base.timer import Timer import.
| def test_timeouts_are_not_shared_between_instances(): | ||
| a = ScriptedSwitch(appear_results=[]) | ||
| b = ScriptedSwitch(appear_results=[]) | ||
|
|
||
| assert a.SWITCH_TAB_TIMEOUT is not None | ||
| assert b.SWITCH_TAB_TIMEOUT is not None |
There was a problem hiding this comment.
nitpick (testing): test_timeouts_are_not_shared_between_instances 没有测试计时器隔离性：它只断言两个类属性不为 None，既没有启动并发的 switch_role 调用，也没有检查它们的 Timer 实例。即使超时状态被意外共享，该测试也会通过。
建议修复： 让两个实例并发执行，或以其他方式验证每次调用都会创建并推进一个独立的计时器。
Original comment in English
nitpick (testing): test_timeouts_are_not_shared_between_instances does not test timer isolation: it only asserts that two class attributes are non-None, and never starts concurrent switch_role calls or inspects their Timer instances. The test passes even if timeout state were accidentally shared.
Suggested fix: Exercise both instances concurrently or otherwise verify that each invocation creates and advances an independent timer.
Change Summary
Reconstructed Intent点击此处展开
Observed Constraints点击此处展开
Intent Alignment
Release Risk
Validation Gaps点击此处展开
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
Findings
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
1 participant
Summary
SwitchOnmyoji.switch_rolehad two unconditionalwhile Trueloops with no exit condition. Add a bounded timeout to each so a stuck page fails fast with a named error instead of spinning until the globalGameStuckErrorwatchdog fires.Real observed failure on client 1.8.66 (versionCode 260610):
The three saved stuck-window screenshots show the game sitting on the hero-exchange (英杰交换) sub-page, with
英杰highlighted instead of阴阳师.I_ONMYOJI_SWITCHtherefore never opens the role list, the loop re-clicks every 0.8s, and the run dies ~70s later on an unrelated watchdog error that names none of the real cause.Root cause
tasks/Component/SwitchOnmyoji/switch_onmyoji.py— both loops inswitch_rolearewhile True; nothing bounds them.Change
One file, 18 added lines. Each loop now starts a
Timerand raisesScriptErrorafterSWITCH_TAB_TIMEOUT/SWITCH_BATTLE_TIMEOUT(20s), with a message that names the failed step.No
sleepadded; the timers are plainmodule.base.timer.Timerinstances created per call, so concurrent tasks do not share state.Verification
Live on the real client, 3 consecutive runs, healthy path unchanged:
Forced-failure path (timeout lowered to 0 so the role list can never be reached):
Unit tests,
tests/test_switch_onmyoji.py(5 passed):ScriptErrorScriptErrorui_clickback-path still taken when the confirm icon is missingNotes
tests/tasks/Component/GeneralBattle/test_battle_wait.pyfails to import_DEFAULT_PER_BATTLEon this base SHA. Verified pre-existing on the clean baseline, unrelated to this PR.Sourcery 总结
当无法达到预期的 UI 状态时，让阴阳师角色切换快速失败并提供可操作的错误信息。
错误修复：
ScriptError异常，而不是无限运行。增强功能：
测试：
Original summary in English
Summary by Sourcery
Make onmyoji role switching fail fast with actionable errors when the expected UI state cannot be reached.
Bug Fixes:
Enhancements:
Tests: