test: stabilize watchdog rotating-log test and promote Windows lane to blocking - #10070
Open
fszcd wants to merge 3 commits into
Open
test: stabilize watchdog rotating-log test and promote Windows lane to blocking#10070fszcd wants to merge 3 commits into
fszcd wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/unit/test_event_loop_diagnostics.py" line_range="57" />
<code_context>
@pytest.mark.asyncio
async def test_event_loop_watchdog_writes_rotating_log(tmp_path):
"""The watchdog should write to and rotate its log file."""
- log_path = tmp_path / "logs" / "event_loop_watchdog.log"
- log_path.parent.mkdir()
</code_context>
<issue_to_address>
**nitpick:** The test docstring says the watchdog writes to and rotates its log file, but the changed test only makes the watchdog write to `io.StringIO` and tests rotation by calling the private helper directly. The documentation therefore describes an integration behavior that this test no longer exercises.
**Suggested fix:** Update the docstring to describe the two focused checks, or retain an explicit watchdog-to-disk integration assertion.
```suggestion
"""The watchdog should dump stalled-loop stacks and rotate oversized log files."""
```
</issue_to_address>Sourcery assessment
Approved.
Author
|
Note for maintainers: the CI workflows on this PR are awaiting approval (first-time contributor). Everything is ready on our side — could someone approve the workflow runs when convenient? Happy to address any feedback. |
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.
The
unit-testsworkflow keeps the Windows lane best-effort viacontinue-on-error, with a comment saying to promote it once the pre-existing Windows failures are fixed. The suite has actually been green on the Windows lane for a while: I scanned the last ~40 workflow runs and every completed Windows lane passed, with one exception — a flaky failure oftests/unit/test_event_loop_diagnostics.py::test_event_loop_watchdog_writes_rotating_log(example run). This PR deflakes that test and promotes the Windows lane to blocking, per the workflow's own TODO comment.unit-tests工作流目前用continue-on-error让 Windows lane 保持非阻塞,注释写明等既有 Windows 失败修复后应提升为阻塞。我扫描了最近约 40 次 workflow 运行:已完成的 Windows lane 全部通过,唯一例外是test_event_loop_watchdog_writes_rotating_log的一次偶发失败(见上方链接)。本 PR 消除该测试的偶发因素,并按注释意图将 Windows lane 提升为阻塞。Root cause of the flake / 偶发原因
The test asserts that the watchdog's stack dump contains the test file's frame. Two Windows-flavoured races break that:
The watchdog keeps only one dump per stall (deduplicated by heartbeat). If the dump fires before the loop reaches
time.sleep()(the heartbeat starts when the watchdog task starts, not when the stall starts) or only after the stall has ended (the watchdog thread is starved through the whole 50 ms stall on a loaded 2-core runner), the dump catches the loop inside pytest machinery instead of the test frame.On Windows, virus scanners briefly hold freshly written files open, so the prefill
write_text()followed ~20 ms later by the dump-timePath.replace()rotation can fail withWinError 32, leaving no.1file at all (the failure is only logged as a warning).每次停滞只保留一份 dump(按 heartbeat 去重)。若 dump 在循环进入
time.sleep()前触发(heartbeat 在 watchdog 任务启动时记录,而非停滞开始时),或在停滞结束后才触发(高负载双核 runner 上看门狗线程被调度饿着、错过整个 50ms 停滞窗口),dump 抓到的是 pytest 框架栈而非测试栈。Windows 上杀毒软件会短暂占用刚写入的文件,预填
write_text()约 20ms 后 dump 时的Path.replace()轮转会以WinError 32失败,导致根本不生成.1文件(仅以 warning 形式记录)。Modifications / 改动点
tests/unit/test_event_loop_diagnostics.py— split the two concerns insidetest_event_loop_watchdog_writes_rotating_log:dump_fileparameter (anio.StringIO), retrying the intentional stall until the dump lands inside this test (bounded at 5 attempts)._open_watchdog_log_file, with a short settle delay after the prefill write so transient file locks (AV scanners) cannot race thePath.replace, plus a bounded retry..github/workflows/unit_tests.yml— removedcontinue-on-errorforwindows-latestand the now-stale comment above it.tests/unit/test_event_loop_diagnostics.py— 在test_event_loop_watchdog_writes_rotating_log内拆分的两个关注点:栈 dump 通过dump_file(io.StringIO)验证,有界重试直至 dump 落入本测试;日志轮转改为直接同步验证_open_watchdog_log_file,预填写入后留出短暂延迟避开杀毒软件的瞬时文件锁,并加有界重试。不改动任何生产代码,两项行为的覆盖保持不变。.github/workflows/unit_tests.yml— 移除windows-latest的continue-on-error及其上方已过时的注释。Verification / 验证
Full suite on a Windows 11 host (Python 3.13,
uv run python -m pytest tests): 2686 passed, 5 skipped (the 5 skips are symlink tests requiring Developer Mode, unrelated to this change).The restructured test file under deliberate CPU saturation (8 busy-loop workers): 40/40 iterations green. For comparison, a naive timing-only fix still failed 5/40 iterations under the same load, and the previous version of the test was observed failing on the Windows CI lane (run).
本机 Windows 11 全量套件(Python 3.13):2686 passed, 5 skipped(5 个跳过为需要开发者模式的 symlink 测试,与本改动无关)。重构后的测试文件在 8 个满载 CPU worker 下连续 40 次全绿;作为对照,仅调整时序的朴素修法在同等负载下仍有 5/40 失败。
This is NOT a breaking change. / 这不是一个破坏性变更。
Checklist / 检查清单
Summary by Sourcery
Stabilize event-loop watchdog diagnostics coverage and make the Windows unit-test lane blocking.
Bug Fixes:
Enhancements:
CI:
Tests: