Expand test coverage #182
Expand test coverage #182
Conversation
WalkthroughThis change introduces test coverage tooling and reporting to the project by updating the Makefile and Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant SummarizeTool
participant Command
participant ToolMessage
Tester->>SummarizeTool: Instantiate summarize tool (with agent name)
Tester->>SummarizeTool: Call summarize tool asynchronously (with summary, tool_call_id)
SummarizeTool->>Command: Create Command object (with summary)
SummarizeTool->>ToolMessage: Create ToolMessage (with content, tool_call_id)
SummarizeTool-->>Tester: Return Command (with ToolMessage)
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pyproject.toml (2)
32-35: Avoid duplicating runtime dependencies indevextra
pytest-asynciois already declared in the maindependencieslist (line 15).
Keeping the same requirement inoptional-dependencies.devis redundant and may lead to version-resolution conflicts if the two specifiers ever diverge.- "pytest-asyncio~=0.26.0",Removing the duplicate keeps the lock-file smaller and guarantees a single source of truth for the version.
107-115: Minor: overlapping omit-globs add noise
"*/tests/*"already blankets the more specific patterns"*/test_*.py"and"*/__pycache__/*".
The duplicates do not break anything, but slimming the list improves readability.No functional change required; feel free to ignore if you prefer explicit duplication.
Makefile (2)
48-54: HTML report generated twice
test-coveragealready produces an HTML report via--cov-report=html.
coverage-reportrerunscoverage html, doubling runtime and possibly overwriting artefacts with different timestamps.Consider either:
-test-coverage: - uv run pytest tests/unit_tests --cov=src --cov-report=html --cov-report=term - -coverage-report: - uv run coverage html - @echo "Coverage report generated in htmlcov/index.html" +test-coverage: + uv run pytest tests/unit_tests --cov=src --cov-report=html --cov-report=term + @echo "HTML coverage report available at htmlcov/index.html" + +# Optional: keep this target only if you anticipate generating reports +# without re-running the test suite.Streamlines CI minutes and local developer feedback.
131-133: Help text refers to non-existentteststargetThe help banner lists:
@echo 'tests - run unit tests'but no
tests:rule exists (onlytest/test-unit). Either add an alias target or drop the line to avoid confusion.tests/unit_tests/common/test_tools.py (1)
228-268: Brittle console-output assertionThe test asserts an exact 42-char underline:
assert "==========================================" in captured.outAny formatting tweak (e.g. 40 or 50
=) will break the test while the tool still behaves correctly.- assert "==========================================" in captured.out + # At least 10 consecutive '=' is enough to prove the banner + assert "=" * 10 in captured.outKeeps intent while future-proofing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Makefile(2 hunks)pyproject.toml(2 hunks)tests/unit_tests/common/test_tools.py(2 hunks)
🔇 Additional comments (3)
tests/unit_tests/common/test_tools.py (3)
5-7: Good choice on explicit type importsBringing in
ToolMessageandCommandkeeps the assertions type-safe and readable.
270-300: Capsys use is fine, but consider flushing once per invocationSuccessive
captured = capsys.readouterr()resets the buffer; if future asserts need both agents’ outputs simultaneously this pattern will hide the first printout.
Not critical now—just a heads-up for maintainability.
302-315: Edge-case test looks solidValidating empty summaries guarantees graceful handling of a common corner case.
| exclude_lines = [ | ||
| "pragma: no cover", | ||
| "def __repr__", | ||
| "if self.debug:", | ||
| "if settings.DEBUG", | ||
| "raise AssertionError", | ||
| "raise NotImplementedError", | ||
| "if 0:", | ||
| "if __name__ == .__main__.:", | ||
| "if TYPE_CHECKING:", | ||
| ] |
There was a problem hiding this comment.
Exclude-line pattern is incorrect – coverage filter will never match
exclude_lines contains
"if __name__ == .__main__.:",
which is missing the quoting around __main__.
- "if __name__ == .__main__.:",
+ "if __name__ == '__main__':",Without the fix, the typical “run as script” guard stays in the coverage report and can artificially drop overall percentages.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exclude_lines = [ | |
| "pragma: no cover", | |
| "def __repr__", | |
| "if self.debug:", | |
| "if settings.DEBUG", | |
| "raise AssertionError", | |
| "raise NotImplementedError", | |
| "if 0:", | |
| "if __name__ == .__main__.:", | |
| "if TYPE_CHECKING:", | |
| ] | |
| exclude_lines = [ | |
| "pragma: no cover", | |
| "def __repr__", | |
| "if self.debug:", | |
| "if settings.DEBUG", | |
| "raise AssertionError", | |
| "raise NotImplementedError", | |
| "if 0:", | |
| "if __name__ == '__main__':", | |
| "if TYPE_CHECKING:", | |
| ] |
🤖 Prompt for AI Agents
In pyproject.toml around lines 120 to 130, the exclude_lines pattern for the
run-as-script guard is incorrect because it lacks proper quoting around
__main__. Fix this by changing the pattern from "if __name__ == .__main__." to
"if __name__ == '__main__':" so the coverage filter correctly excludes this line
from coverage reports.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/checks.yml (1)
66-72: Review Codecov upload configuration
The step usesfail_ci_if_error: false, which will silence upload failures. If coverage reporting is critical, consider enabling strict CI failure or monitoring upload logs.- fail_ci_if_error: false + fail_ci_if_error: true
| - name: Run tests with coverage | ||
| run: make test-coverage |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify test-coverage Makefile target and output
Ensure the Makefile defines a test-coverage target and that it produces a valid coverage report (e.g., coverage.xml) at the repo root. Without it, the Codecov upload will have no data.
🏁 Script executed:
#!/bin/bash
# Check Makefile for test-coverage target
rg -n "^test-coverage:" Makefile
# Confirm coverage.xml generation command exists
rg -n "coverage xml" -g "Makefile" || echo "No 'coverage xml' command found"Length of output: 166
🏁 Script executed:
#!/bin/bash
# Display the test-coverage target and the commands it runs
rg -n '^test-coverage:' -A 10 MakefileLength of output: 346
Add XML coverage output for Codecov
The test-coverage target only generates HTML and terminal reports, so no coverage.xml is produced for Codecov to upload. Please update the Makefile to include XML output:
• File: Makefile (around line 48)
test-coverage:
- uv run pytest tests/unit_tests --cov=src --cov-report=html --cov-report=term
+ uv run pytest tests/unit_tests \
+ --cov=src \
+ --cov-report=html \
+ --cov-report=term \
+ --cov-report=xmlThis will emit coverage.xml at the repo root for the Codecov action.
🤖 Prompt for AI Agents
In .github/workflows/checks.yml at lines 64-65, the workflow runs the Makefile
target test-coverage, but the Makefile does not generate a coverage.xml file
needed for Codecov. Update the Makefile around line 48 to modify the
test-coverage target to include generating an XML coverage report (coverage.xml)
at the repo root, typically by adding a command like coverage xml or equivalent,
so Codecov can upload the coverage data correctly.
Adds coverage tools and additional tests around agent tools
Summary by CodeRabbit