feat: Add basic setup for direct testing of rust code using cargo test
#6
Conversation
WalkthroughThe pull request introduces several enhancements to the project's testing and development infrastructure. A new GitHub Actions workflow job for Cargo testing has been added, and the Changes
Sequence DiagramsequenceDiagram
participant CI as GitHub Actions
participant Workflow as Cargo Test Workflow
participant Test as Test Runner
CI->>Workflow: Trigger workflow
Workflow->>Test: Run cargo test
Test-->>Workflow: Test results
Workflow-->>CI: Workflow completion
Poem
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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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)
crates/triangulation/src/point.rs (1)
305-307: Consider optimizing the vector length calculation.The current implementation using
powi(2)could be slightly optimized by using direct multiplication.- ((p1.x - p2.x).powi(2) + (p1.y - p2.y).powi(2)).sqrt() + let dx = p1.x - p2.x; + let dy = p1.y - p2.y; + (dx * dx + dy * dy).sqrt()crates/bermuda/tests/test_point.rs (1)
11-17: Consider adding more test cases for vector addition.While the current test is good, vector addition would benefit from additional test cases to verify:
- Adding zero vector
- Adding negative vectors
- Adding vectors with larger components
#[rstest] -fn test_vector_add() { +#[case(1.0, 0.0, 1.0, 1.0, 2.0, 1.0)] +#[case(0.0, 0.0, 1.0, 1.0, 1.0, 1.0)] // zero vector +#[case(1.0, 1.0, -1.0, -1.0, 0.0, 0.0)] // negative vector +#[case(10.0, 20.0, 30.0, 40.0, 40.0, 60.0)] // larger components +fn test_vector_add( + #[case] x1: f32, #[case] y1: f32, + #[case] x2: f32, #[case] y2: f32, + #[case] expected_x: f32, #[case] expected_y: f32 +) { assert_eq!( - Point::new(1.0, 0.0) + Vector::new(1.0, 1.0), - Point::new(2.0, 1.0) + Point::new(x1, y1) + Vector::new(x2, y2), + Point::new(expected_x, expected_y) ); }.github/workflows/wheels.yml (1)
59-60: Enhance test command and capture test results.The current test command is basic. Consider adding more comprehensive test options and capturing test results.
Here's a suggested enhancement:
- - name: run tests - run: cargo test + - name: Run cargo tests + run: | + cargo test --all-features --workspace -- -Z unstable-options --format json --report-time > test_results.json + - name: Upload test results + if: always() + uses: actions/upload-artifact@v4 + with: + name: cargo-test-results + path: test_results.json
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/wheels.yml(1 hunks)Cargo.toml(1 hunks)crates/bermuda/Cargo.toml(1 hunks)crates/bermuda/tests/test_point.rs(1 hunks)crates/triangulation/src/path_triangulation.rs(1 hunks)crates/triangulation/src/point.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build wheels on macos-13
- GitHub Check: Build wheels on macos-latest
- GitHub Check: Build wheels on windows-latest
- GitHub Check: Build wheels on ubuntu-latest
🔇 Additional comments (6)
Cargo.toml (1)
17-17: LGTM! Good choice of testing framework.The addition of
rstestis appropriate for implementing parameterized tests, and version pinning helps ensure reproducible builds.crates/triangulation/src/point.rs (1)
286-304: Excellent documentation and implementation!The vector_length function is well-documented with clear examples and implements the Euclidean distance calculation correctly.
crates/triangulation/src/path_triangulation.rs (1)
48-48: Good type clarification!Using the fully qualified
point::Vectortype improves code clarity and prevents potential naming conflicts.crates/bermuda/tests/test_point.rs (1)
4-9: Well-structured test for segment ordering!The test effectively verifies that segments are considered equal regardless of point order, which is crucial for path triangulation.
crates/bermuda/Cargo.toml (1)
16-18: LGTM! Good choice of testing framework.The addition of
rstestas a workspace-inherited dev dependency is a good practice. This framework will enable parameterized testing and make test cases more maintainable..github/workflows/wheels.yml (1)
63-63: LGTM! Good job on job dependencies.The build_wheels job correctly depends on both test jobs, ensuring all tests pass before building.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/wheels.yml (1)
59-59: Update actions/cache to v4.The actions/cache@v3 is outdated. Update to v4 for better performance and security.
Apply this change:
- - uses: actions/cache@v3 + - uses: actions/cache@v4🧰 Tools
🪛 actionlint (1.7.4)
59-59: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
crates/triangulation/src/point.rs (2)
305-308: Consider performance and numerical stability improvements.While the implementation is correct, consider these improvements:
- For performance-critical code paths, you might want to add a
vector_length_squaredfunction to avoid the square root when only comparing distances.- For numerical stability with large coordinates, consider normalizing the values.
Here's a suggested implementation:
pub fn vector_length(p1: Point, p2: Point) -> Coord { let dx = p1.x - p2.x; let dy = p1.y - p2.y; (dx * dx + dy * dy).sqrt() } + +/// Calculates the squared Euclidean distance between two points. +/// This is more efficient than `vector_length` when only comparing distances. +pub fn vector_length_squared(p1: Point, p2: Point) -> Coord { + let dx = p1.x - p2.x; + let dy = p1.y - p2.y; + dx * dx + dy * dy +}
305-308: Consider better integration with existing types.The current implementation has some architectural considerations:
- The name
vector_lengthmight be misleading as it calculates distance between points.- The function might be more discoverable as a method on
Point.Consider these alternatives:
- Rename to
point_distanceordistance_betweenfor clarity- Move the function to
impl Point:impl Point { pub fn new(x: Coord, y: Coord) -> Self { Self { x, y } } + + /// Calculates the Euclidean distance to another point. + pub fn distance_to(&self, other: &Point) -> Coord { + let dx = self.x - other.x; + let dy = self.y - other.y; + (dx * dx + dy * dy).sqrt() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/wheels.yml(1 hunks)crates/bermuda/tests/test_point.rs(1 hunks)crates/triangulation/src/point.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/bermuda/tests/test_point.rs
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/wheels.yml
59-59: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (2)
.github/workflows/wheels.yml (1)
72-72: LGTM! Good dependency chain.The build_wheels job correctly depends on both test and cargo-test jobs, ensuring wheels are only built after all tests pass.
crates/triangulation/src/point.rs (1)
286-304: Well-structured documentation that follows Rust conventions.The documentation is comprehensive and includes all essential sections. The example is clear and demonstrates proper usage.
| cargo-test: | ||
| name: Run cargo tests | ||
| needs: pre-commit | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/cache@v3 | ||
| with: | ||
| path: | | ||
| ~/.cargo/bin/ | ||
| ~/.cargo/registry/index/ | ||
| ~/.cargo/registry/cache/ | ||
| ~/.cargo/git/db/ | ||
| target/ | ||
| key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} | ||
| - name: run tests | ||
| run: cargo test --all-features --workspace | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit Rust toolchain setup.
While the cargo test configuration looks good, it's missing explicit Rust toolchain setup. This could lead to inconsistent builds across different environments.
Add the rust-toolchain setup step:
cargo-test:
name: Run cargo tests
needs: pre-commit
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
+ - uses: dtolnay/rust-toolchain@stable
+ with:
+ components: clippy, rustfmt
- uses: actions/cache@v3
with:
path: |
~/.cargo/bin/
~/.cargo/registry/index/
~/.cargo/registry/cache/
~/.cargo/git/db/
target/
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
- name: run tests
run: cargo test --all-features --workspace📝 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.
| cargo-test: | |
| name: Run cargo tests | |
| needs: pre-commit | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - uses: actions/cache@v3 | |
| with: | |
| path: | | |
| ~/.cargo/bin/ | |
| ~/.cargo/registry/index/ | |
| ~/.cargo/registry/cache/ | |
| ~/.cargo/git/db/ | |
| target/ | |
| key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} | |
| - name: run tests | |
| run: cargo test --all-features --workspace | |
| cargo-test: | |
| name: Run cargo tests | |
| needs: pre-commit | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - uses: dtolnay/rust-toolchain@stable | |
| with: | |
| components: clippy, rustfmt | |
| - uses: actions/cache@v3 | |
| with: | |
| path: | | |
| ~/.cargo/bin/ | |
| ~/.cargo/registry/index/ | |
| ~/.cargo/registry/cache/ | |
| ~/.cargo/git/db/ | |
| target/ | |
| key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} | |
| - name: run tests | |
| run: cargo test --all-features --workspace |
🧰 Tools
🪛 actionlint (1.7.4)
59-59: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| /// Calculates the Euclidean distance between two points. | ||
| /// | ||
| /// # Arguments | ||
| /// * `p1` - The first point. | ||
| /// * `p2` - The second point. | ||
| /// | ||
| /// # Returns | ||
| /// Returns the distance between `p1` and `p2` as a `Coord`. | ||
| /// | ||
| /// # Examples | ||
| /// ``` | ||
| /// use triangulation::point::{Point, vector_length}; | ||
| /// | ||
| /// let point1 = Point { x: 3.0, y: 0.0 }; | ||
| /// let point2 = Point { x: 0.0, y: 4.0 }; | ||
| /// | ||
| /// let distance = vector_length(point1, point2); | ||
| /// assert_eq!(distance, 5.0); | ||
| /// ``` | ||
| pub fn vector_length(p1: Point, p2: Point) -> Coord { | ||
| ((p1.x - p2.x).powi(2) + (p1.y - p2.y).powi(2)).sqrt() | ||
| let dx = p1.x - p2.x; | ||
| let dy = p1.y - p2.y; | ||
| (dx * dx + dy * dy).sqrt() |
There was a problem hiding this comment.
💡 Codebase verification
Additional test coverage needed for vector_length function
The function currently only has a single doctest example. Given its usage in path triangulation calculations, comprehensive testing is essential. Add a dedicated test module with cases for:
- Zero distance between points
- Negative coordinates
- Large coordinate values for numerical stability
- Points with identical coordinates
- Points aligned horizontally and vertically
🔗 Analysis chain
Enhance test coverage.
While the documentation includes a basic test, consider adding tests for:
- Edge cases (zero distance, negative coordinates)
- Integration with Vector operations
- Numerical stability with large coordinates
Let's check the existing test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing tests of the new function
rg -l "vector_length|distance.*test" crates/
Length of output: 127
Script:
#!/bin/bash
# Check content of both files focusing on tests
echo "=== point.rs ==="
rg -B 2 -A 10 "vector_length|#\[test\]" crates/triangulation/src/point.rs
echo -e "\n=== path_triangulation.rs ==="
rg -B 2 -A 10 "vector_length|#\[test\]" crates/triangulation/src/path_triangulation.rs
Length of output: 1643
|
Will improve tests in other PRs |
Created based on #4.
To simplify parallel working on multiple PR by setup test infrastructure in a separate PR.
Summary by CodeRabbit
New Features
Tests
Chores