feat: add tests for Python package; bump min Python version; run in CI #172
Conversation
WalkthroughA new GitHub Actions workflow named Changes
Poem
Warning Rate limit exceeded@sd2k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Deploying augurs with
|
| Latest commit: |
1a876bf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ef0c7c16.augurs.pages.dev |
| Branch Preview URL: | https://python-tests.augurs.pages.dev |
sd2k
force-pushed
the
python-tests
branch
5 times, most recently
from
17858e5 to
ea87fe1
Compare
|
Marking as draft until further tests are added, contributions welcome |
![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
🧹 Outside diff range and nitpick comments (2)
crates/pyaugurs/pyproject.toml (1)
18-21: Consider relaxing pytest version constraintThe pytest version constraint
>=8.3.3is quite strict and recent. Consider using a more flexible version range like>=7.0.0,<9.0.0to:
- Allow for wider compatibility
- Reduce potential dependency conflicts
- Still maintain modern pytest features
[dependency-groups] dev = [ - "pytest>=8.3.3", + "pytest>=7.0.0,<9.0.0", ]crates/pyaugurs/tests/test_dtw.py (1)
8-21: Add test IDs and edge cases for better test coverage.While the parameterized tests are well structured, consider these improvements:
- Add test IDs for better test reporting
- Include edge cases for more robust testing
Consider this enhancement:
@pytest.mark.parametrize( - "opts, input, expected", + "opts, input, expected, test_id", [ ({}, [[0.0, 1.0, 2.0], [3.0, 4.0, 5.0]], 5.0990195135926845), ({"window": 2}, [[0.0, 1.0, 2.0], [3.0, 4.0, 5.0]], 5.0990195135926845), ({"distance_fn": "euclidean"}, [[0.0, 1.0, 2.0], [3.0, 4.0, 5.0]], 5.0990195135926845), ({"distance_fn": "manhattan"}, [[0.0, 1.0, 2.0], [3.0, 4.0, 5.0]], 9), + # Edge cases + ({}, [[], []], 0.0, "empty_arrays"), + ({}, [[1.0], [1.0, 2.0]], pytest.raises(ValueError), "different_lengths"), + ({"distance_fn": "invalid"}, [[0.0], [1.0]], pytest.raises(ValueError), "invalid_distance_fn"), ], + ids=lambda x: x[-1] if isinstance(x, tuple) and len(x) > 3 else None ) -def test_distance(self, opts, input, expected): +def test_distance(self, opts, input, expected, test_id=None): d = Dtw(**opts) arrays = [np.array(x) for x in input] - np.testing.assert_allclose(d.distance(*arrays), expected) + if isinstance(expected, float): + np.testing.assert_allclose(d.distance(*arrays), expected) + else: + with expected: + d.distance(*arrays)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
crates/pyaugurs/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/pytest.yml(1 hunks)crates/pyaugurs/.gitignore(1 hunks)crates/pyaugurs/pyproject.toml(2 hunks)crates/pyaugurs/tests/test_dtw.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/pyaugurs/.gitignore
🧰 Additional context used
🪛 yamllint
.github/workflows/pytest.yml
[error] 7-7: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
crates/pyaugurs/tests/test_dtw.py (2)
1-7: LGTM! Imports and class structure are well-organized.
The imports are appropriate, and the test class follows pytest naming conventions.
22-48: Verify return type and add more test cases for distance_matrix.
The current tests cover basic functionality, but consider:
- Testing with different sized input sets
- Verifying asymmetric distance functions
- Documenting the expected return type (appears to be a DataFrame)
Let's verify the return type and available distance functions:
Consider adding these test cases:
# Additional test cases to consider adding:
(
{"distance_fn": "custom_asymmetric"},
[[0.0, 1.0], [2.0, 3.0]],
[
[0.0, 2.0],
[3.0, 0.0],
]
),
(
{},
[[0.0], [1.0], [2.0], [3.0]], # Test with more sequences
# Expected 4x4 matrix
),
sd2k
added
good first issue
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/pytest.yml (1)
3-7: Fix trailing whitespace on line 7The workflow triggers look good, but there's a trailing whitespace that should be removed.
pull_request: - +🧰 Tools
🪛 yamllint
[error] 7-7: trailing spaces
(trailing-spaces)
crates/pyaugurs/tests/test_dtw.py (2)
8-21: Consider adding more test cases for edge scenarios.The current test implementation is good, with proper parameterization and floating-point comparison. However, consider adding test cases for:
- Empty arrays
- Arrays of different lengths
- Invalid distance functions
- Invalid window sizes
- NaN/Inf values
Example additional test cases:
@pytest.mark.parametrize( "opts, input, expected_error", [ ({}, [[], []], ValueError), # Empty arrays ({}, [[0.0], [0.0, 1.0]], ValueError), # Different lengths ({"distance_fn": "invalid"}, [[0.0], [1.0]], ValueError), # Invalid distance ({"window": -1}, [[0.0], [1.0]], ValueError), # Invalid window ], ) def test_distance_errors(self, opts, input, expected_error): d = Dtw(**opts) arrays = [np.array(x) for x in input] with pytest.raises(expected_error): d.distance(*arrays)
22-48: Consider adding matrix property validations.While the current tests verify the distance matrix values, consider adding explicit checks for important matrix properties:
- Symmetry (M[i,j] == M[j,i])
- Zero diagonal (M[i,i] == 0)
- Non-negativity (M[i,j] >= 0)
Example additional validation:
def test_distance_matrix_properties(self): d = Dtw() arrays = [np.array([i, i+1]) for i in range(3)] matrix = d.distance_matrix(arrays).to_numpy() # Test symmetry np.testing.assert_allclose(matrix, matrix.T) # Test zero diagonal np.testing.assert_allclose(np.diag(matrix), 0) # Test non-negativity assert np.all(matrix >= 0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
crates/pyaugurs/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/pytest.yml(1 hunks)crates/pyaugurs/.gitignore(1 hunks)crates/pyaugurs/pyproject.toml(2 hunks)crates/pyaugurs/tests/test_dtw.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/pyaugurs/.gitignore
🧰 Additional context used
🪛 yamllint
.github/workflows/pytest.yml
[error] 7-7: trailing spaces
(trailing-spaces)
🔇 Additional comments (8)
crates/pyaugurs/pyproject.toml (1)
18-21:
Verify pytest version constraint
The pytest version constraint >=8.3.3 seems unusually high, as the latest stable version of pytest is in the 7.x series.
Let's verify the pytest version:
The version constraint appears to be incorrect as pytest 8.3.3 doesn't exist yet. Consider:
- Using a stable version constraint like
pytest>=7.0.0,<8.0.0 - Or if you need a specific minimum version, use the latest stable version
[dependency-groups]
dev = [
- "pytest>=8.3.3",
+ "pytest>=7.0.0,<8.0.0",
]✅ Verification successful
Pytest version constraint is correct
The version constraint pytest>=8.3.3 is actually correct. The verification shows that:
- Pytest 8.3.3 exists and is the latest version available on PyPI
- It's a valid release with 2 distribution files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check latest pytest versions and compatibility
# Check PyPI for latest pytest versions
echo "Latest pytest version:"
curl -s https://pypi.org/pypi/pytest/json | jq -r '.info.version'
# Check release date of specified version
echo "Checking release date and compatibility of 8.3.3..."
curl -s https://pypi.org/pypi/pytest/8.3.3/json 2>/dev/null | jq -r '.info.release_date'
Length of output: 844
Script:
#!/bin/bash
# Let's verify the pytest version more thoroughly
echo "Checking pytest version history:"
curl -s https://pypi.org/pypi/pytest/json | jq -r '.releases | keys[]' | sort -V | tail -n 5
echo -e "\nChecking if version 8.3.3 exists in PyPI releases:"
curl -s https://pypi.org/pypi/pytest/json | jq -r '.releases["8.3.3"] | length'
echo -e "\nChecking project's minimum Python version:"
cat pyproject.toml | grep "python_version"
Length of output: 594
.github/workflows/pytest.yml (4)
8-12: LGTM: Job configuration is appropriate
The job configuration is well-structured, using the latest Ubuntu runner which is suitable for Python testing.
22-29: LGTM: Build configuration is well optimized
The build step is well configured with:
- Appropriate target architecture
- Build caching via sccache
- Correct working directory
30-33: Verify test directory structure
The test configuration looks good, but let's verify the test directory structure is correct.
#!/bin/bash
# Description: Verify test directory structure
# Expected: Should find pytest files in the specified directory
echo "Checking test directory structure:"
fd -t f -e py . crates/pyaugurs/tests
echo "Checking for pytest configuration:"
fd -t f "pytest.ini|conftest.py" crates/pyaugurs13-21: Verify Python version compatibility
The setup steps look good, using recent versions of all actions. However, let's verify the Python version specified in the pyproject.toml matches our requirements.
✅ Verification successful
Python version requirement confirmed
The setup is correctly configured. The pyproject.toml specifies Python >=3.9 which is a modern and well-supported version. The workflow's use of python-version-file to source this requirement ensures consistency between the project's requirements and CI environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Python version requirements
# Expected: Should find Python version >= 3.9 as mentioned in the AI summary
echo "Checking Python version requirement in pyproject.toml:"
cat crates/pyaugurs/pyproject.toml | rg "python_version|python-version"
Length of output: 282
Script:
#!/bin/bash
# Let's check the content of pyproject.toml directly
cat crates/pyaugurs/pyproject.toml
Length of output: 478
crates/pyaugurs/tests/test_dtw.py (3)
1-5: LGTM! Well-organized imports following PEP 8.
7-7: LGTM! Class follows pytest naming conventions.
1-48: Verify test coverage of all Dtw class methods.
Let's ensure we have test coverage for all public methods of the Dtw class.
✅ Verification successful
Let me try to find the correct path for the Dtw implementation.
Let me try to find the Dtw implementation by looking at the import statement in the test file.
Let me check the content of the augurs.pyi file which contains the Dtw class definition.
Test coverage is complete for Dtw class
The test coverage is comprehensive as all public methods of the Dtw class are tested:
distance()method is tested with various options (default, window, euclidean/manhattan distance)distance_matrix()method is tested with both euclidean and manhattan distance functions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all public methods in Dtw class
echo "Public methods in Dtw class:"
ast-grep --pattern 'class Dtw:
$$$
def $_($$$):
$$$' crates/pyaugurs/augurs/*.py
echo -e "\nMethods with tests:"
ast-grep --pattern 'def test_$_($$$):
$$$' crates/pyaugurs/tests/test_dtw.py
Length of output: 372
Script:
#!/bin/bash
# Find the Dtw implementation file
echo "Looking for Dtw implementation:"
fd -e py -e pyi "dtw"
echo -e "\nChecking content of found files:"
fd -e py -e pyi "dtw" --exec cat {}
Length of output: 1892
Script:
#!/bin/bash
# Find the augurs package and Dtw implementation
echo "Looking for augurs package:"
fd -t d "augurs"
echo -e "\nLooking for Dtw in Python files:"
rg -l "class Dtw"
echo -e "\nChecking Python files in crates/pyaugurs:"
fd -e py . crates/pyaugurs --exec cat {}
Length of output: 2601
Script:
#!/bin/bash
echo "Content of augurs.pyi:"
cat crates/pyaugurs/augurs.pyi
Length of output: 3387
Summary by CodeRabbit
New Features
Dtwclass, improving coverage on distance calculations.Bug Fixes
Chores
__pycache__to the.gitignorefile to prevent tracking of compiled Python files.