-
-
Notifications
You must be signed in to change notification settings - Fork 591
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
Refactor --coverage in the test runner #2667
Conversation
Thanks for the pull request @Cadair! Everything looks great! |
.travis.yml
Outdated
|
||
- python: 3.6 | ||
stage: Comprehensive tests | ||
env: SETUP_CMD="test --online --coverage --parallel 12" |
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.
What if there's a bug for odd numbers? ;)
Hello @Cadair! Thanks for updating the PR.
Comment last updated on June 22, 2018 at 11:22 Hours UTC |
This enables us to have either term or html reports (html is still the default) and allows coverage to work with parallel
This changes the path in the .coverage file to be based on the source directory rather than the test directory. This makes it possible for tools like codecov or coveralls to read the coverage report relative to the git clone.
Well the VSO is all on 🔥 again, so it's messing with codecov, but all seems well. |
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.
Looks good to me! Really looking forward to seeing it upstream :)
sunpy/tests/setup_command.py
Outdated
cmd_pre += pre | ||
cmd_post += post | ||
# Copy the raw .coverage file back so it can be used for CI reports | ||
cwd = os.path.join(os.path.abspath(".")) |
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.
Wouldn't just os.path.abspath(".")
would suffice here or maybe os.getcwd()
?
sunpy/tests/setup_command.py
Outdated
|
||
# Special case html as the default report | ||
if self.cov_report and (isinstance(self.cov_report, bool) or "html" in self.cov_report): | ||
html_cov = os.path.join(os.path.abspath("."), "htmlcov") |
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 could be os.path.join(cwd, "htmlcov")
where cwd
comes from above.
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.
Looks good to me! Just a small comment in terms of documentation.
sunpy/tests/helpers.py
Outdated
|
||
|
||
def _patch_coverage(testdir, sourcedir): | ||
import coverage |
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.
It would be good to have some kind of docstring or comment that explains in what way the coverage file will be patched.
This also includes a nasty little hack for measuring coverage of the test runner
sunpy/tests/runner.py is correctly measured we force the interpreter to | ||
reload it here while coverage is watching. | ||
""" | ||
imp.reload(sunpy.tests.runner) |
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.
@nabobalis this isn't python 2 compat. I think in 2.7 this will just need to be the reload()
builtin
@@ -30,7 +30,7 @@ env: | |||
- MAIN_CMD='python setup.py' | |||
- SETUP_CMD='test --coverage' | |||
- CONDA_CHANNELS='sunpy' | |||
- CONDA_DEPENDENCIES='openjpeg Cython jinja2 scipy matplotlib mock requests beautifulsoup4 sqlalchemy scikit-image pytest-mock lxml pyyaml pandas nomkl pytest-astropy suds-jurko glymur' | |||
- CONDA_DEPENDENCIES='openjpeg Cython jinja2 scipy matplotlib mock requests beautifulsoup4 sqlalchemy scikit-image pytest-mock lxml pyyaml pandas nomkl pytest-astropy suds-jurko glymur pytest-xdist' |
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.
no need for this, pytest-xdist
is installed by ci-helpers when SETUP_CMD
has parallel
or numprocesses
in it.
This moves us away from using astropy to calculate the code coverage and to using pytest-cov. This means that
--parallel
and--coverage
from the test runner work, as well as enabling--cov-report term
to work fromsetup.py