On Mon, Feb 15 2021, Johannes Sixt wrote: > Am 15.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason: >> Fix a regression in the test framework for userdiff added in >> bfa7d01413 (t4018: an infrastructure to test hunk headers, >> 2014-03-21). >> The testing infrastructure added in that change went overboard with >> simplifying the tests, to the point where we lost test coverage. >> Before that we'd been able to test the full context line, or ever >> since the feature was originally added in f258475a6e (Per-path >> attribute based hunk header selection., 2007-07-06). >> After bfa7d01413 all we cared about was whether "RIGHT" appeared on >> the line. We thus lost the information about whether or not "RIGHT" >> was extracted from the line for the hunk header, or the line appeared >> in full (or other subset of the line). >> Let's bring back coverage for that by adding corresponding *.ctx >> files, this has the added advantage that we're doing a "test_cmp", so >> when we have failures it's just a non-zero exit code from "grep", >> we'll actually have something meaningful in the "-v" output. >> As we'll see in a follow-up commit this is an intermediate step >> towards even better test coverage. I'm structuring these tests in such >> a way as to benefit from the diff.colorMove detection. >> The "sed -n -e" here was originally a single 's/^.*@@\( \|$\)//p' >> pattern, but the '\( \|$\)' part had portability issues on OSX and >> AIX. >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> >> --- >> t/t4018-diff-funcname.sh | 7 +++--- >> t/t4018/README | 22 +++++++++---------- >> t/t4018/README.ctx | 1 + >> t/t4018/bash-arithmetic-function.ctx | 1 + >> t/t4018/bash-bashism-style-compact.ctx | 1 + >> [...and so on...] > > This is what I meant by "without burdening test writers with lots of > subtleties". > > I'm not a friend of this change :-( > > I think you are going overboard with required test precision. To have > useful tests for userdiff patterns that demonstrate its features, > authors should write *many* tests. The right balance should be on the > coverage of userdiff pattern features, not on the subtle details of > each and everyone of it. Requiring that many additional context files > makes it *really hard* to comply. I agree that this change sucks when viewed in isolation. I think it has value as part of the larger series, since (as noted in the commit message) the point here is to allow the reviewer to see the the test content & semantics aren't different. I suppose I could do some version of squashing this and 12/27, but then I'd be introducing the full testing of the context line at the same time. By doing it this way I split the change in test semantics from the test structure change itself in 12/27.
