Skip to content

Conversation

@blag
Copy link
Contributor

@blag blag commented May 25, 2018

Adds tests directories to coverage results, and reworks the coverage handling in the root Makefile to (try to) avoid rerunning tests if they've already been run.

WIP because I may have broken everything since it's a subtantial rewrite of the make logic, but other than that I'm pretty happy with it.

When I ran everything manually and gathered coverage result I got about 80% coverage throughout the entire codebase (tests included), which (in my experience) is very good for a project this size.

Questions:

  • Some covered files might be considered noise in coverage results (wsgi.py, setup.py, dist_utils.py) - do we want to ignore those in our reports? I kinda do.
  • Do we care about coverage in */tests/? Some projects like it (pynacl), some people don't (rebar3).
  • Should I print out headers for the combine/report/html coverage tasks?

@blag blag force-pushed the rewrite-makefile-coverage branch 3 times, most recently from 5e37ab6 to 94f5394 Compare May 25, 2018 22:24
Makefile Outdated
@echo "Removing all coverage results directories"
@echo
rm -rf .coverage \
.coverage-unit .coverage-unit-* \
Copy link
Member

@Kami Kami May 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be safer to quote every parameter passed to rm -rf so it doesn't accidentally become .coverage-unit- * (accidental or intentional whitespace before *) or similar in the future.

@blag blag force-pushed the rewrite-makefile-coverage branch 3 times, most recently from 4166d70 to bea3412 Compare June 1, 2018 08:32

[run]
include = st2*
omit = *tests/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to omit the coverage generation for the tests/ files themselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second bullet point in the PR summary:

  • Do we care about coverage in */tests/? Some projects like it (pynacl), some people don't (rebar3).

I used to not include tests in coverage results, but the pynacl project changed my mind. I have found dead tests in projects thanks to coverage results. So I'm in favor of including *tests/ in coverage results.

@Kami
Copy link
Member

Kami commented Jun 4, 2018

I tested the changes locally, everything looks fine 👍

I will go ahead and merge this into master so it will be easier to keep things in sync, especially the PR / branch with OS X changes.

If decided and needed, we can omit *tests/ from coverage results in a subsequent PR.

@Kami Kami added this to the 2.8.0 milestone Jun 4, 2018
@Kami
Copy link
Member

Kami commented Jun 4, 2018

@m4dcoder It looks like that in this PR / branch, the test_chain_pause_resume_with_error test consistently fails (I already re-ran it 4 times and it keeps failing).

It's likely that the coverage related instrumentation some how compounds the original race / similar issue in that test. So hopefully consistent failure in this build will help you track / narrow it down?

@arm4b
Copy link
Member

arm4b commented Jun 4, 2018

Finding dead tests is a good reason, but including coverage for *tests/ dir themselves will affect the entire % coverage, increasing it.

When I ran everything manually and gathered coverage result I got about 80% coverage throughout the entire codebase (tests included)

The percentage is supposed to show what's the test coverage of the production code, not the tests themselves.
And so every time we add more test cases (which are usually include a lot LOC), - it unproportionally affect coverage of the real app code in unfair way.
With tests dir included I believe it's cheating the % project coverage for which we show a badge in main README.

Since it's about OSS community I believe what people would expect to see is the actual production code coverage as an important metric of the project quality, not the random test files.

I propose to be honest with ourselves and our users.
If we need to find the dead tests, - let's use the appropriate tool for that and not misuse the test coverage metric.

@blag blag force-pushed the rewrite-makefile-coverage branch 7 times, most recently from 8402863 to 182c5a1 Compare June 12, 2018 17:29
@blag
Copy link
Contributor Author

blag commented Jun 12, 2018

I pushed the last commit to skip tests that are failing when coverage is turned on, but I don't want to merge it with that in (it skips the entire ActionChainRunnerPauseResumeTest test case). Fixed.

@m4dcoder When you get a chance, please give me some guidance on the failing tests (all of the tests in contrib/runners/action_chain_runner/tests/unit/test_actionchain_pause_resume.py:ActionChainRunnerPauseResumeTest). Done.

@blag blag force-pushed the rewrite-makefile-coverage branch 2 times, most recently from 3f86f82 to 576ae76 Compare June 15, 2018 01:01
@blag blag changed the title WIP: Rewrite coverage handling in the root Makefile Rewrite coverage handling in the root Makefile Jun 15, 2018
@blag
Copy link
Contributor Author

blag commented Jun 15, 2018

I removed the commit that skipped tests and instead fixed the tests that were having issues being run with coverage turned on.

@armab - the new INCLUDE_TESTS_IN_COVERAGE environment variable controls whether tests are considered in the coverage data, and it defaults to off/false by default, including the Travis tests and subsequent coverage reporting. I think this is a good compromise: we can now easily manually include tests in coverage results every now and then to find dead tests, but otherwise it's turned off.

@blag blag merged commit 1f320d1 into master Jun 15, 2018
@blag blag deleted the rewrite-makefile-coverage branch June 15, 2018 16:56
@arm4b
Copy link
Member

arm4b commented Jun 15, 2018

@blag Sounds good, thank you!

include = st2*
omit = *tests/*
branch = True
concurrency = eventlet
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a heads up - that's the offending line which broke codecov.io reporting which I fixed in #4270.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants