| Message ID | fedd48bd-28e8-ccc8-ae50-42d9b2ea1c16@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] t4015: improve coverage of function context test | expand |
On Wed, Dec 18, 2019 at 07:05:54PM +0100, René Scharfe wrote: > Include an actual function line in the test files to check if context is > expanded to include the whole function, and add an ignored change before > function context to check if that one stays hidden, while the originally > ignored change within function context is shown. > > [...] > test_expect_success 'combine --ignore-blank-lines with --function-context' ' > - test_write_lines 1 "" 2 3 4 5 >a && > - test_write_lines 1 2 3 4 >b && > + test_write_lines 1 2 3 "" function 1 2 3 4 5 "" 6 7 8 9 >a && > + test_write_lines "" 1 2 3 "" function 1 2 3 4 5 6 7 8 >b && I'm a little mixed on this one. This _is_ a much better test of how the two features should be have together. But I think the reason the original was so short was that it was added when fixing a bug where we'd iterate off the beginning of list of lines, which now no longer happens. Maybe we should cover both cases in two separate tests? I'd also suggest using "a b c" for the first three lines to avoid confusion (I don't think it's important that they're the same as the lines inside the "function"). -Peff
Am 18.12.19 um 21:12 schrieb Jeff King: > On Wed, Dec 18, 2019 at 07:05:54PM +0100, René Scharfe wrote: > >> Include an actual function line in the test files to check if context is >> expanded to include the whole function, and add an ignored change before >> function context to check if that one stays hidden, while the originally >> ignored change within function context is shown. >> >> [...] >> test_expect_success 'combine --ignore-blank-lines with --function-context' ' >> - test_write_lines 1 "" 2 3 4 5 >a && >> - test_write_lines 1 2 3 4 >b && >> + test_write_lines 1 2 3 "" function 1 2 3 4 5 "" 6 7 8 9 >a && >> + test_write_lines "" 1 2 3 "" function 1 2 3 4 5 6 7 8 >b && > > I'm a little mixed on this one. This _is_ a much better test of how the > two features should be have together. But I think the reason the > original was so short was that it was added when fixing a bug where we'd > iterate off the beginning of list of lines, which now no longer happens. That fix, b777f3fd61 (xdiff: clamp function context indices in post-image, 2019-07-23), should no longer be necessary, but I didn't check that thoroughly. Since we still have it (and I don't intend to remove it, better keep that extra safety), it makes sense to keep the specific test. > Maybe we should cover both cases in two separate tests? That's easy enough to do. The hardest part is coming up with a name, but simply counting up should do the trick here. > I'd also suggest using "a b c" for the first three lines to avoid > confusion (I don't think it's important that they're the same as the > lines inside the "function"). Good point. That turns the last line into a function line, though, which is unnecessary and confused me a bit, but I think it's a net win. René
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index 65615e2fa9..59b2412c22 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -2051,19 +2051,24 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' ' ' test_expect_success 'combine --ignore-blank-lines with --function-context' ' - test_write_lines 1 "" 2 3 4 5 >a && - test_write_lines 1 2 3 4 >b && + test_write_lines 1 2 3 "" function 1 2 3 4 5 "" 6 7 8 9 >a && + test_write_lines "" 1 2 3 "" function 1 2 3 4 5 6 7 8 >b && test_must_fail git diff --no-index \ --ignore-blank-lines --function-context a b >actual.raw && sed -n "/@@/,\$p" <actual.raw >actual && cat <<-\EOF >expect && - @@ -1,6 +1,4 @@ + @@ -5,11 +6,9 @@ + function 1 - - 2 3 4 - -5 + 5 + - + 6 + 7 + 8 + -9 EOF test_cmp expect actual '
Include an actual function line in the test files to check if context is expanded to include the whole function, and add an ignored change before function context to check if that one stays hidden, while the originally ignored change within function context is shown. Signed-off-by: René Scharfe <l.s.r@web.de> --- Original submission: https://lore.kernel.org/git/e47b77b4-7b7a-3d59-9e24-934528c5e297@web.de/ t/t4015-diff-whitespace.sh | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) -- 2.24.1