Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add basic setup for direct testing of rust code using cargo test #6

Merged
merged 3 commits into from
Jan 29, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion .github/workflows/wheels.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,26 @@ jobs:
- name: run test
run: uv run --group test pytest

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

Comment on lines +53 to +70
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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)

build_wheels:
needs: test
needs: [test, cargo-test]
name: Build wheels on ${{ matrix.os }}
runs-on: ${{ matrix.os }}
strategy:
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ repository = "https://github.com/napari/bermuda.git"
triangulation = { path = "crates/triangulation" }
pyo3 = { version = "0.23.3", features = ["extension-module"] }
numpy = "0.23"
rstest = "0.24.0"


[workspace.lints.rust]
Expand Down
3 changes: 3 additions & 0 deletions crates/bermuda/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ pyo3 = { workspace = true }
numpy = { workspace = true }
triangulation = { workspace = true }

[dev-dependencies]
rstest = { workspace = true}

[lib]
name = "_bermuda"
crate-type = ["cdylib"]
28 changes: 28 additions & 0 deletions crates/bermuda/tests/test_point.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
use rstest::rstest;
use triangulation::point::{Point, Segment, Vector};

#[rstest]
fn test_segment_order() {
let s1 = Segment::new(Point::new(0.0, 0.0), Point::new(1.0, 1.0));
let s2 = Segment::new(Point::new(1.0, 1.0), Point::new(0.0, 0.0));
assert_eq!(s1, s2);
}

#[rstest]
#[case::base(1.0, 0.0, 1.0, 1.0, 2.0, 1.0)]
#[case::zero_vector(0.0, 0.0, 1.0, 1.0, 1.0, 1.0)] // zero vector
#[case::negative_vector(1.0, 1.0, -1.0, -1.0, 0.0, 0.0)] // negative vector
#[case::larger_components(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(x1, y1) + Vector::new(x2, y2),
Point::new(expected_x, expected_y)
);
}
3 changes: 1 addition & 2 deletions crates/triangulation/src/path_triangulation.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use crate::point;
use crate::point::Vector;

#[derive(Debug)]
pub struct PathTriangulation {
Expand Down Expand Up @@ -46,7 +45,7 @@ fn add_triangles_for_join(
bevel: bool,
) -> f32 {
let idx = triangles.offsets.len();
let mitter: Vector;
let mitter: point::Vector;
let length = point::vector_length(p2, p3);
let p1_p2_diff_norm = (p2 - p1) / prev_length;
let p2_p3_diff_norm = (p3 - p2) / length;
Expand Down
23 changes: 22 additions & 1 deletion crates/triangulation/src/point.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,27 @@ impl Triangle {
}
}

/// 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()
Comment on lines +286 to +308
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. Edge cases (zero distance, negative coordinates)
  2. Integration with Vector operations
  3. 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

}
Loading