Skip to content
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

Issue 152 unit tests #164

Merged
merged 19 commits into from
Aug 9, 2020

Conversation

jmoujaes
Copy link
Contributor

@jmoujaes jmoujaes commented Sep 8, 2018

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

Suggestions are appreciated!

@karimbahgat
Copy link
Collaborator

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

@jmoujaes
Copy link
Contributor Author

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

@karimbahgat
Copy link
Collaborator

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
Copy link
Contributor Author

jmoujaes commented Feb 9, 2019

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

@jmoujaes
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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 travis-ci/travis-ci#9002).

@jmoujaes
Copy link
Contributor Author

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 karimbahgat merged commit 4cf125e into GeospatialPython:master Aug 9, 2020
@karimbahgat
Copy link
Collaborator

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
Copy link
Contributor Author

jmoujaes commented Aug 9, 2020

@karimbahgat happy to help! Thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants