add macro to test all implementations #135

rafaqz · 2024-05-06T13:20:10Z

WIP conversion of all tests to use our new @test_all_implementations macro to make sure all geometries work everywhere.

@asinghvi17 I added some helpers for nicer testset titles that combine the topic and the module name.

Closes #134

(probably doesn't work currently !)

TODO: There is no GI.convert for features and feature collections so we cant pass features into the macro. We should add it to GeoInterface with a fallback that just converts the geometries and rewraps them with GI Feature.

asinghvi17

Looks good, but every test file will have to include("helper.jl") because they're wrapped inside SafeTestsets.

test/helpers.jl

skygering · 2024-05-14T16:36:03Z

test/helpers.jl

+        end
+    end
+    quote
+        for mod in (GeoInterface, ArchGDAL, GeometryBasics, LibGEOS)


Might be nice to pull these out to the top of the file as a const list to make it clear which ones are tested without having to skim the whole file. Also, that way if other geometries make it into the scene we can easily add to the test.

Also, do we want the GeoInterface conversion to be tuples? Or do we not care about the point type here?

I'll move it up. Hopefully the point type doesn't matter much...

skygering

I am glad we have this. In a few places I was trying to make examples of different types of geometries to test this. It is nice that this is automated now.

rafaqz · 2024-05-15T18:23:31Z

Some ecosystem problems to fix here:

ArchGDAL cant convert to Line https://github.com/JuliaGeo/GeometryOps.jl/actions/runs/9100439649/job/25015291938?pr=135#step:7:301
And GeometryBasics cant either https://github.com/JuliaGeo/GeometryOps.jl/actions/runs/9100439649/job/25015291938?pr=135#step:7:352
ArchGDAL cant make empty points with the createpoint constructor https://github.com/JuliaGeo/GeometryOps.jl/actions/runs/9100439649/job/25015291938?pr=135#step:7:558
And LibGEOS convert to Point is broken https://github.com/JuliaGeo/GeometryOps.jl/actions/runs/9100439649/job/25015291938?pr=135#step:7:476
LibGEOS cant convert to GeometryCollection https://github.com/JuliaGeo/GeometryOps.jl/actions/runs/9100439649/job/25015291938?pr=135#step:7:672
GeometryBasics needs to just conert to LineString for LinearRing https://github.com/JuliaGeo/GeometryOps.jl/actions/runs/9100439649/job/25015291938?pr=135#step:7:722
GeometryBasics has no GI.ncoord on MultiPolygon https://github.com/JuliaGeo/GeometryOps.jl/actions/runs/9100439649/job/25015291938?pr=135#step:7:875

@evetion we finally have a big test suite to throw at everything and a way to do it easily :D

asinghvi17 · 2024-05-15T22:15:10Z

I can deal with the GeometryBasics stuff today, will push a change to the CI to use that branch once I have.

asinghvi17 · 2024-05-16T00:51:54Z

Looks like the distance test failures are because GB and AG polygons hold LineStringTraits, whereas LG and GI polygons hold LinearRingTraits.

rafaqz · 2024-05-16T14:24:22Z

@asinghvi17 do you want to update the barycentric tests with @test_all_implementations? none of the geometries have variable names yet to pass to the macro, and you will probably do that naming in a more sensible way than me

asinghvi17 · 2024-05-16T15:06:38Z

Sure, will push it up in a bit.

felixcremer · 2024-05-16T21:08:01Z

we finally have a big test suite to throw at everything and a way to do it easily :D

Does this mean we should add GeometryOps as reverse dependency test to these other packages?

rafaqz · 2024-05-17T08:03:01Z

Maybe at some stage. We could add an env var to only test one package

asinghvi17 · 2024-06-09T19:13:25Z

test/methods/geom_relations.jl

+        @test_all_implementations (g1, g2) begin
+            if swap_points
+                g1, g2 = g2, g1
+                sg1, sg2 = sg2, sg1
+            end
+            go_val = GO_f(g1, g2)
+            lg_val = LG_f(g1, g2)
+            @test go_val == lg_val
+            go_val != lg_val && println("\n↑ TEST INFO: $sg1 $f_name $sg2 - $sdesc \n\n")
        end


This is killing test logs - it is literally impossible to read them because of the number of testsets. There must be a better way to handle this case - maybe using an external loop over the modules instead of an internal one over the geometries?

Inverting the loop wont change the amount of printing as its in the middle already/either way.

I think we instead need to remove that line? Maybe we could make it a testset instead?

I don't think the printing is an issue because as we fix the test cases, those will naturally fix themselves. I guess the problem is that each case has an individual testset - we should change that so that each module just runs all the test cases instead.

Maybe in this case we could use an explicit loop instead of the macro?

This will be upstreamed later, but it's easier to keep track here

* Move types to a new `types.jl` file so they are more accessible and general * Add GEOSCorrection and LibGEOSExt * Minor fixes * Write out stubs for not-yet-implemented functions * Implement `GEOS` algorithm * Update ext/GeometryOpsLibGEOSExt/segmentize.jl * switch from typed keys to no keys * fix erroneous typing in buffer * add test skeleton + move enforce to GO proper * Switch to SafeTestsets for tests This is meant to assure that conflicts in e.g. polygon definitions are avoided, and that tests are immediately runnable as files. * Test segmentize properly * Apply suggestions from code review * Fix tests * Allow LibGEOS to perform natural conversion on DE-9IM and poly set ops * Describe exactly why the filter statement exists in the extension * Remove GEOS correction (not working yet) * Add TopologyPreserve simplification method * Add a basic comment to not_implemented_yet.jl * Add a "default" implementation for buffer that returns GO geoms * Switch back to explicit conversion to LG in all simple overrides * Fix incorrect references * Add Downloads and NaturalEarth to the test env * Fix primitive tests * Add a hack for LibGEOS geometrycollection conversion * Fix accidental swapping in x and y in test * Add overlaps testing for GEOS * Bump version to v0.1.7 (#150) * Set compat for LibGEOS to be after the GeometryCollection patch * Force `simplify` to always be called with GEOS() for GEOS functions * Go back to regular testsets from SafeTestsets * Revert the geom relations tests for #135 * Get simplify working as well * Activate the LibGEOS listed tests * Fix the segmentize test * Fix buffer

test/helpers.jl

asinghvi17 · 2024-07-13T13:24:03Z

I'm going to make a PR that changes the implementation to be a custom @test instead of a custom @testset

rafaqz · 2024-08-02T07:07:43Z

@asinghvi17 did you end up working on this? otherwise I can finish it

asinghvi17 · 2024-08-02T07:19:29Z

I am currently on the road so haven't got anything done...go for it! Are you doing this in the a and b list generators?

rafaqz · 2024-08-02T07:31:24Z

not sure you mean by a and b list generators...

rafaqz · 2024-08-02T07:44:44Z

Do you know if there is a macro utility other packages use to walk the AST and replace $var with var, and make a vars list at the same time?

It seems like a generic thing someone has written

~~Oops the parser actualy does that! Not sure how we can get interpolated variables to do the conversion step.~~

asinghvi17 · 2024-08-02T07:50:47Z

Oops, somehow I confused this for the other issue. I did a bit of work on this, will push it tomorrow!

asinghvi17 · 2024-08-02T07:53:00Z

BenchmarkTools probably has something for the var stuff

rafaqz · 2024-08-02T08:38:16Z

Ok BenchmarkTools quasiquote! is what we want:

https://github.com/JuliaCI/BenchmarkTools.jl/blob/cb09e406b84c6a75f257e2b8dbec7fa4f8b8aca5/src/execution.jl#L361-L380

Its small enough to just copy/paste

rafaqz · 2024-08-04T16:38:57Z

This should be good to go.

I think I've made the printing a bunch shorter. But its still mostly the @testset_implements macro as doing this in a @test_implements macro doesn't really work - the geoms are used in the shared setup code.

I've also actually used SafeTestsets.jl (we depended on it but didnt use it) and updated everything for that.

One thought I have is we should show wherever we are manually skipping packages - like maybe print a warning about it?

asinghvi17 · 2024-08-05T12:07:21Z

I guess I'm not totally clear on where the setup code needs to be package specific?

I've also actually used SafeTestsets.jl (we depended on it but didnt use it) and updated everything for that.

👍

One thought I have is we should show wherever we are manually skipping packages - like maybe print a warning about it?

What defines "skipping packages"? E.g. GeometryBasics doesn't support geometrycollections out of the box...

Similarly I don't think there's infrastructure to convert to Shapefile geoms, or GeoJSON geoms. Are we implicitly skipping those?

One thought I had was to define e.g. GI.convert(ClockwiseExteriorWindingOrder, geom) to enable us to test winding order, closed vs unclosed rings, etc. That can probably go in another PR though.

asinghvi17 · 2024-08-05T13:20:03Z

test/helpers.jl

+
+export @test_implementations, @testset_implementations
+
+const TEST_MODULES = [GeoInterface, ArchGDAL, GeometryBasics, LibGEOS]


Should this be configurable by Preferences.jl or an env variable?

asinghvi17 · 2024-08-05T14:36:45Z

Overall, aside from those comments, I think we should merge!

asinghvi17

I'm sold!

rafaqz · 2024-08-05T17:13:22Z

By skipping I mean manually passing a subset of the defaults - so we would notify about skips by just getting the setdiff of the passed modules and the standard ones and putting it in @warn. But that can be later.

rafaqz requested review from skygering and asinghvi17 May 6, 2024 13:20

asinghvi17 approved these changes May 6, 2024

View reviewed changes

test/helpers.jl Outdated Show resolved Hide resolved

test/helpers.jl Outdated Show resolved Hide resolved

asinghvi17 reviewed May 6, 2024

View reviewed changes

test/helpers.jl Outdated Show resolved Hide resolved

asinghvi17 reviewed May 7, 2024

View reviewed changes

test/helpers.jl Outdated Show resolved Hide resolved

skygering reviewed May 14, 2024

View reviewed changes

rafaqz force-pushed the test_all_implementations branch from 5c749cb to 887dfcf Compare May 15, 2024 17:54

rafaqz force-pushed the test_all_implementations branch from cfe3378 to a0a76f8 Compare May 16, 2024 14:02

asinghvi17 reviewed Jun 9, 2024

View reviewed changes

rafaqz and others added 13 commits June 10, 2024 18:20

add macro to test all implementations

e12b1e5

few more

7a1159c

Apply suggestions from code review

eb76c60

Double interpolation to escape quotenode

bbdedfd

mostly passing

edb37df

Eval in missing GeometryBasics methods

81577e5

This will be upstreamed later, but it's easier to keep track here

Add a few more GB methods + comments

0c5c889

Add LibGEOS geometry collection constructor

270d660

Add Downloads to Project

f46f8dd

fix reproject test

cb5320f

test most of the rest

49a9d32

fix macro names

25af6ce

more tests

98cf808

rafaqz force-pushed the test_all_implementations branch from 8121476 to a3af803 Compare June 10, 2024 21:11

asinghvi17 added a commit that referenced this pull request Jun 11, 2024

Revert the geom relations tests for #135

458b62b

Merge branch 'main' into test_all_implementations

3d7e4d2

asinghvi17 reviewed Jun 11, 2024

View reviewed changes

test/helpers.jl Outdated Show resolved Hide resolved

asinghvi17 added 3 commits June 25, 2024 08:41

Merge branch 'main' into test_all_implementations

af838d6

Remove LibGEOS eval

0f94b72

Merge branch 'main' into test_all_implementations

5a8bdc5

rafaqz added 3 commits August 3, 2024 15:33

Merge branch 'main' into test_all_implementations

8fec1b1

new macros

5312099

refactor tests

738e9cd

rafaqz marked this pull request as ready for review August 4, 2024 00:41

bugfix title

483dd65

asinghvi17 reviewed Aug 5, 2024

View reviewed changes

Merge branch 'main' into test_all_implementations

c1f6433

asinghvi17 approved these changes Aug 5, 2024

View reviewed changes

rafaqz merged commit 266454d into main Aug 5, 2024
5 of 6 checks passed

rafaqz deleted the test_all_implementations branch August 5, 2024 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add macro to test all implementations #135

add macro to test all implementations #135

rafaqz commented May 6, 2024 •

edited

Loading

asinghvi17 left a comment

skygering May 14, 2024 •

edited

Loading

rafaqz May 14, 2024

skygering left a comment

rafaqz commented May 15, 2024 •

edited by asinghvi17

Loading

asinghvi17 commented May 15, 2024

asinghvi17 commented May 16, 2024

rafaqz commented May 16, 2024

asinghvi17 commented May 16, 2024

felixcremer commented May 16, 2024

rafaqz commented May 17, 2024

asinghvi17 Jun 9, 2024

rafaqz Jun 10, 2024 •

edited

Loading

asinghvi17 Jun 10, 2024

asinghvi17 commented Jul 13, 2024

rafaqz commented Aug 2, 2024

asinghvi17 commented Aug 2, 2024

rafaqz commented Aug 2, 2024

rafaqz commented Aug 2, 2024 •

edited

Loading

asinghvi17 commented Aug 2, 2024

asinghvi17 commented Aug 2, 2024 •

edited

Loading

rafaqz commented Aug 2, 2024

rafaqz commented Aug 4, 2024

asinghvi17 commented Aug 5, 2024 •

edited

Loading

asinghvi17 Aug 5, 2024

asinghvi17 commented Aug 5, 2024

asinghvi17 left a comment

rafaqz commented Aug 5, 2024 •

edited

Loading


		export @test_implementations, @testset_implementations

		const TEST_MODULES = [GeoInterface, ArchGDAL, GeometryBasics, LibGEOS]

add macro to test all implementations #135

add macro to test all implementations #135

Conversation

rafaqz commented May 6, 2024 • edited Loading

asinghvi17 left a comment

Choose a reason for hiding this comment

skygering May 14, 2024 • edited Loading

Choose a reason for hiding this comment

rafaqz May 14, 2024

Choose a reason for hiding this comment

skygering left a comment

Choose a reason for hiding this comment

rafaqz commented May 15, 2024 • edited by asinghvi17 Loading

asinghvi17 commented May 15, 2024

asinghvi17 commented May 16, 2024

rafaqz commented May 16, 2024

asinghvi17 commented May 16, 2024

felixcremer commented May 16, 2024

rafaqz commented May 17, 2024

asinghvi17 Jun 9, 2024

Choose a reason for hiding this comment

rafaqz Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

asinghvi17 Jun 10, 2024

Choose a reason for hiding this comment

asinghvi17 commented Jul 13, 2024

rafaqz commented Aug 2, 2024

asinghvi17 commented Aug 2, 2024

rafaqz commented Aug 2, 2024

rafaqz commented Aug 2, 2024 • edited Loading

asinghvi17 commented Aug 2, 2024

asinghvi17 commented Aug 2, 2024 • edited Loading

rafaqz commented Aug 2, 2024

rafaqz commented Aug 4, 2024

asinghvi17 commented Aug 5, 2024 • edited Loading

asinghvi17 Aug 5, 2024

Choose a reason for hiding this comment

asinghvi17 commented Aug 5, 2024

asinghvi17 left a comment

Choose a reason for hiding this comment

rafaqz commented Aug 5, 2024 • edited Loading

rafaqz commented May 6, 2024 •

edited

Loading

skygering May 14, 2024 •

edited

Loading

rafaqz commented May 15, 2024 •

edited by asinghvi17

Loading

rafaqz Jun 10, 2024 •

edited

Loading

rafaqz commented Aug 2, 2024 •

edited

Loading

asinghvi17 commented Aug 2, 2024 •

edited

Loading

asinghvi17 commented Aug 5, 2024 •

edited

Loading

rafaqz commented Aug 5, 2024 •

edited

Loading