search

openedx / edx-platform

The Open edX LMS & Studio, powering education sites around the world!
https://openedx.org
GNU Affero General Public License v3.0
7.33k stars 3.84k forks source link

Enforce 100% coverage on the test modules themselves #32659

Open kdmccormick opened 1 year ago

kdmccormick commented 1 year ago

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.

nedbat commented 1 year ago

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

kdmccormick commented 1 year ago

That would definitely help!