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

Add coverage reporting to tests #917

Merged
merged 2 commits into from
Nov 8, 2021

Conversation

samdoran
Copy link
Contributor

@samdoran samdoran commented Nov 5, 2021

Enable and configure test coverage.
Add a tox environment for cleaning up coverage reports and docs builds.

@samdoran samdoran added the needs_triage New item that needs to be triaged label Nov 8, 2021
@samdoran samdoran marked this pull request as ready for review November 8, 2021 19:01
@samdoran samdoran requested a review from a team as a code owner November 8, 2021 19:01
- Make sure the ignores we have are relevant to the project.
- Break up ignores into topical sections
- Switch most directory ignores to relative and directory only rather than
  absolute but matching files or directories
- Remove macOS and editor specific ignores (should be managed by the user)
@Shrews Shrews added gate and removed needs_triage New item that needs to be triaged labels Nov 8, 2021
Copy link
Contributor

@ansible-zuul ansible-zuul bot left a comment

Choose a reason for hiding this comment

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

LGTM!

@ansible-zuul ansible-zuul bot merged commit 5188fc8 into ansible:devel Nov 8, 2021
@@ -11,3 +11,7 @@ addopts =
--durations 10
--durations-min 1
--strict-markers
--cov
Copy link
Member

Choose a reason for hiding this comment

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

@samdoran I also recommend adding an explicit -p pytest_cov to make sure that this plugin is pre-loaded earlier (this is sometimes important) rather than later (on the plugin discovery stage).

Copy link
Member

Choose a reason for hiding this comment

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

Another comment is that you should measure the coverage for both the project and the tests. For this, you'd need to use a few subsequent args: --cov=ansible_runner --cov=test/. It may be really helpful to find out if some of the tests never run in CI, for example.

Here's what the coveragepy author posted last year: https://nedbatchelder.com/blog/202008/you_should_include_your_tests_in_coverage.html.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Plus here's my example of the coveragepy config https://github.com/ansible/pylibssh/blob/devel/.coveragerc. Hope you'll find it useful (although, some parts are related to having C-extensions so you may not need those).

pytest.ini Show resolved Hide resolved
skip_install = True
allow_external = /bin/sh
commands =
/bin/sh -c "rm -rf {toxinidir}/test/coverage/*"
Copy link
Member

Choose a reason for hiding this comment

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

@samdoran I have a few comments here:

  1. You could've used a combination of git clean commands which is easier to maintain, here's an example: https://github.com/ansible/pylibssh/blob/924771a/tox.ini#L369-L380.
  2. Instead of calling sh/rm, you could do {envpython} -c 'import shutil; shutil.rmdir("{toxinidir}/test/coverage/")' and avoid having to use allow_externals.

@@ -0,0 +1,16 @@
[run]
Copy link
Member

Choose a reason for hiding this comment

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

I think you may need parallel = True and branch = True as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

branch = True, yes I should add that. parallel = True is being set by pytest-cov.

@@ -11,3 +11,7 @@ addopts =
--durations 10
--durations-min 1
--strict-markers
--cov
Copy link
Member

Choose a reason for hiding this comment

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

Another comment is that you should measure the coverage for both the project and the tests. For this, you'd need to use a few subsequent args: --cov=ansible_runner --cov=test/. It may be really helpful to find out if some of the tests never run in CI, for example.

Here's what the coveragepy author posted last year: https://nedbatchelder.com/blog/202008/you_should_include_your_tests_in_coverage.html.

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.

None yet

3 participants