mengw15 commented on PR #5164: URL: https://github.com/apache/texera/pull/5164#issuecomment-4527835861
> > A general question about Codecov policy: > > I understand that we want to aim for getting every PR to 100% patch coverage. But sometimes covering the last few branches requires changes that go well beyond the PR's actual scope — for example, refactoring production code purely for testability (introducing DI seams or test-only constructor parameters), or adopting a new project-wide testing framework (e.g. a mocking library the project doesn't currently use). > > Each of those is a project-wide trade-off, not a PR-local decision. > > Question: is 100% Codecov patch coverage a hard merge requirement? > > Thanks. We have to admit that our current coverage across the entire repo is awkwardly low. We used to rely on manual tests and reviews to catch bugs, but as the project goes larger and code production speed goes quicker, we need to rely on tests to guard the behaviors. We will need unit tests, e2e tests, benchmarks, etc. Coverage report is only reporting for unit tests, and these are the easiest goal to fit (compared to e2e and benchmarks). > > To unit test a module, sometimes we need to design the source code in a way that is testable. Some times when external services (e.g., http request, databases, or remote file systems) are not available, we will need to mock them. Those are kind of necessary steps we need to pay to go forward. > > I understand now it is a painful time to ask contributors to start to fill up the gap on unit tests and make coverage on the patch higher, because many of our test infra are missing. As you mentioned, sometimes we are lack of mocking packages. However, we will only try to add tests cases then we can find out we are missing some infra. In my view, if a PR touches a part of code, and we did not have coverage before, it is a good chance to fill the tests with the PR. I am fine if the tests go beyond the source change of the PR: we can only do so to fill the gap of previously non-tested code. For instance, if a PR touches a few lines of a service.ts, and we discover that this service.ts was never tested before, then I would encourage you to add tests about the service.ts in the same PR. Even if a project-wide change is needed (e.g., introduce a mock library), I would still encourage you to add it, so that the introduction of the library can be justified by your PR's test usage. > > > Question: is 100% Codecov patch coverage a hard merge requirement? > > No. At any stage, 100% patch coverage should not be treated as a hard merge requirement, because some lines of code are inherently difficult or impractical to test. > > We currently treat coverage as a risk signal rather than a strict gate. For example, a patch with 0% coverage is usually risky and should generally be avoided. However, if the committer reviewing the PR believes the remaining risk is acceptable, the PR can still be merged. During review, I hope we use coverage as evidence that the author made a reasonable effort to add tests. If the PR author explains why certain code paths are hard or impossible to test, and the explanation is convincing, that also demonstrates testing effort. The goal is not to enforce a fixed percentage blindly, but to make sure we understand the risk and that the author has seriously considered how the change should be tested. > > Getting back to this PR specifically, based on the coverage report I can see that frontend files are being well tested, but backend we had 0% test. So I commented to encourage you to attempt to add test cases for backend changes. If you need help on test framework, please let me know and we can work together to add them. > > Gradually I hope we can increase our overall test coverage, and that can greatly reduce review efforts. Thanks for the detailed reply and explanation. I asked this general question because, while working on the backend tests for this PR, I felt that some of the remaining uncovered paths may require changes that go beyond the original scope of this PR. Your explanation makes sense to me. I agree that coverage should be treated as a risk signal rather than a strict gate, and that this is also a good opportunity to gradually improve tests in areas that were not covered before. I will try to add reasonable backend test cases for this PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
