ci: split build into lint and test workflows with a Windows matrix - #603
Open
sherjy wants to merge 2 commits into
Open
ci: split build into lint and test workflows with a Windows matrix#603sherjy wants to merge 2 commits into
sherjy wants to merge 2 commits into
Conversation
The oversize branch of test_fetch_rejects_bad_responses_without_caching sends a 65,537-byte body (_MAX_BYTES + 1), so pytest derived a node id tens of thousands of characters long. On Windows that id is written into the PYTEST_CURRENT_TEST environment variable, which is capped at 32,767 characters, so setup and teardown raised 'ValueError: the environment variable is longer than 32767 characters' and the case errored on windows-latest only (Linux has no such limit). Wrap each case in pytest.param(..., id=...) with a short, stable id; the request bodies and assertions are unchanged, so the size guard is still exercised.
build.yml ran lint, pre-commit, mypy, deptry and pytest serially in a single ubuntu-only job. Adding Windows there would rerun the lint steps on every cell and pull pre-commit onto Windows (CRLF pitfalls, cf. NevaMind-AI#482). - lint.yml: ubuntu-latest, Python 3.13; runs 'make check' (lock check, pre-commit, mypy, deptry) exactly once. - test.yml: matrix {ubuntu-latest, windows-latest} x {3.11, 3.13}, fail-fast disabled; runs pytest with coverage directly (make is not guaranteed on windows-latest). Both keep the existing same-repo dedupe guard. Required status checks should be updated from 'build' to 'lint' and 'test'.
Contributor
|
感谢你的 PR~ 关于 windows CI 的引入我之前提过一次,但当时没有结果。 memU 确实需要引入跨平台的 ci,但不是现在。 因为跨平台测试目前不应该被所有代码改动触发,对于 memU 的主体它是跨平台安全的。 需要被测试的主要是各个 host 的跨平台 macos cron 和 windows scheduler 的部分。而这部分代码最近会频繁新增和删减。现在引入跨平台单测 ci 会需要频繁修改 ci 文件。 为了更快的开发,当前更倾向手动测试,等到跨平台支持稳定后,再新增按需触发的 CI workflow。 一个始终触发的 windows / macOS workflow 会拖慢 CI 的进行速度,当前并不希望如此。 |
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.
Summary
Split the single
build.ymljob into two workflows so lint runs once on Linux while tests gain an OS matrix that includes Windows.lint.yml—ubuntu-latest, Python 3.13; runsmake check(lock-file check, pre-commit, mypy, deptry) exactly once.test.yml— matrix{ubuntu-latest, windows-latest} × {3.11, 3.13},fail-fast: false; runs pytest with coverage directly (not viamake, which isn't guaranteed onwindows-latest).build.yml.pr-title.yml,publish-memu-cli.yml, andrelease-please.ymlare untouched.if:guard, so branch pushes and fork PRs behave exactly as before.Closes #505.
Why
Per #505, adding Windows to the combined job would rerun lint/pre-commit on every OS×Python cell and pull pre-commit onto Windows, where its hooks have CRLF pitfalls (cf. #482). Keeping lint single-run on ubuntu and giving only pytest the OS matrix avoids that and lets platform-specific test failures surface.
Surfaced and fixed a real Windows-only failure
Enabling
windows-latestimmediately caught one — exactly the point of #505.tests/test_host_templates.py::test_fetch_rejects_bad_responses_without_cachinghas an "oversize" case that builds a 65,537-byte body, so pytest derived a node id longer than Windows' 32,767-character limit for thePYTEST_CURRENT_TESTenvironment variable. Setup and teardown then raisedValueError: the environment variable is longer than 32767 characterson Windows only (Linux has no such cap).Fixed by giving each parametrization a short, explicit
pytest.param(..., id=...). The request bodies and assertions are unchanged, so the size guard is still exercised.Verification
Locally on Windows (Python 3.13):
pytest→ 222 passed, 0 errors (previously 221 passed + 2 errors before the id fix).make check: pre-commit, mypy (129 source files), deptry, anduv lock --lockedall clean.CI on this PR additionally covers ubuntu × {3.11, 3.13} and windows × {3.11, 3.13}.
Notes for maintainers
{3.11, 3.13}Python pair intest.yml(build.yml already tested both) and added the OS dimension. Happy to reduce to 3.13-only if you prefer the exact shape proposed in [IMPROVEMENT] Split CI: separate lint from test, paving the way for a Windows test matrix #505.buildtolintandtest.