-
-
Notifications
You must be signed in to change notification settings - Fork 155
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
[WIP] RFC8 - Move to pytest testing tool [Issue#428] #492
Conversation
These are not needed anymore on GET requests
Removed a lot of boiler plate code, simplifying things
Reorganized the location of test data files. This keeps things a bit cleaner
These are not needed anymore on GET requests
Removed a lot of boiler plate code, simplifying things
Reorganized the location of test data files. This keeps things a bit cleaner
Removed a lot of boiler plate code, simplifying things
These changes have to be manual, since this branch changed the location of the expected test files
This simplifies installation for develoment and testing purposes by requiring a single pip install -r requirements-dev.txt
cc @kalxas @ricardogsilva I've done a first pass review and have provide specific comments/questions as part of the review -- very impressive work here! Additional comments:
|
source = | ||
pycsw | ||
.tox/*/lib/python*/site-packages/pycsw | ||
/usr/local/lib/python*/dist-packages/pycsw |
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.
Is this hardcoded path safe enough or should it be abstracted somehow?
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.
I believe so.
- The second line refers to paths inside the virtualenvs generated by tox, so they are always the same, and they do not depend on the base system.
- The third line is the standard path where python packages are installed whenever they are pip installed in a debian-like system. I'm not so sure this is the same on other distros. However, I don't think it is relevant, as this configuration's purpose is for allowing the coverage output's to be treated as the same by
coverage.py
, specifically for the continuous integration service. Since we are using Travis, which is based on ubuntu, the paths are the expected ones.
@@ -0,0 +1,12 @@ | |||
[run] |
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.
is this file for coverage.py? Is coverage.py enabled by the tox --cov
flag?
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.
- Yes,
.coveragerc
is the configuration for runningcoverage.py
. More details here. In our case I am using it mostly to:- enable branch covering, which is useful to see whether tests cover multiple options in
if.. elif.. else
blocks - allow for collecting results inside tox virtualenvs
- enable branch covering, which is useful to see whether tests cover multiple options in
- Yes, coverage is enabled in tox too. This is configured in the
tox.ini
. I've chosen to enable coverage only on python3.4 for now. In the future we can report coverage on all python versions and then combine them all, but this will require additional work
@@ -6,15 +6,15 @@ | |||
- Python version: | |||
- pycsw version: | |||
- source/distribution | |||
- [ ] git clone |
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.
These changes in this file are already committed upstream.
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.
yeah, I really have to clean up the commit history...
- pip install -r requirements-dev.txt | ||
- pip install -r requirements-standalone.txt | ||
- python setup.py -q install | ||
- pip install tox |
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.
Shouldn't we just include tox in requirements-dev.txt
and pip install -r requirements-dev.txt
?
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.
That is not possible. The way in which tox
works is that it is responsible for creating separate virtualenvs for each python version (and other combinations of dependencies, like one virtualenv for python3.4 with sqlite, another virtualenv for python3.4 with postgresql, etc). As such, the tox process is the one which is going to be calling pip to install the dependencies inside each virtualenv after it has been created. This happens when we run tox
on the command-line. This means that tox must be installed system-wide and a priori of installing pycsw rquirements.
In short, its tox's job to install the requirements, so tox has to be already installed and there is no benefit in including it in requirements.txt
.
@@ -48,6 +48,9 @@ | |||
|
|||
# -- General configuration ----------------------------------------------------- | |||
|
|||
# locale |
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.
These changes in this file are already committed upstream.
@@ -0,0 +1,30 @@ | |||
# This file exists to force git to include this directory in the code |
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.
Should this file be called .gitkeep
to be consistent with other .gitkeep
files in the repo?
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.
I'd actually prefer if the other (what other .gitkeep
files are there elsewhere in the codebase if not in the tests directories) files would be renamed to .gitignore
and would have a textual description of their purpose, as I've done here.
It took me a while to understand what this .gitkeep
thing was. It is not a git feature and is therefore not documented in the git docs. It is merely a convention that these files are called .gitkeep
. IMHO using .gitignore
with a brief explanation of the file's purpose inside is more explicit.
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.
OK, Can you make this consistency change across the codebase?
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.
yes, I can do it :)
Maybe after this has been merged?
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, thanks!
@@ -0,0 +1,5 @@ | |||
Exception-Harvest-missing-resourcetype,service=CSW&version=2.0.2&request=Harvest |
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.
I think this file should be git mv
'd as opposed to added/deleted? This will help preserve history.
@@ -0,0 +1,30 @@ | |||
# This file exists to force git to include this directory in the code |
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.
I think this file should be git mv
'd as opposed to added/deleted? This will help preserve history.
@@ -0,0 +1,20 @@ | |||
empty,mode=oaipmh |
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.
I think this file should be git mv
'd as opposed to added/deleted? This will help preserve history.
@@ -0,0 +1,5 @@ | |||
explain,mode=sru |
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.
I think this file should be git mv
'd as opposed to added/deleted? This will help preserve history.
|
I'm having a difficult time figuring out how to properly clean up the git commit history and I'm kind of scared to mess thing up. Can you help me out with this? Thanks a lot in advance |
It seems I've found a way to clean up the git history and commits by checking out the master and then diff'ing my changes onto it (with some more tweaking on top). It's still WIP, but I-m confident it will work. I'll report when I have it working. Afterwards I'll be closing this PR and opening another with the cleaned stuff. |
I am closing this PR for now. The same functionality is available in a newer PR, with a cleaner commit history |
Overview
This pull request implements the changes proposed in RFC8 - Move to pytest testing tool
The work is not finished yet. I am submitting the PR already in order to have a more public place for early review for the RFC and also to allow testing the incoming changes to travis.
A rough draft of the completed/todo list and current status:
sqlite
databasepostgresql
databasepaverment.py
runtests
test runnerpytest-cov
pluginpytest-timeout
pluginsqlite
as backend andpython2.6
sqlite
as backend andpython2.7
sqlite
as backend andpython3.4
sqlite
as backend andpython3.5
I plan to complete the missing items on the list shortly and then call for a vote on the RFC. Currently I'm facing some troubles with the
harvesting
suite as I'm getting some errors that seem to be external to pycsw. The other suites are all passing.Related Issue / Discussion
RFC8 - Move to pytest testing tool
Issue #428
Additional Information
I'm contemplating other tasks that could be done and that are somehow related to this PR. I've decided to keep them out of scope for this:
pytest-xdist
plugin - May require further changes and I did not feel like delaying this PR even more;postgresql
as backend - There are some tests that are currently not working with postgresql. This has nothing to do with this PR, the tests are broken in the current master as well. We can tackle this more effectively once we get an automated way to run postgresql tests;mysql
database - Requires further development. Since this feature is not present in the current master I guess it is not a priority;Contributions and Licensing
(as per https://github.com/geopython/pycsw/blob/master/CONTRIBUTING.rst#contributions-and-licensing)