| Message ID | 20210215154427.32693-23-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | userdiff: refactor + test + doc + misc improvements | expand |
Am 15.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason: > The existing tests in "t/t4018/" are unrealistic in that they're all > setting up small few-line isolated test cases with one thing we could > match as a hunk header, right above the one change in the file. > > Expand those tests by accumulating changes within the same file type > in the "test_diff_funcname" function. So e.g. for "bash" we'll end up > a "bash.acc" file with 15 s/ChangeMe/IWasChanged/ changes. > > This stress tests whether the hunk header selection will "jump across" > to an earlier change because the match for that is greedier. > > As it turns out we had one false positive in "t/t4018/cpp.sh" and > "t4018/matlab.sh" because of how the tests were structured, we must > always give the "ChangeMe" line at least one line of separation from > the header, since it was at the end of those tests we'd select the > "wrong" header. Let's adjust the spacing to compensate. I can't wrap my head around this. The ChangeMe lines are one line away from the RIGHT lines. Why would we need a line below ChangeMe? Or is this just a fall-out from this change itself? Then I'd appreciate to describe it as "This change now triggers false positives in... because... Let's adjust the spacing compensate.". > > So in the end we found nothing of interest here, regardless, I think > it is useful to continue to test in this mode. It's likely to aid in > finding bugs in combinations of our positive and negative matching as > we add more built-in patterns. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > t/t4018-diff-funcname.sh | 19 +++++++++++++++++++ > t/t4018/cpp.sh | 1 + > t/t4018/matlab.sh | 3 +++ > 3 files changed, 23 insertions(+) > > diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh > index 2efe4e5bdd..8b4500037f 100755 > --- a/t/t4018-diff-funcname.sh > +++ b/t/t4018-diff-funcname.sh > @@ -65,12 +65,26 @@ test_diff_funcname () { > do_change_me "$what" > ' && > > + test_expect_success "setup: $desc (accumulated)" ' > + cat arg.test >>arg.tests && > + cp arg.tests "$what".acc && > + git add "$what".acc && > + do_change_me "$what".acc > + ' && > + > test_expect_success "$desc" ' > git diff -U1 "$what" >diff && > last_diff_context_line diff >actual && > test_cmp expected actual > ' && > > + test_expect_success "$desc (accumulated)" ' > + git diff -U1 "$what".acc >diff && > + last_diff_context_line diff >actual.lines && > + tail -n 1 actual.lines >actual && > + test_cmp expected actual > + ' && > + > test_expect_success "teardown: $desc" ' > # In case any custom config was set immediately before > # the test itself in the test file > @@ -93,6 +107,11 @@ do > echo "$what" >arg.what > ' && > > + test_expect_success "setup: hunk header for $what (accumulated)" ' > + >arg.tests && > + echo "$what.acc diff=$what" >>.gitattributes > + ' && > + > . "$test" > done > > diff --git a/t/t4018/cpp.sh b/t/t4018/cpp.sh > index 185d40d5ef..e0ab749316 100755 > --- a/t/t4018/cpp.sh > +++ b/t/t4018/cpp.sh > @@ -206,6 +206,7 @@ void wrong() > struct RIGHT_iterator_tag {}; > > int ChangeMe; > + > EOF_TEST > > test_diff_funcname 'cpp: template function definition' \ > diff --git a/t/t4018/matlab.sh b/t/t4018/matlab.sh > index f62289148e..fba410e6f5 100755 > --- a/t/t4018/matlab.sh > +++ b/t/t4018/matlab.sh > @@ -31,6 +31,7 @@ EOF_HUNK > %%% RIGHT section > # this is octave script > ChangeMe = 1; > + > EOF_TEST > > test_diff_funcname 'matlab: octave section 2' \ > @@ -40,6 +41,7 @@ EOF_HUNK > ## RIGHT section > # this is octave script > ChangeMe = 1; > + > EOF_TEST > > test_diff_funcname 'matlab: section' \ > @@ -49,4 +51,5 @@ EOF_HUNK > %% RIGHT section > % this is understood by both matlab and octave > ChangeMe = 1; > + > EOF_TEST > -- Hannes
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh index 2efe4e5bdd..8b4500037f 100755 --- a/t/t4018-diff-funcname.sh +++ b/t/t4018-diff-funcname.sh @@ -65,12 +65,26 @@ test_diff_funcname () { do_change_me "$what" ' && + test_expect_success "setup: $desc (accumulated)" ' + cat arg.test >>arg.tests && + cp arg.tests "$what".acc && + git add "$what".acc && + do_change_me "$what".acc + ' && + test_expect_success "$desc" ' git diff -U1 "$what" >diff && last_diff_context_line diff >actual && test_cmp expected actual ' && + test_expect_success "$desc (accumulated)" ' + git diff -U1 "$what".acc >diff && + last_diff_context_line diff >actual.lines && + tail -n 1 actual.lines >actual && + test_cmp expected actual + ' && + test_expect_success "teardown: $desc" ' # In case any custom config was set immediately before # the test itself in the test file @@ -93,6 +107,11 @@ do echo "$what" >arg.what ' && + test_expect_success "setup: hunk header for $what (accumulated)" ' + >arg.tests && + echo "$what.acc diff=$what" >>.gitattributes + ' && + . "$test" done diff --git a/t/t4018/cpp.sh b/t/t4018/cpp.sh index 185d40d5ef..e0ab749316 100755 --- a/t/t4018/cpp.sh +++ b/t/t4018/cpp.sh @@ -206,6 +206,7 @@ void wrong() struct RIGHT_iterator_tag {}; int ChangeMe; + EOF_TEST test_diff_funcname 'cpp: template function definition' \ diff --git a/t/t4018/matlab.sh b/t/t4018/matlab.sh index f62289148e..fba410e6f5 100755 --- a/t/t4018/matlab.sh +++ b/t/t4018/matlab.sh @@ -31,6 +31,7 @@ EOF_HUNK %%% RIGHT section # this is octave script ChangeMe = 1; + EOF_TEST test_diff_funcname 'matlab: octave section 2' \ @@ -40,6 +41,7 @@ EOF_HUNK ## RIGHT section # this is octave script ChangeMe = 1; + EOF_TEST test_diff_funcname 'matlab: section' \ @@ -49,4 +51,5 @@ EOF_HUNK %% RIGHT section % this is understood by both matlab and octave ChangeMe = 1; + EOF_TEST
The existing tests in "t/t4018/" are unrealistic in that they're all setting up small few-line isolated test cases with one thing we could match as a hunk header, right above the one change in the file. Expand those tests by accumulating changes within the same file type in the "test_diff_funcname" function. So e.g. for "bash" we'll end up a "bash.acc" file with 15 s/ChangeMe/IWasChanged/ changes. This stress tests whether the hunk header selection will "jump across" to an earlier change because the match for that is greedier. As it turns out we had one false positive in "t/t4018/cpp.sh" and "t4018/matlab.sh" because of how the tests were structured, we must always give the "ChangeMe" line at least one line of separation from the header, since it was at the end of those tests we'd select the "wrong" header. Let's adjust the spacing to compensate. So in the end we found nothing of interest here, regardless, I think it is useful to continue to test in this mode. It's likely to aid in finding bugs in combinations of our positive and negative matching as we add more built-in patterns. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t4018-diff-funcname.sh | 19 +++++++++++++++++++ t/t4018/cpp.sh | 1 + t/t4018/matlab.sh | 3 +++ 3 files changed, 23 insertions(+)