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

Coverage cleanups #1674

Merged
merged 9 commits into from Nov 14, 2022
Merged

Coverage cleanups #1674

merged 9 commits into from Nov 14, 2022

Conversation

mhsmith
Copy link
Member

@mhsmith mhsmith commented Nov 9, 2022

Fixes #1673.

Also revists the source and source_pkgs configuration to make sure that we detect files which are never imported. I couldn't find any such files in core, dummy or winforms, but I tested adding one and it was reported correctly.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@mhsmith mhsmith marked this pull request as ready for review November 9, 2022 17:39
@mhsmith
Copy link
Member Author

mhsmith commented Nov 9, 2022

Comparing the CI coverage output between the main branch and this branch, it appears to be identical, as expected.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

A nice set of cleanups, and it definitely seems to be working; however:

  1. The ergonomics of the "simple developer case" aren't great. Ideally, either tox -e py in a project would either include a coverage report, or there would be a separate entry point like tox -e coverage to do the "combine and cover" dance. Needing 3 moderately complex commands with very specific arguments to work out whether my new test covers the features I've added isn't a great developer experience - especially when the usual expectation is "pytest && coverage report"
  2. Whatever the instructions end up being, they need to be collected in the contribution docs, rather than buried in a config file comment.

Given that tox -e py is being used in each subproject to run the tests, one approach might be to add a top-level tox entry point to run tests & report coverage - tox -e py -- core (or tox -e core?)

@mhsmith
Copy link
Member Author

mhsmith commented Nov 10, 2022

OK, I've made tox -e py run coverage combine and coverage report automatically, because we want to encourage developers to pay attention to this.

I've also removed the backend coverage merge job, since each backend job is already reporting its own coverage. We might want to restore this if we ever start running the backends on multiple Python versions, but we should still merge each backend separately so we can start enforcing 100% coverage one backend at a time.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

All looks good; however the developer docs need to be updated to reflect these changes. The dev instructions still provide instructions for manually running coverage, and don't mention tox at all. We probably want to point people at tox as the "go to" simple case for running the test suite. I'm not sure if we need to preserve the docs about TOGA_BACKEND and manually running coverage in an "expert mode" instructions; technically, that knowledge is wrapped up in the tox and CI scripts if anyone wants to go down that route, and that stops the knowledge bitrotting over time (although hopefully we're nearing the end of the big changes to repo layout).

There's also some potential overlap with #1675 and adding a dev extras requires, so we don't need to manually specify installing pre-commit, coverage, etc. If you want to collect all the doc updates in that PR instead of this one to avoid having edits in 2 places, I'm fine with that. On that basis, I'll mark this as approved, but not merge it; if you want to merge it as-is, consider these comments about developer instructions to be a pre-requisite for my approval on #1675.

@saroad2
Copy link
Member

saroad2 commented Nov 13, 2022

Pay attention that all tox.ini scripts are now using bash, which means they will not work for Windows developers (like me)

mhsmith added a commit to freakboy3742/toga that referenced this pull request Nov 13, 2022
@mhsmith
Copy link
Member Author

mhsmith commented Nov 13, 2022

Pay attention that all tox.ini scripts are now using bash, which means they will not work for Windows developers (like me)

Good point, I'll fix that.

# Conflicts:
#	android/tox.ini
#	cocoa/tox.ini
#	core/tox.ini
#	gtk/tox.ini
#	iOS/tox.ini
#	web/tox.ini
#	winforms/tox.ini
@mhsmith
Copy link
Member Author

mhsmith commented Nov 13, 2022

I'm getting tired of editing 7 files whenever I make a change to this, so I've merged all the tox.ini files into the root one, with tox environments named as follows:

$ tox -a
py-android
py-cocoa
py-core
py-gtk
py-iOS
py-web
py-winforms
py37-android
py37-cocoa
...

And I've updated the documentation accordingly.

adding a dev extras requires, so we don't need to manually specify installing pre-commit, coverage, etc.

Coverage commands are now all handled by tox, so there's no need to install coverage in the main virtual environment. As for pre-commit, its scope covers the whole repository, including non-Python stuff like YAML and TOML formatting. So rather than tying that to the installation of any particular package, I think it's better to keep the instructions to install that separately.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The top level tox config works really well for me, and the fallback case of running pytest and coverage manually isn't that more complicated. It's also working for my testing on Windows, so I think we can call this done.

There's some lingering questions about the contribution docs and a dev dependency, but I think we can continue that discussion on #1675.

@freakboy3742 freakboy3742 merged commit 3af18a5 into beeware:main Nov 14, 2022
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.

Core test coverage report doesn't work locally
3 participants