Clean up coverage generation and reporting in tox & travis. #187

wsanchez · 2017-06-21T20:11:17Z

Summary of what's changing here:

tox.ini:
- Print a coverage report after unit tests, excluding 100% covered files.
- Add coverage reports (HTML and XML)
- Add coverage_combine that combines coverage data and fails if coverage is incomplete.
- Add codecov environment for publishing to Codecov.
travis.yml:
- Publish to Codecov after each environment is run and let Codecov do the combine itself
  - A failed run doesn't prevent publishing any results
  - Codecov reports status faster
.coveragerc is now coverage.conf:
- Add [report] section, now that reports are generated in the Tox config
.gitignore:
- Remove some ignores that shouldn't be needed, since Tox puts things in .tox
- ✓ Use fixed paths for items that only exist in a fixed path

* Use absolute paths for fixed locations. * Sort * Remove items that should be nested into .tox/ now.

Add [report] section.

* twistedchecker-diff shouldn't be in default envlist, as it only works in CI. * "trial" envs now just run trial. "coverage" envs run trial under coverage. * Clean up coverage runs so that we aren't littering the source tree with a bunch of .coverage.* files. * Add coverage_combine env which gathers coverage from other runs and reports on the total result. * Add codecov env to publish to codecov. Change Travis config: * Use "coverage" envs. * Publish to codecov after each test run, instead of one combined publish if they all succeed.

codecov · 2017-06-21T20:16:21Z

Codecov Report

Merging #187 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
- Coverage   95.89%   95.84%   -0.05%     
==========================================
  Files          12       12              
  Lines        1558     1542      -16     
  Branches       86       86              
==========================================
- Hits         1494     1478      -16     
  Misses         52       52              
  Partials       12       12

Impacted Files	Coverage Δ
src/klein/test/test_resource.py	`96.83% <0%> (-0.07%)`	⬇️
src/klein/app.py	`98.55% <0%> (-0.02%)`	⬇️
src/klein/__init__.py	`100% <0%> (ø)`	⬆️

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 dba85f8...a73859b. Read the comment docs.

wsanchez · 2017-06-21T21:18:18Z

Ready for review.

glyph

I don't really understand the intent of these changes. I've made a couple of notes, but could you just write up a brief description about the intended structure here?

glyph · 2017-06-27T04:41:28Z

tox.ini

-    trial-py{27,py,34,35,36}-tw{155,166,current,trunk}
+    coverage-py{27,py,34,35,36}-tw{155,166,current,trunk}
+
+    coverage_combine


Including this in the envlist is bad because that means it gets run in some random order in both tox with no arguments and detox with no arguments (where it's actively harmful). You can still run it with tox -e without putting it in the envlist.

Really? When I run tox with no arguments, it always runs them in a fixed order, and combine runs last. I haven't noticed this randomization…

@glyph: Yeah, it runs strictly in the intended order. Are you sure it's random for you?

OK, I didn't realize tox actually ran the environments in order. Nevertheless, detox runs them all at the same time, so this is still a bad idea ;)

Removed in 657da86

glyph · 2017-06-27T04:42:27Z

tox.ini


    docs, docs-linkcheck

 skip_missing_interpreters = True


 ##
-# Default testenv
+# Build (default environment)


I don't understand this comment change. Arguably the comment should just be deleted since it doesn't say much, but this is not a "build" now, is it? it's still a test env. Builds are done with setup.py wheel.

Changed in 77390bb

glyph · 2017-06-27T04:42:53Z

tox.ini

+    coverage: coverage
+
+
+whitelist_externals =


This is a bad idea because it makes the tests hard to run on Windows.

Removed in 7aac72d and 862acc5

glyph · 2017-06-27T04:43:17Z

tox.ini

+
+passenv =
+    PATH
+    LANG LC_ALL


Do we really need to do localized testing? What is the purpose of passing LANG?

Hold over from another thing, removing

Removed in f02c021

glyph · 2017-06-27T04:44:07Z

tox.ini

+    coverage: COVERAGE_FILE={toxworkdir}/coverage/coverage.{envname}
+    {coverage_combine,codecov}: COVERAGE_FILE={toxworkdir}/coverage/coverage
+
+    {coverage,coverage_combine}: COVERAGE_HTML={envlogdir}/coverage_report_html


should this be .html rather than _html?

No, it's a directory name

glyph · 2017-06-27T04:50:41Z

tox.ini

+    coverage: cp "{env:COVERAGE_FILE}" "{envlogdir}/coverage"
+
+    # Run coverage reports, ignore exit status
+    coverage: - coverage html --rcfile="{toxinidir}/.coveragerc" -d "{env:COVERAGE_HTML}"


Coverage reporting is really slow, and should almost always be done only after a coverage combine. definitely we shouldn't be generating both HTML and XML reports on every travis run, where they're just going to be thrown away.

Hrm. It takes pretty consistently a bit under 0.4s on my laptop to run coverage html and alittle over 0.4s to run coverage xml.

The reason it's don't done here after a combine is to make it easy to check coverage for each run separately. With Klein that doesn't very much (at all, last I checked), so that's not a big deal.

It'll be easy enough to make it separate, so I'll do that.

Fixed in 6bcccb1

glyph · 2017-06-27T04:54:26Z

tox.ini

+    coverage combine --append
+
+    # Copy aside coverage data for each test environment in case we want to look at it later
+    cp "{env:COVERAGE_FILE}" "{envlogdir}/coverage"


Doesn't this just copy the coverage file from the coverage_combine environment? i.e. nothing?

When you run coverage_combine, the source files are deleted. This sets them aside in case you need to look at the coverage for a specific run after having run coverage_combine (eg. tox with no arguments). It's not critical, though.

Removed in 7aac72d

glyph · 2017-06-27T04:55:14Z

tox.ini

+
+    # Generate XML and publish to codecov.io
+    # Ignore errors generating coverage XML, which may be due to < 100% coverage; we still want to publish
+    - coverage xml --rcfile="{toxinidir}/.coveragerc" -o "{env:COVERAGE_XML}"


Why does this line start with a dash but the next one doesn't?

This prevents tox from erroring out if this exits with an error due to insufficient coverage; we still want to proceed to invoking codecov (the next line) in that case.

moshez

I see @glyph already asked for changes, so I just added a few more notes.

moshez · 2017-06-26T23:03:40Z

.gitignore

@@ -1,11 +1,7 @@
 *.pyc
-_trial_temp


It's hard to tell which parts of this diff are spurious and which are actual change. can you keep the old order?

OK, it's not that big a file, and I don't think it helped that much, but re-ordered.

Reordered in 3e7d556

moshez · 2017-06-28T03:36:03Z

tox.ini

+    trial: trial --random=0 --logfile="{envlogdir}/trial.log" --temp-directory="{envlogdir}/trial.d" {posargs:klein}
+
+    coverage: mkdir -p "{toxworkdir}/coverage"
+    coverage: coverage run --rcfile="{toxinidir}/.coveragerc" "{envdir}/bin/trial" --random=0 --logfile="{envlogdir}/trial.log" --temp-directory="{envlogdir}/trial.d" {posargs:klein}


If you're explicitly passing in --rcfile, I see no reason to name the file ".coveragerc" and thus make it harder to find.

That's just what the file has always been called. I don't have any trouble finding it, but I'm happy to rename it.

Renamed in d44dc11

Sorry, I should have replied initially:

.coveragerc is the official / standard name of this file - see the docs at http://coverage.readthedocs.io/en/coverage-4.4.1/config.html . coverage.conf is some random thing we just made up. I would much rather not rename it.

moshez · 2017-06-28T03:37:31Z

tox.ini

+    - coverage xml --rcfile="{toxinidir}/.coveragerc" -o "{env:COVERAGE_XML}"
+
+    # Don't ignore exit status here; this is our failure status if coverage is insufficient.
+    coverage report --rcfile="{toxinidir}/.coveragerc" --omit "*/test/*"


I usually prefer to put the --fail-under here, so it's more obvious what conditions we use for failure.

OK, I buy that.

This has a side effect I somehow didn't consider, which is that I don't need to ignore exit status so much in the tox file.

Fixed in bc03700

…eports

…atters.

wsanchez · 2017-06-29T00:20:54Z

Ready for review again.

@glyph: Summary of changes posted at the top; sorry I didn't do that at the outset.

twm · 2017-06-30T06:00:14Z

tox.ini

+
+passenv =
+    PATH
+    CI CONTINUOUS_INTEGRATION


I don't see anything in this changeset which references these. Are they for Hypothesis in #181?

wsanchez · 2017-06-30T19:41:56Z

Lemme break this into parts…

wsanchez added 3 commits June 21, 2017 13:01

Clean up:

f78bca5

* Use absolute paths for fixed locations. * Sort * Remove items that should be nested into .tox/ now.

[paths] section isn't needed.

65a7344

Add [report] section.

wsanchez mentioned this pull request Jun 21, 2017

various tox cleanups #182

Closed

wsanchez added the improvement label Jun 21, 2017

glyph reviewed Jun 27, 2017

View reviewed changes

moshez reviewed Jun 28, 2017

View reviewed changes

wsanchez added 11 commits June 28, 2017 13:53

Better comment

77390bb

No more whitelist_externals

7aac72d

No LANG

f02c021

Simpler mkdir replacement

862acc5

Rename .coveragerc to coverage.conf

d44dc11

No coverage_combine in default envlist

657da86

Reorder for moshez

3e7d556

Merge branch 'master' into coverage

444b976

Require -html and/or -xml qualifier to run html and/or xml coverage r…

6bcccb1

…eports

Merge branch 'coverage' of github.com:twisted/klein into coverage

a51e628

Move fail_under from config to --fail-under in command line when it m…

bc03700

…atters.

twm reviewed Jun 30, 2017

View reviewed changes

wsanchez added the work in progress label Jun 30, 2017

wsanchez added 6 commits July 3, 2017 10:50

Merge branch 'master' into coverage

39a742f

cosmetic

a7a1659

Merge branch 'master' into coverage

1583db9

Merge branch 'master' into coverage

586d77a

Comment

7ce96c9

Pull up from ignore-paths

c583d61

wsanchez and others added 5 commits July 10, 2017 15:18

Remove extra env vars

8a2ebc2

cosmetic

3543943

Merge branch 'master' into coverage

b828beb

Merge branch 'master' into coverage

54ef3a2

Merge branch 'master' into coverage

a73859b

wsanchez closed this Dec 14, 2017

wsanchez deleted the coverage branch December 14, 2017 22:51

Clean up coverage generation and reporting in tox & travis. #187

Clean up coverage generation and reporting in tox & travis. #187

Conversation

wsanchez commented Jun 21, 2017 • edited Loading

codecov bot commented Jun 21, 2017 • edited Loading

Codecov Report

wsanchez commented Jun 21, 2017

glyph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsanchez Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moshez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsanchez commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsanchez commented Jun 30, 2017

wsanchez commented Jun 21, 2017 •

edited

Loading

codecov bot commented Jun 21, 2017 •

edited

Loading

wsanchez Jun 28, 2017 •

edited

Loading