feat(test): Add pytest and unit test structure for spider-py; Rename existing C++ test tasks. - #186
Conversation
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
python/spider-py/pyproject.toml (1)
16-18: Fix console script target: package path isspider, notspider_py.The installed entry point will fail at runtime because the module path doesn’t exist. The source tree defines
src/spider/..., so the console script should targetspider.task_executor.task_executor:main.Apply this diff:
[project.scripts] -spider_task_executor = "spider_py.task_executor.task_executor:main" +spider_task_executor = "spider.task_executor.task_executor:main"
🧹 Nitpick comments (5)
python/spider-py/pyproject.toml (2)
37-39: pytest configtestpathsis appropriate for this src/ layout.Minor suggestion: consider adding
addopts = "-ra"for concise reporting of skipped/xfailed, if desired.If you want this, extend the config:
[tool.pytest.ini_options] testpaths = ["tests"] +addopts = "-ra"
71-77: Per-file ignores for tests look sensible.Allowing asserts, print, and subprocess usage in tests is reasonable. Keep an eye on S603: ensure subprocess inputs are controlled.
python/spider-py/src/spider/core/task.py (1)
4-5: Skeleton Task class: LGTM for initial scaffolding.No functional behaviour yet, which matches the “sample” nature of this PR.
If you want to future-proof typings without behaviour, you could add a
passfor clarity, but it’s not required since the docstring is a valid body.python/spider-py/tests/core/test_task.py (1)
6-9: Test is fine as a smoke check.Optionally strengthen the assertion to also validate type.
For example:
- task = Task() - assert task is not None + task = Task() + assert task is not None + assert isinstance(task, Task)python/spider-py/src/spider/task_executor/task_executor.py (1)
4-5: Entry point is a no-op; consider a minimal message or TODO.As-is, the console script will exit immediately. For developer feedback, print a stub message or raise
NotImplementedError.Example:
def main() -> None: - """Main function to execute the task.""" + """Main function to execute the task.""" + # TODO: Implement task execution wiring. + print("spider_task_executor: not implemented yet")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
python/spider-py/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/workflows/code-linting-checks.yaml(1 hunks)lint-tasks.yaml(1 hunks)python/spider-py/pyproject.toml(3 hunks)python/spider-py/src/spider/__init__.py(1 hunks)python/spider-py/src/spider/client/__init__.py(1 hunks)python/spider-py/src/spider/core/__init__.py(1 hunks)python/spider-py/src/spider/core/task.py(1 hunks)python/spider-py/src/spider/task_executor/__init__.py(1 hunks)python/spider-py/src/spider/task_executor/task_executor.py(1 hunks)python/spider-py/tests/core/test_task.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- python/spider-py/src/spider/core/init.py
- python/spider-py/src/spider/init.py
- python/spider-py/src/spider/client/init.py
- python/spider-py/src/spider/task_executor/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- lint-tasks.yaml
- .github/workflows/code-linting-checks.yaml
🧰 Additional context used
🧬 Code Graph Analysis (1)
python/spider-py/tests/core/test_task.py (1)
python/spider-py/src/spider/core/task.py (1)
Task(4-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (1)
python/spider-py/pyproject.toml (1)
22-22: Adding pytest as a dev dependency: LGTM.Good choice and version floor. Matches the stated PR objective to introduce Python tests.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
We have too many task files in the repo's root. I will create a PR later to structure them into a task directory like we did in CLP main.
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/src/dev-docs/testing.md (1)
33-38: Good rename to language-scoped test tasks; consider noting uv as a prerequisite for Python testsThe table is clear and aligns with the new cpp- and spider-py- naming scheme. It matches the task names introduced in test-tasks.yaml.
Minor suggestion: add a brief note that test:spider-py-unit-tests requires uv to be installed (for local devs who may not have it), or link to an install snippet.
test-tasks.yaml (1)
21-26: cpp-unit-tests naming resolves prior ambiguityThis addresses the earlier concern about “all” being misleading. The new name is clearer.
🧹 Nitpick comments (1)
test-tasks.yaml (1)
34-49: Use variables instead of hard-coded venv paths; optionally switch to cmds for consistencyHard-coding ../test-venv makes this brittle. Prefer using G_TEST_VENV_DIR, which you’ve already defined. Also, elsewhere you use cmds; using cmds here improves readability.
Suggested change:
- cpp-integration: + cpp-integration: dir: "{{.G_BUILD_SPIDER_DIR}}" deps: - "venv" - task: ":build:cpp-target" vars: TARGETS: [ "spider_task_executor", "worker_test", "client_test", "spider_worker", "spider_scheduler", "integrationTest"] - cmd: |- - . ../test-venv/bin/activate - ../test-venv/bin/pytest tests/integration + cmds: + - ". {{.G_TEST_VENV_DIR}}/bin/activate" + - "{{.G_TEST_VENV_DIR}}/bin/pytest tests/integration"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/unit-tests.yaml(1 hunks)docs/src/dev-docs/testing.md(2 hunks)test-tasks.yaml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/unit-tests.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
🔇 Additional comments (3)
docs/src/dev-docs/testing.md (1)
56-58: Integration task rename looks consistenttest:cpp-integration matches the task rename in test-tasks.yaml and keeps C++ integration tests distinct from unit tests.
test-tasks.yaml (2)
9-14: Non-storage C++ unit test task LGTMThe Catch2-style filter is correctly quoted to avoid shell globbing, and the dependency on build-unit-test is preserved.
15-20: Storage C++ unit test task LGTMNaming and invocation are consistent with the non-storage task; dependencies are correct.
| spider-py-unit-tests: | ||
| dir: "{{.G_SRC_PYTHON_DIR}}" | ||
| env: | ||
| # Don't create __pycache__ directories in the source tree. | ||
| PYTHONDONTWRITEBYTECODE: "1" | ||
| cmds: | ||
| - "uv run pytest" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Python unit tests task: add pytest flags for signal/noise; verify uv availability in CI and docs
- Optional: add -q -ra to make CI output concise yet informative.
- Ensure uv is reliably installed in CI and documented for local use (docs mention the task but not the prerequisite).
Apply (optional) flags:
- - "uv run pytest"
+ - "uv run pytest -q -ra"Verification script to confirm uv installation steps exist and that G_SRC_PYTHON_DIR is defined:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Confirm tasks/workflows invoke uv and pytest"
rg -n -C2 -P '\buv\s+run\b|\buv\s+--version\b|\bpytest\b' . -g '!**/node_modules/**'
echo
echo "2) Search for uv installation steps in workflows"
rg -n -C2 -P '(astral-sh/setup-uv|pip\s+install\s+uv\b|curl\s+.*astral\.sh/uv/install)' .github/workflows -g '!**/node_modules/**' || {
echo "WARN: No explicit uv installation step found in workflows."
}
echo
echo "3) Check that G_SRC_PYTHON_DIR is defined somewhere (Taskfiles, env, or includes)"
rg -n -C2 -P '\bG_SRC_PYTHON_DIR\b' . -g '!**/node_modules/**' || {
echo "WARN: G_SRC_PYTHON_DIR not found—ensure it is defined in a parent Taskfile/vars."
}Length of output: 10151
Add pytest flags; install uv CLI in CI and document prerequisite
- test-tasks.yaml (around lines 84–86): add
-q -rato the pytest invocation- - "uv run pytest" + - "uv run pytest -q -ra"
- .github/workflows/*: insert an Install uv CLI step before running tests, for example:
- name: Install uv CLI run: pip install uv
- README.md (or other setup docs): explicitly list uv CLI as a prerequisite for local development.
G_SRC_PYTHON_DIR is already defined in taskfile.yaml—no changes required there.
🤖 Prompt for AI Agents
In test-tasks.yaml around lines 80 to 86, the pytest invocation should include
the recommended flags and CI must ensure the uv CLI is available: change the
pytest command to include "-q -ra" (i.e., "uv run pytest -q -ra"); update your
GitHub workflow files to add a step before running tests that installs the uv
CLI (e.g., run "pip install uv"); and add a short note to README.md (or
development setup docs) listing "uv CLI" as a prerequisite for local development
and CI to ensure contributors know to install it.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(test): Add pytest and unit test structure for spider-py; Rename existing C++ test tasks.
2 participants
Description
Note
This PR depends on #179.
This PR:
pytestas Python dev dependency.pytestand renames C++ test tasks.Checklist
breaking change.
Validation performed
non-storage-unit-testsworkflow executes python test tasks.Summary by CodeRabbit
Tests
Documentation
Chores