-
Notifications
You must be signed in to change notification settings - Fork 229
Add tests for tempfile_from_geojson() #1354
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
Conversation
…odataframe() to test_helpers.py
This is failing tests on the Python 3.7 CI jobs with |
def test_tempfile_from_geojson(): | ||
""" | ||
Test tempfile_from_geojson works when passed a geopandas GeoDataFrame. | ||
""" | ||
linestring = shapely.geometry.LineString([(20, 15), (30, 15)]) | ||
polygon = shapely.geometry.Polygon([(20, 10), (23, 10), (23, 14), (20, 14)]) | ||
gdf = gpd.GeoDataFrame( | ||
index=["polygon", "linestring"], | ||
geometry=[polygon, linestring], | ||
) | ||
with tempfile_from_geojson(gdf) as geojson_string: | ||
assert isinstance(geojson_string, str) | ||
with open(geojson_string) as tmpfile: | ||
assert os.path.basename(tmpfile.name).startswith("pygmt-") | ||
assert os.path.basename(tmpfile.name).endswith(".gmt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is failing tests on the Python 3.7 CI jobs with
ModuleNotFoundError: No module named 'geopandas'
. Shouldgeopandas
be added/not made optional to requirements.txt and environment.yml?
No, geopandas
should be an optional-only dependency. Could you provide some context as to why these two tests for the tempfile_from_geojson() function are to be added? To be honest, this tempfile_from_geojson function (added in #1000) is meant to be a temporary solution until GMT can read GeoJSON directly (as per GenericMappingTools/gmt#4599) though that might take some time.
If you must though, I'd suggest using pytest.importorskip to load geopandas and shapely, similar to at
pygmt/pygmt/tests/test_geopandas.py
Lines 8 to 9 in b2808cf
gpd = pytest.importorskip("geopandas") | |
shapely = pytest.importorskip("shapely") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I added it was that the code coverage was showing tempfile_from_geojson()
wasn't tested. I just assumed we hadn't gotten around to writing tests for it yet so I wrote them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tempfile_from_geojson function does have full test coverage (see https://codecov.io/gh/GenericMappingTools/pygmt/src/master/pygmt/helpers/tempfile.py). It is tested indirectly at test_geopandas.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll close the PR.
This pull requests adds two tests to
test_helpers.py
that testtempfile_from_geojson()
Fixes #
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Slash Commands
You can write slash commands (
/command
) in the first line of a comment to performspecific operations. Supported slash commands are:
/format
: automatically format and lint the code/test-gmt-dev
: run full tests on the latest GMT development version