- Notifications
You must be signed in to change notification settings - Fork294
Re-enable codecov#364
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
(Once CI finishes; I'll rebase to pull in the other CI patch) |
codecov-io commentedNov 23, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #364 +/- ##========================================= Coverage ? 90.78% ========================================= Files ? 50 Lines ? 6927 Branches ? 1322 ========================================= Hits ? 6289 Misses ? 482 Partials ? 156 Continue to review full report at Codecov.
|
#365 should be merged first, as that makes this one a bit easier. |
I'll take a look at this tomorrow. Thank you! |
| optional: -r{toxinidir}/requirements-optional.txt | ||
| -r{toxinidir}/requirements-test.txt | ||
| doc: Sphinx |
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 looks like when this ran the tests withsix==1.9, it'd also run the tests with the optional requirements. I'm pretty sure your changes don't do that--six19 is a separate environment fromoptional.
I don't know offhand if that'simportant, but it seems like if we want to maintain parity, it'd be better to add a line here like:
six19: -r{toxinidir}/requirements-optional.txtThere 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.
Sorry, yes I missed that. It would be better to achieve that by usingTOXENV=six19-optional .
| commands = | ||
| {envbindir}/py.test {posargs} | ||
| six19: pip install six==1.9 | ||
| {env:PYTEST_COMMAND:{envbindir}/py.test} {posargs} | ||
| flake8 {toxinidir} |
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.
In other projects I work on, I run flake8 as a separate environment because there isn't much reason to run it multiple times. Having said that, it seems like it only takes a few seconds on html5lib-python, so seems like it'd not a big deal here.
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.
flake8 could be moved out to a separate factor easily, and even a different testenv.
It is important to run flake8 on each version of python, as pyflakes is version dependent.
The benefit of keeping it in here is that devs dont need to remember to do it, and they typically only test on one environment. For CI, the running time is inconsequential compared to the rest of the very voluminous tests ;-)
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.
+1 to everything you say here. Don't change what you have.
@jayvdb Are you still working on this or are you done? |
You've found one thing that needs fixing, and I'm happy to change the flake8 invocation also. |
The use of `coverage combine` without multiple setsof data has been resulting in a warning `No data to combine`and the data not being submitted.
Coverage of PyPy was disabled however it workswithout trouble, albeit a little slow.To allow tox to run tests under coverage,and with arguments passed to coverage,PYTEST_COMMAND can now be used to overridethe default command used to run the tests.Due to pips inability to deal with multiplerequirements for the same package, six==1.9 isforcably installed after the tox environmenthas been setup.
2.2.5 , or 3.2.5? |
Bleh. I'm having typing problems today. I meant 3.2.5. Thanks! |
I have cherry-picked your commit; I hope that is OK. |
You rock--thank you! |
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 looks super! Thank you so much!
This also removes dependence on
./requirements-install.shandflake8-run.sh