feat: setup super basic python testing and GHA to run them#58
feat: setup super basic python testing and GHA to run them#58westonplatter wants to merge 4 commits intoMaciek-roboblog:mainfrom
Conversation
WalkthroughA continuous integration workflow was added to run tests on pull requests using GitHub Actions. The Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub
participant CI Runner
Developer->>GitHub: Open Pull Request
GitHub->>CI Runner: Trigger CI workflow (tests.yml)
CI Runner->>CI Runner: Checkout code
CI Runner->>CI Runner: Install dependencies (uv sync)
CI Runner->>CI Runner: Run tests (pytest)
CI Runner->>GitHub: Report test results
Poem
✨ Finishing Touches
🪧 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
Documentation and Community
|
![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 (3)
.github/workflows/tests.yml (1)
3-4: Fix trailing whitespace formatting issue.Static analysis detected trailing spaces that should be removed for clean formatting.
-on: +on:tests/test_calculator.py (2)
15-30: Consider refactoring helper method to reduce argument count.The helper method has 6 parameters, which exceeds the typical limit of 5. Consider using a builder pattern or dataclass for better maintainability.
+from dataclasses import dataclass + +@dataclass +class SessionBlockParams: + input_tokens: int + output_tokens: int + duration_minutes: int + cost_usd: float = 1.0 + is_active: bool = True + - def create_session_block(self, input_tokens, output_tokens, duration_minutes, cost_usd=1.0, is_active=True): + def create_session_block(self, params: SessionBlockParams): """Create test session block. Cache tokens are always ignored in calculations.""" return SessionBlock( id="test-session", start_time=self.base_time, end_time=self.base_time + timedelta(hours=5), - actual_end_time=self.base_time + timedelta(minutes=duration_minutes), - is_active=is_active, + actual_end_time=self.base_time + timedelta(minutes=params.duration_minutes), + is_active=params.is_active, token_counts=TokenCounts( - input_tokens=input_tokens, - output_tokens=output_tokens, + input_tokens=params.input_tokens, + output_tokens=params.output_tokens, cache_creation_tokens=999, # Should be ignored cache_read_tokens=999 # Should be ignored ), - cost_usd=cost_usd + cost_usd=params.cost_usd )
1-58: Clean up formatting issues throughout the file.Static analysis detected multiple whitespace and trailing space issues that should be addressed for code quality.
The following lines need formatting cleanup:
- Remove whitespace from blank lines: 10, 14, 31, 40, 42, 45, 54, 56
- Remove trailing whitespace: 35, 36, 37, 49, 50
Apply these fixes to maintain clean formatting standards.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/tests.yml(1 hunks)pyproject.toml(2 hunks)tests/test_calculator.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_calculator.py (2)
usage_analyzer/core/calculator.py (2)
BurnRateCalculator(13-66)calculate_burn_rate(16-33)usage_analyzer/models/data_structures.py (3)
SessionBlock(53-84)TokenCounts(29-47)duration_minutes(77-84)
🪛 YAMLlint (1.37.1)
.github/workflows/tests.yml
[error] 3-3: trailing spaces
(trailing-spaces)
🪛 Ruff (0.11.9)
tests/test_calculator.py
10-10: Blank line contains whitespace
Remove whitespace from blank line
(W293)
14-14: Blank line contains whitespace
Remove whitespace from blank line
(W293)
31-31: Blank line contains whitespace
Remove whitespace from blank line
(W293)
35-35: Trailing whitespace
Remove trailing whitespace
(W291)
36-36: Trailing whitespace
Remove trailing whitespace
(W291)
37-37: Trailing whitespace
Remove trailing whitespace
(W291)
40-40: Blank line contains whitespace
Remove whitespace from blank line
(W293)
42-42: Blank line contains whitespace
Remove whitespace from blank line
(W293)
45-45: Blank line contains whitespace
Remove whitespace from blank line
(W293)
49-49: Trailing whitespace
Remove trailing whitespace
(W291)
50-50: Trailing whitespace
Remove trailing whitespace
(W291)
54-54: Blank line contains whitespace
Remove whitespace from blank line
(W293)
56-56: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🪛 Pylint (3.3.7)
tests/test_calculator.py
[refactor] 15-15: Too many arguments (6/5)
(R0913)
[refactor] 15-15: Too many positional arguments (6/5)
(R0917)
🔇 Additional comments (3)
.github/workflows/tests.yml (1)
1-22: LGTM! Well-structured CI workflow.The workflow follows modern best practices using
uvfor fast dependency management and includes appropriate caching. The job structure is clean and efficient.tests/test_calculator.py (2)
32-44: LGTM! Test logic is mathematically correct.The test correctly verifies:
- Token calculation: (1000 + 500) / 30 = 50 tokens/minute ✓
- Cost calculation: 1.50 / 30 × 60 = 3.0 $/hour ✓
This aligns with the implementation in
usage_analyzer/core/calculator.pylines 20-26.
46-57: LGTM! Correctly verifies cache token exclusion.The test properly confirms that cache tokens (set to 999 each) are ignored in burn rate calculations, using only input + output tokens as expected from the implementation.
| "ruff>=0.8.0", | ||
| "pre-commit>=2.20.0; python_version<'3.8'", | ||
| "pre-commit>=3.0.0; python_version>='3.8'", | ||
| "pytest>=6.0.0", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate redundant pytest dependencies to avoid version conflicts.
The pytest dependency is declared in two places with different version requirements:
- Line 45:
pytest>=6.0.0in[project.optional-dependencies].dev - Lines 77-80:
pytest>=8.3.5in[dependency-groups].dev
This redundancy could lead to confusion about which version requirement applies.
Choose one approach and remove the other. Since [dependency-groups] is the newer PEP 735 standard, consider keeping that and removing from the optional-dependencies:
- "pytest>=6.0.0",Or alternatively, remove the newer [dependency-groups] section if you prefer the traditional approach.
Also applies to: 77-80
🤖 Prompt for AI Agents
In pyproject.toml at lines 45 and 77-80, there are redundant pytest dependency
declarations with conflicting versions. To fix this, choose either the
traditional `[project.optional-dependencies].dev` section or the newer
`[dependency-groups].dev` section to declare pytest, and remove the other to
avoid confusion and version conflicts. Preferably keep the
`[dependency-groups].dev` declaration with `pytest>=8.3.5` and remove the
`pytest>=6.0.0` entry from `[project.optional-dependencies].dev`.
|
@westonplatter check new version :D |
|
@Maciek-roboblog great, I'll take a look! |
2 participants
Changes
Summary by CodeRabbit
Chores
Tests