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

Python coverage cleanup in CircleCI #2517

Merged
merged 21 commits into from Dec 19, 2017

Conversation

Projects
None yet
4 participants
@brianhelba
Copy link
Member

commented Dec 13, 2017

  • Attempt to submit pytest results to CircleCI for analysis
  • Remove the PYTHON_COVERAGE CMake option
    • Python coverage will now always run. It was already always being run for Pytest tests.
  • Remove the PYTHON_BRANCH_COVERAGE CMake option
    • This option should be set directly the Coverage config file (which is the only way to set it for Pytest).
  • Remove the PYTHON_COVERAGE_CONFIG CMake option and unify Coverage configs
    • Both the legacy CMake+unittest and Pytest runners will now use the same .coveragerc file.
  • Always write coverage reports to "build/test/coverage"
    • This ensures that Pytest-generated coverage reports are written to the same place as unittest / CMake-generated reports.
  • Write intermediate Python coverage files inside "build/test/coverage"
  • Reorganize the root .gitignore file
    • This also stops ignoring the "htmlcov" directory, which should not longer be generated by Javascript coverage.

@brianhelba brianhelba changed the title Python coverage cleanup in CircleCI WIP: Python coverage cleanup in CircleCI Dec 14, 2017

@manthey

This comment has been minimized.

Copy link
Member

commented Dec 14, 2017

This LGTM, if @danlamanna is content with not having the junit xml as an artifact.

@brianhelba brianhelba force-pushed the circle-coverage branch 2 times, most recently from 6ce35a0 to 33fef25 Dec 18, 2017

.idea/
.PyCharm50/
.*.swo
.*.swp

This comment has been minimized.

Copy link
@zachmullen

zachmullen Dec 18, 2017

Member

Not really related to this PR, but we should remove all the platform specific stuff from this file.

@danlamanna

This comment has been minimized.

Copy link
Member

commented Dec 18, 2017

@zachmullen

This comment has been minimized.

Copy link
Member

commented Dec 18, 2017

Talking about

# IDE / OS generated
.DS_Store
.idea/
.PyCharm50/
.*.swo
.*.swp

@brianhelba brianhelba changed the title WIP: Python coverage cleanup in CircleCI Python coverage cleanup in CircleCI Dec 18, 2017

@brianhelba

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2017

@girder/developers PTAL. This is now ready for review (and I've updated the PR description with the commit messages).

@manthey

This comment has been minimized.

Copy link
Member

commented Dec 18, 2017

There are downstream plugins that were using the PYTHON_COVERAGE_CONFIG and PYTHON_BRANCH_COVERAGE options. The ones I know of can do without these. Do we need a migration guide?

brianhelba added some commits Dec 13, 2017

Remove the PYTHON_COVERAGE CMake option
Python coverage will now always run. It was already always being run for
Pytest tests.
Remove the PYTHON_BRANCH_COVERAGE CMake option
This option should be set directly the Coverage config file (which is the
only way to set it for Pytest).
Remove the PYTHON_COVERAGE_CONFIG CMake option and unify Coverage con…
…figs

Both the legacy CMake+unittest and Pytest runners will now use the same
.coveragerc file.
Always write coverage reports to "build/test/coverage"
This ensures that Pytest-generated coverage reports are written to the same
place as unittest / CMake-generated reports.
Reorganize the root .gitignore file
This also stops ignoring the "htmlcov" directory, which should not longer
be generated by Javascript coverage.
Store machine-readable Python coverage in "build/test/coverage/python*"
Also, generate all build directories dynamically, instead of with
".gitignore" placeholder files.

@brianhelba brianhelba force-pushed the circle-coverage branch from 33fef25 to 3e4ea05 Dec 18, 2017

Make Javascript coverage output more robust
At the moment, there is no easy way to output the HTML coverage report
to the artifacts directory. This will have to be resolved via future work.

@brianhelba brianhelba force-pushed the circle-coverage branch from 3e4ea05 to 3b35671 Dec 18, 2017

@danlamanna

This comment has been minimized.

Copy link
Member

commented Dec 18, 2017

@brianhelba I pushed a few commits fixing what we discussed.

pytest.ini Outdated
@@ -1,3 +1,4 @@
[pytest]
addopts = --verbose --showlocals --cov=girder --cov-append --cov-config=.coveragerc --cov-report=""
cache_dir = build/test/cache/pytest

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 19, 2017

Author Member

@danlamanna Is there a reason this needs to be nested, and can't be pytest_cache? Are there other test caches we're planning on supporting?

@brianhelba

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2017

@manthey Can you point to the downstream usages PYTHON_COVERAGE_CONFIG and PYTHON_BRANCH_COVERAGE?

As a short note (which we can add to the changelog): if you want a custom coverage config file or want to enable branch coverage, just define your own coverage combine test, and pass the config file as an option.

Of course, in the future, with Tox, this will likely be even easier, but at the moment, the presence of these variables was blocking the unification of the Pytest and CMake/unittest coverage configs. Also in the future (once these changes make it easier to quickly generate and view details on coverage reports), I plan to re-evaluate the default setting for Python branch coverage and see if enabling it would suit Girder.

@danlamanna

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

danlamanna and others added some commits Dec 19, 2017

Only use Pytest's "--cov-append" argument during CI workflows
During normal runs of Pytest, files may be changed between each invocation,
so existing coverage should be discarded. In CI, it is expected that Pytest
is run after CMake's own coverage-generating tests, so Pytest should
incorporate, rather than delete, the previous tests' coverage.
@brianhelba

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2017

@danlamanna PTAL @ one final commit from me (rationale in the commit message).

Otherwise, I'd consider this PR to be complete. I want to wait for @manthey 's response before a merge though.

add_test(
NAME ${name}
WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}"
COMMAND "${PYTHON_COVERAGE_EXECUTABLE}" run

This comment has been minimized.

Copy link
@manthey

manthey Dec 19, 2017

Member

Do we need the --parallel-mode flag to make distinctly named coverage file?

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 19, 2017

Author Member

This option is now hardcoded into the .coveragerc file.

@manthey

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

@brianhelba Here is one: https://github.com/girder/slicer_cli_web/blob/master/.travis.yml (just in its travis build) -- I am certain that this was cargo-cult copied from somewhere else, and adds no value, but my fear is that someone is using these features in a more serious way.

How would you pass a custom rc file to the python coverage command (or how does it pick up a custom config file)? I guess I have to read the internals of the coverage command to know.

.coveragerc Outdated
directory = build/test/artifacts/python_coverage

[xml]
output = build/test/coverage/python.xml

This comment has been minimized.

Copy link
@manthey

manthey Dec 19, 2017

Member

What's with the name drift? The standard name for this file is coverage.xml. We had changed that to server.xml, and now to python.xml. Every downstream plugin has to jump through hoops to report coverage whenever we change this location or name.

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 19, 2017

Author Member

This was after discussion with @danlamanna . coverage.xml is an ambiguous name, since we also have JS coverge that may be output in an XML format. The previous name of server.xml is also not properly descriptive, since this also includes coverage from the Python client.

At this point, I'm satisfied that this ought to be the final name. I'd chalk up the churn to normal development uncertainty and iteration.

This comment has been minimized.

Copy link
@manthey

manthey Dec 19, 2017

Member

The think the ambiguity is debatable. Nothing in the name coverage.xml implies python, and nothing in python.xml implies coverage. Less ambiguous would be something like py_coverage.xml to match the js_coverage.xml file name from javascript.

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 19, 2017

Author Member

@manthey Good point. Done.

@brianhelba

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2017

@manthey Looking at slicer_cli_web, it appears that they use PYTHON_COVERAGE_CONFIG to point to a local coverage file that's functionally identical to the one that's was / will be used by upstream Girder. They do use PYTHON_BRANCH_COVERAGE to turn on Python branch coverage (which Girder currently has off); however, I'd like to re-address Python branch coverage in a follow-up standalone branch and likely turn it on for all of Girder.

In the future, if plugins want to run coverage with custom options then they can just write a custom coverage config file and point Pytest towards it with the appropriate CLI flag; for legacy unittest tests, they can write their own coverage run ... CMake test.

@brianhelba

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2017

@girder/developers @danlamanna I've just added a CHANGELOG entry and resolved a merge conflict. This is ready for final approval and merge.

@danlamanna

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

Can you make the changelog entry link to this PR?

@brianhelba

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2017

@danlamanna Done.

@brianhelba brianhelba removed the status: WIP label Dec 19, 2017

@brianhelba brianhelba merged commit c602de9 into master Dec 19, 2017

10 checks passed

ci/circleci: py2_coverage Your tests passed on CircleCI!
Details
ci/circleci: py2_integrationTests Your tests passed on CircleCI!
Details
ci/circleci: py2_serverInstall_serverTest Your tests passed on CircleCI!
Details
ci/circleci: py3_coverage Your tests passed on CircleCI!
Details
ci/circleci: py3_integrationTests Your tests passed on CircleCI!
Details
ci/circleci: py3_serverInstall Your tests passed on CircleCI!
Details
ci/circleci: py3_serverTest Your tests passed on CircleCI!
Details
ci/circleci: py3_webBuild_webTest Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 1fc0b23...a9a495b
Details
codecov/project 91.33% remains the same compared to 1fc0b23
Details

@brianhelba brianhelba deleted the circle-coverage branch Dec 19, 2017

@danlamanna

This comment has been minimized.

Copy link
Member

commented Jan 9, 2018

@brianhelba Did you ever file upstream issue(s) about creating the coverage directories? If so could we add code comments and/or issues?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.