TST: Replace test datasets with pyogrio-generated files where possible #441
Conversation
| @@ -196,61 +194,82 @@ def test_read_no_geometry_no_columns_no_fids(naturalearth_lowres, use_arrow): | |||
| ) | |||
|
|
|||
|
|
|||
| def test_read_force_2d(test_fgdb_vsi, use_arrow): | |||
| with pytest.warns( | |||
There was a problem hiding this comment.
Verifying the warning was incidental to this test, and already covered by test_core.py::test_list_layers.
|
|
||
| @pytest.mark.filterwarnings("ignore: Measured") | ||
| @pytest.mark.filterwarnings("ignore: More than one layer found in") |
There was a problem hiding this comment.
We actually should verify that the multiple layers warning is raised.
brendan-ward
requested review from
jorisvandenbossche and
theroggy
and removed request for
jorisvandenbossche
| def test_fgdb_vsi(): | ||
| return f"/vsizip/{_data_dir}/test_fgdb.gdb.zip" |
There was a problem hiding this comment.
Does it matter that we don't have a direct replacement for a zipped FGB?
There was a problem hiding this comment.
I don't think it matters. We have other tests that use the /vsizip/ interface for working with a zipped shapefile, which should be a reasonable proxy for zip files containing other formats.
There was a problem hiding this comment.
This looks great!
One remark: I am not sure we still have coverage for reading an FGDB file? (now that it is removed as fixture file) Probably not that important, but I see we have a test_write_openfilegdb that only asserts the file exists on disk, maybe we can just read the resulting file as well to test the full roundtrip.
Where we were using GeoJSON files, I migrated those directly into conftest.py so they are generated using code, which should make the licensing of those files more clear (i.e., part of Pyogrio source code so our MIT license is clear). I don't have a strong opinion about whether these should be files or in code, but putting them in code seemed reasonable; let me know if you disagree.
Those files were generated manually, so their licensing was OK anyway I think. But also no strong opinion. IIRC I added several of them, and while developing / testing, I think it is a bit easier to have them as files (I typically created them with gdal/pyogrio or manually edited them afterwards) and sometimes tested/compared directly with ogrinfo. But at the end moving the text of the file into the conftest.py is of course trivial, so it's perfectly fine going that way.
|
Added tests to verify roundtrip using OpenFileGDB driver, and test of int64 dtype handling for GDAL >= 3.9 via dataset creation option. |
Per #433, we were including test data files from other parties and may not have been in great compliance with their licenses, even though we were usually using a tiny extract of the source files.
See the updates to
tests/fixtures/README.mdfirst for updated guidance on managing test files, and steps used to create those still present as files.Where we were using GeoJSON files, I migrated those directly into
conftest.pyso they are generated using code, which should make the licensing of those files more clear (i.e., part of Pyogrio source code so our MIT license is clear). I don't have a strong opinion about whether these should be files or in code, but putting them in code seemed reasonable; let me know if you disagree.This also made it easier to create an invalid GeoJSON file for testing polygons with insufficient coordinates (replaces data file with problematic license).
Where we were using 3rd party FGDB datasets, I created new datasets to mimic how we were using those. I used QGIS to hand-digitize (super simple) LineString ZM, Polygon ZM, Curve, CurvePolygon, and MultiSurface datasets, since we were using one of the FGDB datasets to verify that they were correctly downgraded to supported geometry types.