DRY up GEOS integration

.github/workflows/test.yml

@@ -116,7 +116,7 @@ jobs:
         uses: actions/checkout@v3
       - run: cargo check --all-targets --no-default-features
       # we don't want to test `proj-network` because it only enables the `proj` feature
-      - run: cargo test --features "use-proj use-serde earcutr multithreading"
+      - run: cargo test --features "use-proj use-serde earcutr multithreading geos-tests"
 
   geo_traits:
     name: geo-traits

geo/Cargo.toml

@@ -18,6 +18,7 @@ use-proj = ["proj"]
 proj-network = ["use-proj", "proj/network"]
 use-serde = ["serde", "geo-types/serde"]
 multithreading = ["i_overlay/allow_multithreading", "geo-types/multithreading"]
+geos-tests = ["geos"]
 
 [dependencies]
 earcutr = { version = "0.4.2", optional = true }
@@ -28,6 +29,7 @@ geographiclib-rs = { version = "0.2.3", default-features = false }
 log = "0.4.11"
 num-traits = "0.2"
 proj = { version = "0.27.0", optional = true }
+geos = { version = "9.1.1", features = ["geo"], optional = true }
 robust = "1.1.0"
 rstar = "0.12.0"
 serde = { version = "1.0", optional = true, features = ["derive"] }
@@ -37,14 +39,10 @@ i_overlay = { version = "1.9.0, < 1.10.0", default-features = false }
 approx = ">= 0.4.0, < 0.6.0"
 criterion = { version = "0.4", features = ["html_reports"] }
 geo-test-fixtures = { path = "../geo-test-fixtures" }
-# REVIEW: make optional? remove? It's used for some conformance tests which are somewhat redundant with the JTS tests
-geos = { version = "9.1.1", features = ["geo"] }
 jts-test-runner = { path = "../jts-test-runner" }
 pretty_env_logger = "0.4"
 rand = "0.8.0"
 rand_distr = "0.4.3"
-
-### boolean-ops test deps
 wkt = "0.10.1"
 
 [[bench]]

geo/src/algorithm/validation/geometry_collection.rs

@@ -45,9 +45,7 @@ impl<F: GeoFloat> Validation for GeometryCollection<F> {
 mod tests {
     use super::*;
     use crate::algorithm::validation::{assert_validation_errors, InvalidLineString};
-    use crate::{geometry::*, wkt};
-
-    use geos::Geom;
+    use crate::wkt;
 
     #[test]
     fn test_geometrycollection_contain_invalid_element() {
@@ -58,28 +56,17 @@ mod tests {
                 LINESTRING(0. 0.,0. 0.)
             )
         );
+        // the geos crate doesn't support converting geo::GeometryCollection to -> geos::Geom
         assert_validation_errors!(
             gc,
             vec![InvalidGeometryCollection::InvalidGeometry(
                 GeometryIndex(2),
                 Box::new(InvalidGeometry::InvalidLineString(
                     InvalidLineString::TooFewPoints
                 )),
-            )]
+            )],
+            compare_with_geos: false
         );
-
-        let geoms =
-            gc.0.iter()
-                .map(|geometry| match geometry {
-                    Geometry::Point(pt) => pt.try_into().unwrap(),
-                    Geometry::LineString(ls) => ls.try_into().unwrap(),
-                    _ => unreachable!(),
-                })
-                .collect::<Vec<geos::Geometry>>();
-
-        let geometrycollection_geos: geos::Geometry =
-            geos::Geometry::create_geometry_collection(geoms).unwrap();
-        assert_eq!(gc.is_valid(), geometrycollection_geos.is_valid());
     }
 
     #[test]

geo/src/algorithm/validation/line.rs

@@ -53,18 +53,21 @@ mod tests {
     #[test]
     fn test_line_valid() {
         let l = Line::new((0., 0.), (1., 1.));
-        assert_valid!(l);
+        // GEOS doesn't have a Line type
+        assert_valid!(l, compare_with_geos: false);
     }
 
     #[test]
     fn test_line_invalid_not_finite_coords() {
         let l = Line::new((0., 0.), (f64::NEG_INFINITY, 0.));
-        assert_validation_errors!(l, vec![InvalidLine::NonFiniteCoord(CoordIndex(1))]);
+        // GEOS doesn't have a Line type
+        assert_validation_errors!(l, vec![InvalidLine::NonFiniteCoord(CoordIndex(1))], compare_with_geos: false);
     }
 
     #[test]
     fn test_line_invalid_same_points() {
         let l = Line::new((0., 0.), (0., 0.));
-        assert_validation_errors!(l, vec![InvalidLine::IdenticalCoords]);
+        // GEOS doesn't have a Line type
+        assert_validation_errors!(l, vec![InvalidLine::IdenticalCoords], compare_with_geos: false);
     }
 }

geo/src/algorithm/validation/line_string.rs

@@ -56,44 +56,30 @@ mod tests {
     use super::*;
     use crate::algorithm::validation::{assert_valid, assert_validation_errors};
     use crate::wkt;
-    use geos::Geom;
 
     #[test]
     fn test_linestring_valid() {
         let ls = wkt!(LINESTRING(0. 0., 1. 1.));
-        assert_valid!(ls);
-
-        // Test that the linestring has the same validity status than its GEOS equivalent
-        let linestring_geos: geos::Geometry = (&ls).try_into().unwrap();
-        assert_eq!(ls.is_valid(), linestring_geos.is_valid());
+        assert_valid!(&ls);
     }
 
     #[test]
     fn test_linestring_valid_empty() {
-        let ls = wkt!(LINESTRING EMPTY);
-        assert_valid!(ls);
-
-        let linestring_geos: geos::Geometry = (&ls).try_into().unwrap();
-        assert_eq!(ls.is_valid(), linestring_geos.is_valid());
+        let ls: LineString = wkt!(LINESTRING EMPTY);
+        assert_valid!(&ls);
     }
 
     #[test]
     fn test_linestring_invalid_too_few_points_without_duplicate() {
         let ls = wkt!(LINESTRING(0. 0.));
-        assert_validation_errors!(ls, vec![InvalidLineString::TooFewPoints]);
-
-        // Creating this linestring with geos fails (as soon as its creation is attempted)
-        let linestring_geos: geos::GResult<geos::Geometry> = ls.try_into();
-        assert!(linestring_geos.is_err());
+        // NOTE: Rather than build an invalid LineString GEOS errors at construction time, so
+        // we can't compare with GEOS here.
+        assert_validation_errors!(&ls, vec![InvalidLineString::TooFewPoints], compare_with_geos: false);
     }
 
     #[test]
     fn test_linestring_invalid_too_few_points_with_duplicate() {
         let ls = wkt!(LINESTRING(0. 0.,0. 0.));
-        assert_validation_errors!(ls, vec![InvalidLineString::TooFewPoints]);
-
-        // Test that the linestring has the same validity status than its GEOS equivalent
-        let linestring_geos: geos::Geometry = (&ls).try_into().unwrap();
-        assert_eq!(ls.is_valid(), linestring_geos.is_valid());
+        assert_validation_errors!(&ls, vec![InvalidLineString::TooFewPoints]);
     }
 }

geo/src/algorithm/validation/mod.rs

@@ -131,6 +131,24 @@ pub(crate) use test_macros::*;
 mod test_macros {
     macro_rules! assert_valid {
         ($to_validate:expr) => {
+            assert_valid!(
+                $to_validate,
+                compare_with_geos: true
+            );
+        };
+        ($to_validate:expr, compare_with_geos: true) => {
+            assert_valid!(
+                $to_validate,
+                compare_with_geos: false
+            );
+            #[cfg(feature = "geos-tests")]
+            {
+                use geos::Geom;
+                let geos_geom: geos::Geometry = $to_validate.try_into().unwrap();
+                assert!(geos_geom.is_valid(), "GEOS thought the geometry was invalid");
+            }
+        };
+        ($to_validate:expr, compare_with_geos: false) => {
             assert!(
                 $to_validate.is_valid(),
                 "Validation errors: {:?}",
@@ -142,6 +160,26 @@ mod test_macros {
 
     macro_rules! assert_validation_errors {
         ($to_validate:expr, $errors:expr) => {
+            assert_validation_errors!(
+                $to_validate,
+                $errors,
+                compare_with_geos: true
+            );
+        };
+        ($to_validate:expr, $errors:expr, compare_with_geos: true) => {
+             assert_validation_errors!(
+                $to_validate,
+                $errors,
+                compare_with_geos: false
+            );
+            #[cfg(feature = "geos-tests")]
+            {
+                use geos::Geom;
+                let geos_geom: geos::Geometry = $to_validate.try_into().unwrap();
+                assert!(!geos_geom.is_valid(), "GEOS thought the geometry was valid");
+            }
+        };
+         ($to_validate:expr, $errors:expr, compare_with_geos: false) => {
             assert!(!$to_validate.is_valid());
             assert!(
                 !$errors.is_empty(),

geo/src/algorithm/validation/multi_line_string.rs

@@ -46,7 +46,6 @@ mod tests {
         assert_valid, assert_validation_errors, InvalidLineString, InvalidMultiLineString,
     };
     use crate::wkt;
-    use geos::Geom;
 
     #[test]
     fn test_multilinestring_valid() {
@@ -56,11 +55,7 @@ mod tests {
                 (3. 1.,4. 1.)
             )
         );
-        assert_valid!(mls);
-
-        // Test that the linestring has the same validity status than its GEOS equivalent
-        let multilinestring_geos: geos::Geometry = (&mls).try_into().unwrap();
-        assert_eq!(mls.is_valid(), multilinestring_geos.is_valid());
+        assert_valid!(&mls);
     }
 
     #[test]
@@ -74,15 +69,11 @@ mod tests {
             )
         );
         assert_validation_errors!(
-            mls,
+            &mls,
             vec![InvalidMultiLineString::InvalidLineString(
                 GeometryIndex(1),
                 InvalidLineString::TooFewPoints
             )]
         );
-
-        // Test that the linestring has the same validity status than its GEOS equivalent
-        let multilinestring_geos: geos::Geometry = (&mls).try_into().unwrap();
-        assert_eq!(mls.is_valid(), multilinestring_geos.is_valid());
     }
 }

geo/src/algorithm/validation/multi_point.rs

@@ -42,15 +42,11 @@ mod tests {
     use super::*;
     use crate::algorithm::validation::{assert_valid, assert_validation_errors};
     use crate::{geometry::*, wkt};
-    use geos::Geom;
 
     #[test]
     fn test_multipoint_valid() {
         let mp = wkt!(MULTIPOINT(0. 0.,1. 1.));
-        assert_valid!(mp);
-
-        let multipoint_geos: geos::Geometry = (&mp).try_into().unwrap();
-        assert_eq!(mp.is_valid(), multipoint_geos.is_valid());
+        assert_valid!(&mp);
     }
 
     #[test]
@@ -60,15 +56,11 @@ mod tests {
             Point::new(f64::NAN, 1.),
         ]);
         assert_validation_errors!(
-            mp,
+            &mp,
             vec![
                 InvalidMultiPoint::InvalidPoint(GeometryIndex(0), InvalidPoint::NonFiniteCoord),
                 InvalidMultiPoint::InvalidPoint(GeometryIndex(1), InvalidPoint::NonFiniteCoord)
             ]
         );
-
-        // Test that this multipoint has the same validity status than its GEOS equivalent
-        let multipoint_geos: geos::Geometry = (&mp).try_into().unwrap();
-        assert_eq!(mp.is_valid(), multipoint_geos.is_valid());
     }
 }

geo/src/algorithm/validation/multi_polygon.rs

@@ -82,7 +82,6 @@ mod tests {
     use super::*;
     use crate::algorithm::validation::RingRole;
     use crate::wkt;
-    use geos::Geom;
 
     #[test]
     fn test_multipolygon_invalid() {
@@ -102,7 +101,7 @@ mod tests {
             )
         );
         assert_validation_errors!(
-            multi_polygon,
+            &multi_polygon,
             vec![
                 InvalidMultiPolygon::InvalidPolygon(
                     GeometryIndex(0),
@@ -116,9 +115,5 @@ mod tests {
                 ),
             ]
         );
-
-        // Test that the polygon has the same validity status than its GEOS equivalent
-        let multipolygon_geos: geos::Geometry = (&multi_polygon).try_into().unwrap();
-        assert_eq!(multi_polygon.is_valid(), multipolygon_geos.is_valid());
     }
 }

geo/src/algorithm/validation/point.rs

@@ -36,25 +36,17 @@ mod tests {
     use super::*;
     use crate::algorithm::validation::{assert_valid, assert_validation_errors};
     use crate::Point;
-    use geos::Geom;
 
     #[test]
     fn test_point_valid() {
         let p = Point::new(0., 0.);
         assert_valid!(p);
-
-        // Test that the point has the same validity status than its GEOS equivalent
-        let pt_geos: geos::Geometry = (&p).try_into().unwrap();
-        assert_eq!(p.is_valid(), pt_geos.is_valid());
     }
 
     #[test]
     fn test_point_validation_errors() {
         let p = Point::new(f64::NAN, f64::NAN);
         assert_validation_errors!(p, vec![InvalidPoint::NonFiniteCoord]);
-
-        let pt_geos: geos::Geometry = (&p).try_into().unwrap();
-        assert_eq!(p.is_valid(), pt_geos.is_valid());
     }
 
     #[test]