geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Murmann <amurm...@apache.org>
Subject Test coverage and PRs
Date Mon, 15 Apr 2019 22:22:42 GMT
Hi everyone,

TL;DR:
1. Let's call each other out on our PRs when test coverage is missing or
insufficient
2. Let's require test coverage for code that gets refactored as well

We already have a really great wiki page
<https://cwiki.apache.org/confluence/display/GEODE/Criteria+for+Code+Submissions>
on how we evaluate code contributions. Among other things it calls out test
coverage for new features as well as for bug fixes. I don't think we've
always been diligent in ensuring healthy test coverage when we've been
reviewing PRs. This seems like something we should correct as a community
to ensure Geode can provide the high quality our users expect and at the
same time give us fast feedback loops in our daily work. We need to get
better code coverage, so rejecting PRs till they have appropriate test
coverage, seems like an obvious thing we need to do.

It's also interesting to me that the wiki page calls out coverage for
features and bugs. What about refactoring code that is currently missing
coverage? To refactor with confidence we need test coverage. Therefore test
coverage should be a precondition for refactoring. I'd like to amend our
wiki to also require test coverage for refactored code. Ideally that
coverage would already be there, but we all know that's unfortunately
rarely the case. This is going to hurt in the short run, but gets us closer
where we want to be.

As always, if it's unreasonable to provide certain types of test coverage,
then let's have an explicit conversation on the PR.

Note: When I say "appropriate test coverage", I am referring to coverage of
the functionality in breadth, but also in depth via a healthy testing
pyramid <https://martinfowler.com/bliki/TestPyramid.html> that includes
unit tests, integration tests etc.

What do you all think?

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message