aaron.ballman added a comment. In D122954#3427422 <https://reviews.llvm.org/D122954#3427422>, @erichkeane wrote:
> In D122954#3427406 <https://reviews.llvm.org/D122954#3427406>, @tahonermann > wrote: > >>> FWIW, I dislike this idea of doing tests in separate commits from the patch >>> itself, it makes the review of the patch more difficult, and makes looking >>> through history more difficult. >> >> Here is my perspective on this. When adding a new feature, I agree that >> including tests with the code is useful (the tests usually have no >> meaningful behavior without the new code in that case). I think the >> motivation is different for bug fixes though, particularly when there is an >> existing testing gap. Had these tests been added with the initial >> `target_clones` work, then the bug fix commit would reveal the precise >> change in behavior that is the result of the bug fix. I think that is quite >> valuable both as a review tool and as part of the code history. By adding >> the test in a separate commit from the fix, it is possible to observe both >> the previous undesirable behavior as well as precisely what changed with the >> fix. This also helps to enable someone that is looking to identify a commit >> responsible for a bug fix (e.g, someone that is looking to resolve a bug >> report as works-for-me-in-release-X) to definitively identify a commit as >> exhibiting the expected fix. >> >> I've practiced this approach at other places I've worked and never found it >> to make the review process more difficult. If you can elaborate on why you >> feel it makes review more challenging, perhaps there is another way to >> address that. > > I understand it and just disagree. I've NEVER had a tough time 'seeing the > previous behavior' vs 'the current behavior', but I DO have the opposite > problem: Figuring out what the associated tests are for a patch. > > It loses my ability to make sure that your patch covers all of the cases that > are important, and it loses my ability to figure out what the patch is doing > from the test itself. Strong +1 to this. As a reviewer, I want the patch to be making one logical change so that it remains manageable to review, but it needs to be completely self-contained (functional changes, docs, tests, et al) even if that makes the patch a bit bigger. This is especially important because it makes it far easier to revert problematic commits -- if the functional change has an issue, having to separately revert several other related commits is a burden. It also makes git blame more useful when doing code archeology; you can git blame a test to get to the functional changes more quickly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122954/new/ https://reviews.llvm.org/D122954 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
