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

Enforce 100% coverage on the test modules themselves #32659

Open
kdmccormick opened this issue Jul 5, 2023 · 4 comments
Open

Enforce 100% coverage on the test modules themselves #32659

kdmccormick opened this issue Jul 5, 2023 · 4 comments
Labels
code health Proactive technical investment via refactorings, removals, etc.

Comments

@kdmccormick
Copy link
Member

kdmccormick commented Jul 5, 2023

More specifically: check for 100% coverage on modules named */tests/* or */test_*.py as a required PR check.

This is not a proposal to bring edx-platform as a whole to 100% coverage.

Details

Enforcing a coverage threshold on application code certainly comes with its pros and cons. But we can all agree that if a test case is added to edx-platform, running the test suite should run that test case, right?

This would prevent folks from adding code that they think will be run as unit tests, but due to an honest mistake, won't be. That happened here:

This would also shield us from tooling issues which might silently stop part of the test suite from running for some amount of time, potentially letting swaths of tests regress until it's noticed. This happened to the code in xmodule/ once (back when it was common/lib/xmodule/xmodule and had its own test settings), and we were lucky that the suite was easy to restore.

An implication of this, which I'm personally OK with, is that it would enforce that test helpers get removed if the unit tests that call them are removed. I'm sure there are some individual exceptions (eg, maybe codejail-related tests can only be run locally), but those should be possible to exempt as needed.

@kdmccormick kdmccormick added the code health Proactive technical investment via refactorings, removals, etc. label Jul 5, 2023
@nedbat
Copy link
Contributor

nedbat commented Jul 6, 2023

This might be helpful: https://nedbatchelder.com/blog/202111/coverage_goals.html

@kdmccormick
Copy link
Member Author

That would definitely help!

@kdmccormick
Copy link
Member Author

This would have prevented: #35531

@nedbat
Copy link
Contributor

nedbat commented Sep 24, 2024

Preach it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Proactive technical investment via refactorings, removals, etc.
Projects
None yet
Development

No branches or pull requests

2 participants