From 7f423cb3f4e4b580af4a78e19c1e89fa199fc2ba Mon Sep 17 00:00:00 2001 From: Jean SIMARD Date: Tue, 3 Dec 2024 21:47:48 +0100 Subject: [PATCH 1/3] editoast: refactor test in a single test with cases All the existing test follow the same pattern. Since we do have `rstest` that allows us to express them as cases of the same test, let's use that. In order to shorten (and, subjectively, make it more readable and concise?), I also implemented a `FromStr` for `TrackRange`. It's a nice helper to express all the tests more concisely. Signed-off-by: Jean SIMARD --- editoast/src/core/pathfinding.rs | 34 +++++++ editoast/src/views/path/projection.rs | 137 +++++++------------------- 2 files changed, 68 insertions(+), 103 deletions(-) diff --git a/editoast/src/core/pathfinding.rs b/editoast/src/core/pathfinding.rs index 7acc24facad..74c5ba738fd 100644 --- a/editoast/src/core/pathfinding.rs +++ b/editoast/src/core/pathfinding.rs @@ -180,6 +180,40 @@ impl From for TrackRange { } } +#[cfg(test)] +impl std::str::FromStr for TrackRange { + type Err = String; + + fn from_str(s: &str) -> Result { + let Some((name, offsets)) = s.split_once('+') else { + return Err(String::from( + "track range must contain at least a '+' and be of the form \"A+12-25\"", + )); + }; + let track_section = Identifier::from(name); + let Some((begin, end)) = offsets.split_once('-') else { + return Err(String::from("track range must contain '-' to separate the offsets and be of the form \"A+12-25\"")); + }; + let Ok(begin) = begin.parse() else { + return Err(format!("{begin} in track range should be an integer")); + }; + let Ok(end) = end.parse() else { + return Err(format!("{end} in track range should be an integer")); + }; + let (begin, end, direction) = if begin < end { + (begin, end, Direction::StartToStop) + } else { + (end, begin, Direction::StopToStart) + }; + Ok(TrackRange { + track_section, + begin, + end, + direction, + }) + } +} + impl TrackRange { #[cfg(test)] /// Creates a new `TrackRange`. diff --git a/editoast/src/views/path/projection.rs b/editoast/src/views/path/projection.rs index 2a50e79d779..9d114837bc6 100644 --- a/editoast/src/views/path/projection.rs +++ b/editoast/src/views/path/projection.rs @@ -407,111 +407,42 @@ mod tests { ); } - #[test] - fn get_boundaries_case_1() { - let path = vec![ - TrackRange::new("A", 50, 100, Direction::StartToStop), - TrackRange::new("B", 0, 200, Direction::StopToStart), - TrackRange::new("C", 0, 300, Direction::StartToStop), - TrackRange::new("D", 120, 250, Direction::StopToStart), - ]; - let projection = PathProjection::new(&path); - - let track_ranges = vec![ - TrackRange::new("A", 0, 100, Direction::StartToStop), - TrackRange::new("B", 0, 200, Direction::StopToStart), - TrackRange::new("C", 0, 300, Direction::StartToStop), - TrackRange::new("D", 0, 250, Direction::StopToStart), - TrackRange::new("E", 0, 100, Direction::StartToStop), - ]; - - let boundaries = projection.get_intersections(&track_ranges); - let expected: Vec = vec![Intersection::from((50, 730))]; - - assert_eq!(boundaries, expected); - } - - #[test] - fn get_boundaries_case_2() { - let path = vec![ - TrackRange::new("A", 50, 100, Direction::StartToStop), - TrackRange::new("B", 0, 200, Direction::StopToStart), - TrackRange::new("C", 0, 300, Direction::StartToStop), - TrackRange::new("D", 0, 250, Direction::StopToStart), - TrackRange::new("E", 25, 100, Direction::StopToStart), - ]; - let projection = PathProjection::new(&path); - - let track_ranges = vec![ - TrackRange::new("X", 0, 100, Direction::StartToStop), - TrackRange::new("B", 0, 200, Direction::StartToStop), - TrackRange::new("C", 150, 200, Direction::StopToStart), - TrackRange::new("E", 30, 100, Direction::StartToStop), - TrackRange::new("Z", 0, 100, Direction::StartToStop), - ]; - - let boundaries = projection.get_intersections(&track_ranges); - let expected: Vec = vec![ - Intersection::from((100, 350)), - Intersection::from((350, 420)), - ]; - - assert_eq!(boundaries, expected); - } - - #[test] - fn get_boundaries_case_3() { - let path = vec![ - TrackRange::new("A", 50, 100, Direction::StartToStop), - TrackRange::new("B", 0, 200, Direction::StopToStart), - TrackRange::new("C", 0, 300, Direction::StartToStop), - TrackRange::new("D", 0, 250, Direction::StopToStart), - TrackRange::new("E", 25, 100, Direction::StopToStart), - ]; - let projection = PathProjection::new(&path); - - let track_ranges = vec![ - TrackRange::new("A", 0, 100, Direction::StartToStop), - TrackRange::new("B", 0, 200, Direction::StartToStop), - TrackRange::new("X", 0, 100, Direction::StartToStop), - TrackRange::new("C", 150, 200, Direction::StopToStart), - TrackRange::new("Z", 0, 100, Direction::StartToStop), - TrackRange::new("E", 30, 100, Direction::StartToStop), - ]; - - let boundaries = projection.get_intersections(&track_ranges); - let expected: Vec = vec![ - Intersection::from((50, 300)), - Intersection::from((400, 450)), - Intersection::from((550, 620)), - ]; - - assert_eq!(boundaries, expected); - } - - #[test] - fn get_boundaries_case_4() { - let path = vec![TrackRange::new("A", 0, 100, Direction::StartToStop)]; - let projection = PathProjection::new(&path); - - let track_ranges = vec![TrackRange::new("B", 0, 100, Direction::StartToStop)]; - - let boundaries = projection.get_intersections(&track_ranges); - - let expected: Vec = vec![]; - assert_eq!(boundaries, expected); - } - - #[test] - fn get_boundaries_case_5() { - let path = vec![TrackRange::new("A", 0, 100, Direction::StartToStop)]; + #[rstest] + // One track on the path + #[case::one_path_different_track(&["A+0-100"], &["B+0-100"], &[])] + #[case::one_path_no_overlap(&["A+0-100"], &["A+100-200"], &[])] + // Complex paths with complex track ranges + #[case::complex_path_one_intersection( + &["A+50-100", "B+200-0", "C+0-300", "D+250-120"], + &["A+0-100", "B+200-0", "C+0-300", "D+250-0", "E+0-100"], + &[(50, 730)] + )] + #[case::complex_path_two_intersections( + &["A+50-100", "B+200-0", "C+0-300", "D+250-0", "E+100-25"], + &["X+0-100", "B+0-200", "C+200-150", "E+30-100", "Z+0-100"], + &[(100, 350), (350, 420)] + )] + #[case::complex_path_three_intersections( + &["A+50-100", "B+200-0", "C+0-300", "D+250-0", "E+100-25"], + &["A+0-100", "B+0-200", "X+0-100", "C+200-150", "Z+0-100", "E+30-100"], + &[(50, 300), (400, 450), (550, 620)] + )] + fn get_intersections( + #[case] path: &[&str], + #[case] track_ranges: &[&str], + #[case] expected_intersections: &[(u64, u64)], + ) { + let path: Vec = path.iter().map(|s| s.parse().unwrap()).collect(); let projection = PathProjection::new(&path); - let track_ranges = vec![TrackRange::new("A", 100, 200, Direction::StartToStop)]; - - let boundaries = projection.get_intersections(&track_ranges); + let track_ranges: Vec = + track_ranges.iter().map(|s| s.parse().unwrap()).collect(); + let intersections = projection.get_intersections(&track_ranges); + let expected_intersections: Vec = expected_intersections + .iter() + .map(|tuple| Intersection::from(*tuple)) + .collect::>(); - let expected: Vec = vec![]; - assert_eq!(boundaries, expected); + assert_eq!(intersections, expected_intersections); } } From 4ae037e5f38077d4b0c877840886cc11cac90191 Mon Sep 17 00:00:00 2001 From: Jean SIMARD Date: Thu, 19 Dec 2024 11:52:58 +0100 Subject: [PATCH 2/3] editoast: add more test for projection It's easy to add tests, let's add some more for this tricky algorithm. Signed-off-by: Jean SIMARD --- editoast/src/views/path/projection.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/editoast/src/views/path/projection.rs b/editoast/src/views/path/projection.rs index 9d114837bc6..56452d03883 100644 --- a/editoast/src/views/path/projection.rs +++ b/editoast/src/views/path/projection.rs @@ -411,6 +411,12 @@ mod tests { // One track on the path #[case::one_path_different_track(&["A+0-100"], &["B+0-100"], &[])] #[case::one_path_no_overlap(&["A+0-100"], &["A+100-200"], &[])] + #[case::one_path_one_simple_intersection(&["A+120-140"], &["A+100-200"], &[(20, 40)])] + #[case::one_path_one_simple_intersection_reverse_on_track_ranges(&["A+140-120"], &["A+100-200"], &[(20, 40)])] + #[case::two_path_merged(&["A+180-200", "B+100-120"], &["A+100-200", "B+100-200"], &[(80, 120)])] + #[case::two_path_not_merged(&["A+180-220", "B+80-120"], &["A+100-200", "B+100-200"], &[(80, 120)])] + #[case::two_path_merged_with_extra_bounds(&["A+180-220", "B+80-120"], &["A+100-200", "B+100-200"], &[(80, 120)])] + #[case::three_path_with_hole(&["A+150-200", "C+100-150"], &["A+100-200", "B+100-200", "C+100-200"], &[(50, 100), (200, 250)])] // Complex paths with complex track ranges #[case::complex_path_one_intersection( &["A+50-100", "B+200-0", "C+0-300", "D+250-120"], From 72eb803e40ff70dd33b26246a2cddad24ec2c648 Mon Sep 17 00:00:00 2001 From: Jean SIMARD Date: Fri, 13 Dec 2024 16:26:15 +0100 Subject: [PATCH 3/3] editoast: increase the number of tests for projection The idea is to tests all projection, and combine all possible directions. So if we have a test on a projection of B (with direction `StartToStop`) on A (with direction `StartToStop`), we can automate also all combinations of directions for A and for B: - when B is reversed, no change in the expected results (direction of the projected track range has no impact) - when A is reversed, we need to calculate all the offset from the end and therefore, all intersections are inverted. Signed-off-by: Jean SIMARD --- .../editoast_schemas/src/infra/direction.rs | 10 +++ editoast/src/views/path/projection.rs | 78 +++++++++++++++++-- 2 files changed, 81 insertions(+), 7 deletions(-) diff --git a/editoast/editoast_schemas/src/infra/direction.rs b/editoast/editoast_schemas/src/infra/direction.rs index 82e5b602a38..c752e7d9b53 100644 --- a/editoast/editoast_schemas/src/infra/direction.rs +++ b/editoast/editoast_schemas/src/infra/direction.rs @@ -22,3 +22,13 @@ impl From for RangeMapDirection { } } } + +impl Direction { + #[must_use] + pub fn toggle(self) -> Self { + match self { + Self::StartToStop => Self::StopToStart, + Self::StopToStart => Self::StartToStop, + } + } +} diff --git a/editoast/src/views/path/projection.rs b/editoast/src/views/path/projection.rs index 56452d03883..eac354235d0 100644 --- a/editoast/src/views/path/projection.rs +++ b/editoast/src/views/path/projection.rs @@ -288,6 +288,7 @@ pub enum TrackLocationFromPath { mod tests { use super::*; use rstest::rstest; + use std::iter::DoubleEndedIterator; #[test] #[should_panic] @@ -407,6 +408,48 @@ mod tests { ); } + // To invert track ranges, we need to get the list of track ranges + // backwards, and toggle the direction for each track range + fn invert_track_ranges( + track_ranges: impl DoubleEndedIterator, + ) -> Vec { + track_ranges + .rev() + .map(|mut track_range| { + track_range.direction = track_range.direction.toggle(); + track_range + }) + .collect() + } + + // To invert the intersection, we need to get the intersection backwards, + // invert each tuple and change the offsets by subtracting them from + // the total length of the projection path. + // + // For example, let's project "A+120-140" on a path "A+100-200", it will + // give the intersection (20, 40). If we invert the projection path (from + // "A+100-200" into "A+200-100"), we then get an intersection (60, 80). + // This new result can be calculated by: + // - calculating the length of the projection path: 200 - 100 = 100 + // - inverting the original tuple: (20, 40) -> (40, 20) + // - subtracting from the length: (100-40, 100-20) = (60, 80) + fn invert_intersections( + intersections: impl DoubleEndedIterator, + path_length: u64, + ) -> Vec { + // If 'track_range' is inverted, then offset of intersections are backwards + intersections + .into_iter() + .rev() + .map(|intersection| { + Intersection::from(( + path_length - intersection.end(), + path_length - intersection.start(), + )) + }) + .collect() + } + #[rstest] // One track on the path #[case::one_path_different_track(&["A+0-100"], &["B+0-100"], &[])] @@ -437,17 +480,38 @@ mod tests { #[case] path: &[&str], #[case] track_ranges: &[&str], #[case] expected_intersections: &[(u64, u64)], + // If we invert the projected track ranges, it doesn't change the intersection + #[values(false, true)] toggle_path: bool, + // If we invert the projection path, the intersections will be backwards + // and the offsets will be subtracted from the total length + #[values(false, true)] toggle_track_ranges: bool, ) { - let path: Vec = path.iter().map(|s| s.parse().unwrap()).collect(); + let path = path.iter().map(|s| s.parse().unwrap()); + let path = if toggle_path { + invert_track_ranges(path) + } else { + path.collect() + }; let projection = PathProjection::new(&path); - let track_ranges: Vec = - track_ranges.iter().map(|s| s.parse().unwrap()).collect(); - let intersections = projection.get_intersections(&track_ranges); - let expected_intersections: Vec = expected_intersections + let track_ranges = track_ranges.iter().map(|s| s.parse().unwrap()); + let track_ranges = if toggle_track_ranges { + invert_track_ranges(track_ranges) + } else { + track_ranges.collect() + }; + let expected_intersections = expected_intersections .iter() - .map(|tuple| Intersection::from(*tuple)) - .collect::>(); + .copied() + .map(Intersection::from); + let expected_intersections = if toggle_track_ranges { + let length: u64 = track_ranges.iter().map(TrackRange::length).sum(); + invert_intersections(expected_intersections, length) + } else { + expected_intersections.collect() + }; + + let intersections = projection.get_intersections(&track_ranges); assert_eq!(intersections, expected_intersections); }