search

GeospatialPython / pyshp

This library reads and writes ESRI Shapefiles in pure Python.
MIT License
1.1k stars 259 forks source link

Issue 152 unit tests #164

Closed jmoujaes closed 4 years ago

jmoujaes commented 6 years ago

This is a work in progress to add unit tests for the project.

Suggestions are appreciated!

karimbahgat commented 5 years ago

Thanks again for working on this. Just shout out when you feel like it's ready for an initial draft/merge.

jmoujaes commented 5 years ago

@karimbahgat I wouldn't mind doing it now. There are still lots more tests to write but this can be a starting point.

karimbahgat commented 5 years ago

Sorry for the delay. This looks good, agree that we just merge it and add expand on it later. Not sure what the conflict is, should be all separate I think. Would it be easy to fix/reimplement?

jmoujaes commented 5 years ago

@karimbahgat no worries at all. It looks like it should be easy. I'll resolve and update you.

jmoujaes commented 5 years ago

Looks like pytest is not supported on python 3.3. I plan on thinking how to work through this during the week. Suggestions welcome :)

karimbahgat commented 5 years ago

I guess Python 3.3 is considered one of the weird versions that people don't want to support anymore? According to the changelog this happened in version 3.3.0. A quick fix for now might be to simply make sure that we are using a pytest version prior to that chagnge, eg v3.2.5. Not sure how to tell this to TravisCI, maybe in the yml file adding "install pytest==3.2.5"?

jmoujaes commented 5 years ago

I see, thanks for the reference. Sticking to pytest version 3.2.5 sounds good to me.

And if, in the future, we need a feature that is offered by a newer version of pytest, we can specify that the python 3.3 build runs with the old pytest while the other builds run with the newer version of pytest.

Example:

pytest==3.2.5; python_version == '3.3'
pytest==9.9.9; python_version == '2.7' or python_version > '3.3'

where pytest==9.9.9 is the new version of pytest we'd want to use.

However, I'd prefer to stick with the same version across the board.

karimbahgat commented 5 years ago

Odd. From the log, it seems it correctly installed the older versions ("Successfully installed py-1.7.0 pytest-3.2.5"), but still raises some error of "requirement already satisifed", so maybe it didn't install the older version after all. Could it be that adding --upgrade would help?

Alternatively, if you wanted to just get it through, we could drop development support for Python 3.3 which seems to be going away anyway and would only be for testing purposes (see also https://github.com/travis-ci/travis-ci/issues/9002).

jmoujaes commented 5 years ago

Thanks Karim! Trying the --upgrade option indirectly led me to realize that the build was failing when setuptools was version 40.

However, I had to remove a line from the travis config that was intentionally updating setuptools. Perhaps it was being updated for a reason?

W/r to supporting 3.3 - I don't have a strong opinion either way. Supporting one less version is always nice in terms of reducing maintenance overhead. But I don't think it's been problematic for PyShp so far. Anyway, you know better, I'll leave that up to you.

karimbahgat commented 4 years ago

Finally merging this, and just decided to drop Python 3.3 as suggested by you and others. Really want to get some solid testing going, to make sure all sorts of shapefiles and edgecases gets checked, and your initital tests provides a really good starting point. Hopefully now it will be much easier to just add tests via PRs and when bugs appear. Thanks again for this solid work @jmoujaes !

jmoujaes commented 4 years ago

@karimbahgat happy to help! Thanks for reviewing!