Skip to content

test: add PR #37 register allocation regressions - #49

Open
EthanZero2Hero wants to merge 1 commit into
ScratchV-Compiler:mainfrom
EthanZero2Hero:ci/pr37-regression
Open

test: add PR #37 register allocation regressions#49
EthanZero2Hero wants to merge 1 commit into
ScratchV-Compiler:mainfrom
EthanZero2Hero:ci/pr37-regression

Conversation

@EthanZero2Hero

Copy link
Copy Markdown

Summary

Validation

The regression cases were validated against the fetched PR #37 v1.5 implementation. CI will run:

python3.12 -m pytest tests/test_pr37_regression.py -v --tb=short

@github-actions

Copy link
Copy Markdown

🤖 AI Code Review

共审查 2 个变更文件

📁 .github/workflows/ci.yml

🔴 Hardcoded python version — Line 48: python3.12 硬编码,与工作流其他步骤可能不一致(例如 matrix 或系统默认版本)。建议使用 python 或从环境变量/矩阵中获取版本。

🟡 Missing test file validation — 未确认 tests/test_pr37_regression.py 是否存在,若文件缺失则 CI 会失败。建议添加 if: steps.check-test-file.outputs.exists == 'true' 或提前检查文件。

🟡 Step name not reusable — 名称包含 “PR #37”,但此步骤将永久保留在 CI 中,不适用于未来其他 PR。建议改为 “Run register-allocation regression tests”。

💭 No shared pytest configuration — 直接调用 pytest 可能跳过项目级别的 pyproject.tomlpytest.ini 配置(如插件、标记、超时)。建议省略参数,让测试继承项目默认配置。

💭 Step placement — 新步骤位于 Generate test visualization page 之前,但后者仅限 main 分支,而新步骤无条件运行。若本意在 main 上才运行,应添加 if: github.ref == 'refs/heads/main' 或保留为全局(当前逻辑一致)。


📁 tests/test_pr37_regression.py

🟡 Suggestion: Use explicit assertions instead of relying on StopIteration — In test_spilled_vreg_redefinition_is_written_back_before_reload, if redefine or writeback are not found, next() raises StopIteration with a cryptic error message. Consider using pytest.fail() with a descriptive message or assert with a condition.

🟡 Suggestion: Tighten regex to avoid potential false positives — The regex (?<![A-Za-z0-9_])v[0-9]+(?![A-Za-z0-9_]) is used to check for leaked vregs. It correctly excludes comments, but if a physical register were named like v0 (unlikely), it would be flagged. Consider using a more specific pattern like \bv[0-9]+\b if the assembler supports word boundaries.

💭 Nit: Test relies on hardcoded comment strings"store redefined v0" and "reload v0" are implementation details. If the allocator's comment format changes (e.g., "store v0 (redefined)"), the test will break. Consider matching on "store redefined v0" as a substring or using a more robust heuristic.

💭 Nit: Partial duplication of test blocks_pressure_block() and the block in test_spilled_vreg_redefinition... share similar structure. Consider extracting a helper to reduce duplication, but this is minor.


Cynthia-Xichen pushed a commit to Cynthia-Xichen/ScratchV that referenced this pull request Aug 16, 2026
From PR ScratchV-Compiler#49 (ci/pr37-regression), add tests/test_pr37_regression.py
covering spill/reload and machine-operand round-trip for the PR ScratchV-Compiler#37
register allocator.

All 3 tests pass with the project venv.
Cynthia-Xichen pushed a commit to Cynthia-Xichen/ScratchV that referenced this pull request Aug 16, 2026
Run tests/test_pr37_regression.py in CI, from PR ScratchV-Compiler#49 (ci/pr37-regression).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant