tests: add Python test infrastructure with 30 tests#1
Open
Sats-fierillo wants to merge 1 commit intoFierillo:masterfrom
Open
tests: add Python test infrastructure with 30 tests#1Sats-fierillo wants to merge 1 commit intoFierillo:masterfrom
Sats-fierillo wants to merge 1 commit intoFierillo:masterfrom
Conversation
- Add pytest configuration (pytest.ini) - Add shared fixtures in conftest.py (temp files, sample data) - Add test_prodillo_logic.py: winner determination, treasury calc, round state - Add test_bitcoin_prices.py: price tracking, ATH detection, notifications - Update .gitignore: exclude __pycache__, .venv, .pytest_cache All 30 tests passing.
📝 WalkthroughWalkthroughThis PR establishes comprehensive testing infrastructure by introducing pytest configuration, reusable test fixtures for file I/O and sample data, and two test suites validating Bitcoin price tracking logic and Prodillo game mechanics. The .gitignore was updated to exclude test-related cache directories. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
tests/test_bitcoin_prices.py (3)
6-8: Remove unused import.
jsonis imported but never used since JSON operations usewrite_json/read_jsonfrom conftest.♻️ Proposed fix
-import json import pytest from tests.conftest import write_json, read_json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bitcoin_prices.py` around lines 6 - 8, Remove the unused top-level import "json" from tests/test_bitcoin_prices.py; keep the existing imports for pytest and the helper functions write_json and read_json (from tests.conftest), and run tests to confirm no other references to the json symbol remain.
115-133: Usingfloat('inf')- verify JSON serialization.
float('inf')matches TypeScript'sInfinitysemantically, but note that JSON doesn't support infinity values. If these tests ever write/read this data via JSON, it will fail. Current test doesn't persist this data, so it's fine, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bitcoin_prices.py` around lines 115 - 133, The test uses float('inf') for data["dailyMin"], which is not JSON-serializable and will break if the test later writes/reads this structure via JSON; update test_daily_reset_preserves_ath to use a JSON-safe sentinel (e.g., None or a string like "Infinity") for data["dailyMin"] and adjust the assertions accordingly (reference the test function name test_daily_reset_preserves_ath and the data keys "dailyMin"/"dailyMax"/"bitcoinATH"/"bitcoinMax"/"bitcoinMaxBlock" when making the change).
105-109: Decimal parsing behavior differs from TypeScript.The test uses
split(".")[0]but TypeScript'sparseInt("108500.50")returns108500directly. The test comment says "handled by parseInt" but implements different logic. Consider documenting this difference or aligning the test approach.💡 Clarify the comment or align with JS behavior
def test_parse_decimal_response(self): - """Response with decimals handled by parseInt.""" + """Response with decimals - Python requires explicit truncation (JS parseInt does this automatically).""" mock_response = {"last": "108500.50"} - price = int(mock_response["last"].split(".")[0]) + # JS: parseInt("108500.50") → 108500 + # Python: int(float("108500.50")) → 108500 + price = int(float(mock_response["last"])) assert price == 108500🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bitcoin_prices.py` around lines 105 - 109, The test comment says "handled by parseInt" but the implementation uses split(".")[0], which differs from JS parseInt behavior; update test_parse_decimal_response to either (a) match JS-style parsing by converting the string to a float then to int (e.g., use int(float(mock_response["last"])) or math.trunc(float(...)) for truncation) so price is derived like parseInt("108500.50") would, or (b) change the docstring to explicitly state it uses split-based truncation; reference test_parse_decimal_response, mock_response and price when making the change.tests/test_prodillo_logic.py (3)
24-30: Prefix unused variable with underscore.
winner_idis extracted but never used. Prefix with underscore to indicate intentional non-use.♻️ Proposed fix
- winner_id, winner_name, winner_pred = sorted_preds[0] + _winner_id, winner_name, winner_pred = sorted_preds[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_prodillo_logic.py` around lines 24 - 30, The tuple unpacking extracts winner_id but it's unused; change the unpacking in the block that computes sorted_preds (using predictions and bitcoin_max) to prefix the unused variable with an underscore (e.g., _winner_id or _ ) so linters understand it's intentionally unused; update the line that currently reads "winner_id, winner_name, winner_pred = sorted_preds[0]" to use the underscored identifier and leave winner_name and winner_pred as-is.
6-10: Remove unused import.
jsonis imported but never used since JSON operations usewrite_json/read_jsonfrom conftest.♻️ Proposed fix
-import json import pytest from tests.conftest import write_json, read_json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_prodillo_logic.py` around lines 6 - 10, Remove the unused top-level import of json in tests/test_prodillo_logic.py: locate the import statement "import json" near the top of the file (alongside "import pytest" and "from tests.conftest import write_json, read_json") and delete it since JSON operations use write_json/read_json from conftest and nothing in this module references json.
81-97: Usemath.ceil()for idiomatic Python.The
.__ceil__()dunder method works butmath.ceil()is more idiomatic and matches how production logic would typically be ported.♻️ Proposed fix
Add import at top of file:
import mathThen update the calculations:
def test_seventy_nine_percent_prize(self): """Winner gets 79% of treasury (ceiling).""" treasury = 1000 - prize = int(((treasury) * 0.79).__ceil__()) + prize = math.ceil(treasury * 0.79) assert prize == 790 def test_treasury_rounding(self): """Treasury calculation rounds up.""" treasury = 1001 - prize = int(((treasury) * 0.79).__ceil__()) + prize = math.ceil(treasury * 0.79) assert prize == 791 # 790.79 → 791 def test_zero_treasury(self): """Zero treasury gives zero prize.""" treasury = 0 - prize = int(((treasury) * 0.79).__ceil__()) + prize = math.ceil(treasury * 0.79) assert prize == 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_prodillo_logic.py` around lines 81 - 97, Replace the use of the dunder method __ceil__ with math.ceil for clarity and idiomatic Python: add "import math" at the top of the test file and change the prize calculations in test_seventy_nine_percent_prize, test_treasury_rounding, and test_zero_treasury to use math.ceil(treasury * 0.79) (casting to int only if needed by the assertions) so the tests mirror production-style rounding.tests/conftest.py (1)
71-80: Consider adding explicit encoding for cross-platform consistency.While UTF-8 is the default in Python 3, explicit encoding improves readability and ensures consistent behavior.
♻️ Suggested improvement
def write_json(path, data): """Helper to write JSON to a file.""" - with open(path, "w") as f: + with open(path, "w", encoding="utf-8") as f: json.dump(data, f, indent=2) def read_json(path): """Helper to read JSON from a file.""" - with open(path) as f: + with open(path, encoding="utf-8") as f: return json.load(f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 71 - 80, The helper functions write_json and read_json should explicitly set file encoding to ensure consistent behavior across platforms; update the open(...) calls in write_json and read_json to include encoding="utf-8" (and keep the existing mode "w" for write and default "r" for read) so JSON is written and read using UTF-8 consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 8: The .gitignore currently contains a malformed merged entry
"AGENTS.md.venv/"; update the .gitignore by splitting this into two separate
lines so each pattern is its own entry—e.g., add "AGENTS.md" on one line and
".venv/" on the next—so the AGENTS.md file and the .venv virtual environment
directory are ignored correctly.
In `@tests/test_bitcoin_prices.py`:
- Around line 28-42: The tests reference fields dailyMax/dailyMin which don't
match production; update the test fixture in tests/conftest.py and all
assertions in tests/test_bitcoin_prices.py to use the production names
lastReportedMax and lastReportedMin (matching the BitcoinPriceTracker interface
and src/modules/bitcoinPrices.ts), e.g., replace any read/write/assert of
"dailyMax"/"dailyMin" with "lastReportedMax"/"lastReportedMin" so tests align
with the actual code paths in BitcoinPriceTracker.
In `@tests/test_prodillo_logic.py`:
- Around line 188-194: The test test_json_file_missing currently just compares
{} == {} and doesn't exercise the module's error handling; update it to call the
actual helper/logic (or a small Python surrogate) so it verifies loadValues-like
behavior: within test_json_file_missing use the read_json helper (or implement a
small wrapper that calls the code under test) with prodillos_file and catch
FileNotFoundError (or let the helper return {} on error), then assert the
returned value equals {}; reference test function name test_json_file_missing
and the helper name read_json or the project's loadValues behavior when locating
where to change.
---
Nitpick comments:
In `@tests/conftest.py`:
- Around line 71-80: The helper functions write_json and read_json should
explicitly set file encoding to ensure consistent behavior across platforms;
update the open(...) calls in write_json and read_json to include
encoding="utf-8" (and keep the existing mode "w" for write and default "r" for
read) so JSON is written and read using UTF-8 consistently.
In `@tests/test_bitcoin_prices.py`:
- Around line 6-8: Remove the unused top-level import "json" from
tests/test_bitcoin_prices.py; keep the existing imports for pytest and the
helper functions write_json and read_json (from tests.conftest), and run tests
to confirm no other references to the json symbol remain.
- Around line 115-133: The test uses float('inf') for data["dailyMin"], which is
not JSON-serializable and will break if the test later writes/reads this
structure via JSON; update test_daily_reset_preserves_ath to use a JSON-safe
sentinel (e.g., None or a string like "Infinity") for data["dailyMin"] and
adjust the assertions accordingly (reference the test function name
test_daily_reset_preserves_ath and the data keys
"dailyMin"/"dailyMax"/"bitcoinATH"/"bitcoinMax"/"bitcoinMaxBlock" when making
the change).
- Around line 105-109: The test comment says "handled by parseInt" but the
implementation uses split(".")[0], which differs from JS parseInt behavior;
update test_parse_decimal_response to either (a) match JS-style parsing by
converting the string to a float then to int (e.g., use
int(float(mock_response["last"])) or math.trunc(float(...)) for truncation) so
price is derived like parseInt("108500.50") would, or (b) change the docstring
to explicitly state it uses split-based truncation; reference
test_parse_decimal_response, mock_response and price when making the change.
In `@tests/test_prodillo_logic.py`:
- Around line 24-30: The tuple unpacking extracts winner_id but it's unused;
change the unpacking in the block that computes sorted_preds (using predictions
and bitcoin_max) to prefix the unused variable with an underscore (e.g.,
_winner_id or _ ) so linters understand it's intentionally unused; update the
line that currently reads "winner_id, winner_name, winner_pred =
sorted_preds[0]" to use the underscored identifier and leave winner_name and
winner_pred as-is.
- Around line 6-10: Remove the unused top-level import of json in
tests/test_prodillo_logic.py: locate the import statement "import json" near the
top of the file (alongside "import pytest" and "from tests.conftest import
write_json, read_json") and delete it since JSON operations use
write_json/read_json from conftest and nothing in this module references json.
- Around line 81-97: Replace the use of the dunder method __ceil__ with
math.ceil for clarity and idiomatic Python: add "import math" at the top of the
test file and change the prize calculations in test_seventy_nine_percent_prize,
test_treasury_rounding, and test_zero_treasury to use math.ceil(treasury * 0.79)
(casting to int only if needed by the assertions) so the tests mirror
production-style rounding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad88147f-6bc8-4d11-b3ec-ce9612163e66
📒 Files selected for processing (6)
.gitignorepytest.initests/__init__.pytests/conftest.pytests/test_bitcoin_prices.pytests/test_prodillo_logic.py
| dist/ | ||
| debug/ | ||
| AGENTS.md No newline at end of file | ||
| AGENTS.md.venv/ |
There was a problem hiding this comment.
Malformed gitignore entry - likely a merge artifact.
AGENTS.md.venv/ appears to be two entries accidentally merged together. Based on the PR description mentioning .venv should be ignored, this should be split into separate entries.
🔧 Proposed fix
-AGENTS.md.venv/
+AGENTS.md
+.venv/📝 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.
Suggested change
| AGENTS.md.venv/ | |
| AGENTS.md | |
| .venv/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore at line 8, The .gitignore currently contains a malformed merged
entry "AGENTS.md.venv/"; update the .gitignore by splitting this into two
separate lines so each pattern is its own entry—e.g., add "AGENTS.md" on one
line and ".venv/" on the next—so the AGENTS.md file and the .venv virtual
environment directory are ignored correctly.
Comment on lines
+28
to
+42
| def test_new_daily_max(self, bitcoin_file, sample_bitcoin_data): | ||
| """When price > dailyMax but < ATH, update dailyMax.""" | ||
| write_json(bitcoin_file, sample_bitcoin_data) | ||
| data = read_json(bitcoin_file) | ||
|
|
||
| new_price = 108700 # > dailyMax (108500), < ATH (109000) | ||
| assert new_price > data["dailyMax"] | ||
| assert new_price < data["bitcoinATH"] | ||
|
|
||
| data["dailyMax"] = new_price | ||
| write_json(bitcoin_file, data) | ||
|
|
||
| updated = read_json(bitcoin_file) | ||
| assert updated["dailyMax"] == 108700 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check field names in TypeScript types and implementation
echo "=== BitcoinPriceTracker interface ==="
rg -n "dailyMax|dailyMin|lastReportedMax|lastReportedMin" --type=ts -C2
echo ""
echo "=== Sample fixture in conftest.py ==="
rg -n "dailyMax|dailyMin|lastReportedMax|lastReportedMin" tests/conftest.pyRepository: Fierillo/botillo
Length of output: 3705
🏁 Script executed:
#!/bin/bash
# Check all occurrences of field names in test files
echo "=== All field usages in test_bitcoin_prices.py ==="
cat -n tests/test_bitcoin_prices.py | grep -E "dailyMax|dailyMin|lastReportedMax|lastReportedMin"
echo ""
echo "=== Check if there are other test files referencing these fields ==="
find tests -name "*.py" -type f | xargs grep -l "dailyMax\|dailyMin\|lastReportedMax\|lastReportedMin"Repository: Fierillo/botillo
Length of output: 1417
🏁 Script executed:
#!/bin/bash
# Check the imports and test structure in test_bitcoin_prices.py
echo "=== Test file imports and fixtures ==="
head -30 tests/test_bitcoin_prices.py
echo ""
echo "=== Check if tests use actual functions or mocks ==="
rg -n "def test_|@pytest|mock|Mock" tests/test_bitcoin_prices.py | head -20Repository: Fierillo/botillo
Length of output: 2239
🏁 Script executed:
#!/bin/bash
# Check conftest.py to see the fixture definitions
echo "=== conftest.py content ==="
cat tests/conftest.pyRepository: Fierillo/botillo
Length of output: 1931
Field name mismatch between tests and production code.
Tests use dailyMax/dailyMin in fixtures and assertions, but the TypeScript BitcoinPriceTracker interface (src/modules/types.ts:31-32) and production code (src/modules/bitcoinPrices.ts:17-18) use lastReportedMax/lastReportedMin. Update the fixture in tests/conftest.py and all test assertions to use the correct production field names to ensure tests accurately reflect production behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_bitcoin_prices.py` around lines 28 - 42, The tests reference
fields dailyMax/dailyMin which don't match production; update the test fixture
in tests/conftest.py and all assertions in tests/test_bitcoin_prices.py to use
the production names lastReportedMax and lastReportedMin (matching the
BitcoinPriceTracker interface and src/modules/bitcoinPrices.ts), e.g., replace
any read/write/assert of "dailyMax"/"dailyMin" with
"lastReportedMax"/"lastReportedMin" so tests align with the actual code paths in
BitcoinPriceTracker.
Comment on lines
+188
to
+194
| def test_json_file_missing(self, prodillos_file): | ||
| """Reading non-existent file returns empty dict.""" | ||
| import os | ||
| assert not os.path.exists(prodillos_file) | ||
| # This is what the TS loadValues does on error | ||
| data = {} | ||
| assert data == {} |
There was a problem hiding this comment.
Test doesn't verify actual error handling behavior.
This test asserts that an empty dict equals an empty dict, but doesn't actually test the loadValues error handling. Per src/modules/utils.ts:23-31, loadValues() returns {} as T on error—this test should verify that behavior via a mock or by testing the actual helper.
💡 Consider either removing or making it meaningful
Option 1 - Remove if not adding value:
- def test_json_file_missing(self, prodillos_file):
- """Reading non-existent file returns empty dict."""
- import os
- assert not os.path.exists(prodillos_file)
- # This is what the TS loadValues does on error
- data = {}
- assert data == {}Option 2 - Add a Python helper that mimics loadValues behavior and test it:
def test_json_file_missing(self, prodillos_file):
"""Reading non-existent file returns empty dict."""
import os
assert not os.path.exists(prodillos_file)
# Test a helper that mimics TS loadValues error handling
try:
data = read_json(prodillos_file)
except FileNotFoundError:
data = {}
assert data == {}📝 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.
Suggested change
| def test_json_file_missing(self, prodillos_file): | |
| """Reading non-existent file returns empty dict.""" | |
| import os | |
| assert not os.path.exists(prodillos_file) | |
| # This is what the TS loadValues does on error | |
| data = {} | |
| assert data == {} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_prodillo_logic.py` around lines 188 - 194, The test
test_json_file_missing currently just compares {} == {} and doesn't exercise the
module's error handling; update it to call the actual helper/logic (or a small
Python surrogate) so it verifies loadValues-like behavior: within
test_json_file_missing use the read_json helper (or implement a small wrapper
that calls the code under test) with prodillos_file and catch FileNotFoundError
(or let the helper return {} on error), then assert the returned value equals
{}; reference test function name test_json_file_missing and the helper name
read_json or the project's loadValues behavior when locating where to change.
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
1 participant
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.
What
Add Python test infrastructure using pytest.
Tests (30 passing)
test_prodillo_logic.py (17 tests):
test_bitcoin_prices.py (13 tests):
How to run
Next PRs (atomic, one at a time)
anytypesSummary by CodeRabbit
Tests
Chores