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

Re-enable codecov #364

Merged
merged 3 commits into from Nov 29, 2017
Merged

Re-enable codecov #364

merged 3 commits into from Nov 29, 2017

Conversation

@jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Nov 23, 2017

This also removes dependence on ./requirements-install.sh and flake8-run.sh

@jayvdb
Copy link
Contributor Author

@jayvdb jayvdb commented Nov 23, 2017

(Once CI finishes; I'll rebase to pull in the other CI patch)

@jayvdb jayvdb force-pushed the jayvdb:codecov branch from c99bcb2 to d7ad0b0 Nov 23, 2017
@codecov-io
Copy link

@codecov-io codecov-io commented Nov 23, 2017

Codecov Report

No coverage uploaded for pull request base (master@32713a5). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32713a5...e17cf26. Read the comment docs.

@jayvdb jayvdb changed the title Re-enable codecov WIP: Re-enable codecov Nov 23, 2017
@jayvdb
Copy link
Contributor Author

@jayvdb jayvdb commented Nov 23, 2017

#365 should be merged first, as that makes this one a bit easier.

@willkg
Copy link
Contributor

@willkg willkg commented Nov 24, 2017

I'll take a look at this tomorrow. Thank you!

@jayvdb jayvdb force-pushed the jayvdb:codecov branch from d7ad0b0 to d041e3a Nov 24, 2017
@willkg willkg added this to the 1.0 milestone Nov 27, 2017
optional: -r{toxinidir}/requirements-optional.txt
-r{toxinidir}/requirements-test.txt
doc: Sphinx

This comment has been minimized.

@willkg

willkg Nov 27, 2017
Contributor

It looks like when this ran the tests with six==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 from optional.

I don't know offhand if that's important, 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.txt

This comment has been minimized.

@jayvdb

jayvdb Nov 27, 2017
Author Contributor

Sorry, yes I missed that. It would be better to achieve that by using TOXENV=six19-optional .

commands =
{envbindir}/py.test {posargs}
six19: pip install six==1.9
{env:PYTEST_COMMAND:{envbindir}/py.test} {posargs}
flake8 {toxinidir}

This comment has been minimized.

@willkg

willkg Nov 27, 2017
Contributor

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.

This comment has been minimized.

@jayvdb

jayvdb Nov 27, 2017
Author Contributor

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 ;-)

This comment has been minimized.

@willkg

willkg Nov 27, 2017
Contributor

+1 to everything you say here. Don't change what you have.

@willkg
Copy link
Contributor

@willkg willkg commented Nov 27, 2017

@jayvdb Are you still working on this or are you done?

@jayvdb
Copy link
Contributor Author

@jayvdb jayvdb commented Nov 27, 2017

You've found one thing that needs fixing, and I'm happy to change the flake8 invocation also.

@jayvdb jayvdb force-pushed the jayvdb:codecov branch from d041e3a to 943d8c3 Nov 29, 2017
@jayvdb jayvdb changed the title WIP: Re-enable codecov Re-enable codecov Nov 29, 2017
jayvdb added 2 commits Nov 23, 2017
The use of `coverage combine` without multiple sets
of data has been resulting in a warning `No data to combine`
and the data not being submitted.
Coverage of PyPy was disabled however it works
without 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 override
the default command used to run the tests.

Due to pips inability to deal with multiple
requirements for the same package, six==1.9 is
forcably installed after the tox environment
has been setup.
@willkg
Copy link
Contributor

@willkg willkg commented Nov 29, 2017

@jayvdb Can you pin pytest to version 2.2.5? I think that'll make this PR work better in CI and I'll just drop PR #372.

@jayvdb
Copy link
Contributor Author

@jayvdb jayvdb commented Nov 29, 2017

2.2.5 , or 3.2.5?

@willkg
Copy link
Contributor

@willkg willkg commented Nov 29, 2017

Bleh. I'm having typing problems today. I meant 3.2.5. Thanks!

@jayvdb
Copy link
Contributor Author

@jayvdb jayvdb commented Nov 29, 2017

I have cherry-picked your commit; I hope that is OK.

@willkg
Copy link
Contributor

@willkg willkg commented Nov 29, 2017

You rock--thank you!

@willkg
willkg approved these changes Nov 29, 2017
Copy link
Contributor

@willkg willkg left a comment

This looks super! Thank you so much!

@willkg willkg merged commit 1b515f5 into html5lib:master Nov 29, 2017
3 of 4 checks passed
3 of 4 checks passed
codecov/project No report found to compare against
Details
codecov/patch Coverage not affected.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hugovk hugovk mentioned this pull request Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.