On Wed, Mar 22, 2017 at 11:36 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Wed, Mar 22, 2017 at 03:26:21PM -0700, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>>
>> > of things you think we should be putting in the test suite. I.e.
>> > should the tests be:
>> >
>> > a) Only be a collection of invocations of git we'd be comfortable
>> > showing to someone as "this works, and this is how you should do it",
>> > or things that explicitly fail marked with test_must_fail.
>> >
>> > b) or a) && also various surprising combinations of things we don't
>> > necessarily want to encourage or even support in the future, but which
>> > are in there so if we change them, we at least know our change changed
>> > something that worked before.
>>
>> I am strongly inclined to (a). If we cannot decide when we designed
>> the feature, and we anticipate that we may want to change it later,
>> then documenting the choice in a test or two may be a way to remind
>> the choice we happened to have made, but in general I do not think
>> we want to promise (to ourselves) more than what we are willing to
>> commit to.
>
> I've occasionally[1] added tests that are "what we happen to produce
> now", but I almost always mark them with a comment either in the test
> script or in the commit message. What I'm _most_ concerned about is a
> developer later breaking the test, but being unsure if they were
> breaking some real-world case (and not being able to find clues in the
> history).
>
> A secondary concern would be people using the test snippets as guidance
> on what is normal or encouraged.
>
> So I could live with these patches, but I'd prefer to see a comment
> somewhere. And I think I'd have a slight inclination to just stick to
> (a) in the first place, unless there is a really good reason to cover
> the test (like that we do not care between behaviors X and Y, but we
> need to check that it does one of them, and not Z).
Right, or in this case something where we're testing for behavior we
documented for a long time, but never really intended to support.
Junio would you be fine with just this on top:
diff --git a/t/README b/t/README
index 4982d1c521..9e079a360a 100644
--- a/t/README
+++ b/t/README
@@ -379,2 +379,5 @@ Do:
+ - Include tests which assert that the desired & recommended behavior
+ of commands is preserved.
+
- Put all code inside test_expect_success and other assertions.
@@ -424,2 +427,17 @@ Don't:
+ - Include tests which exhaustively test for various edge cases or
+ unintended emergent behavior which we're not interested in
+ supporting in the future.
+
+ An exception to this is are cases where we don't care about
+ different behaviors X and Y, but we need to check that it does one
+ of them, and not Z.
+
+ Another exception are cases where our documentation might
+ unintentionally stated or implied that something was supported or
+ recommended, but we'd like to discourage its use going forward.
+
+ In both of the above cases please prominently comment the test
+ indicating that you're testing for one of these two cases.
+
- exit() within a <script> part.
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 0a7ebf5358..35402ad9a0 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -350,2 +350,6 @@ test_expect_success 'tag -l can accept multiple patterns' '
+# Between around v1.7.6.1 & v2.13.0 the documentation unintentionally
+# implied that --list was what took the <pattern>, not that patterns
+# should be clustered at the very end. This test should not imply that
+# this is a sane thing to support.
test_expect_success 'tag -l can accept multiple patterns interleaved
with -l or --list options' '
Or do you think the "long documented but unintentional" argument isn't
worth a test, in which case squash this:
diff --git a/t/README b/t/README
index 9e079a360a..9f85b8d1cd 100644
--- a/t/README
+++ b/t/README
@@ -433,12 +433,8 @@ Don't:
different behaviors X and Y, but we need to check that it does one
of them, and not Z.
- Another exception are cases where our documentation might
- unintentionally stated or implied that something was supported or
- recommended, but we'd like to discourage its use going forward.
-
- In both of the above cases please prominently comment the test
- indicating that you're testing for one of these two cases.
+ In that case please prominently comment the test indicating that
+ you're testing for one of these two cases.
- exit() within a <script> part.
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 35402ad9a0..83772f6003 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -348,19 +348,6 @@ test_expect_success 'tag -l can accept multiple patterns' '
test_cmp expect actual
'
-# Between around v1.7.6.1 & v2.13.0 the documentation unintentionally
-# implied that --list was what took the <pattern>, not that patterns
-# should be clustered at the very end. This test should not imply that
-# this is a sane thing to support.
-test_expect_success 'tag -l can accept multiple patterns interleaved
with -l or --list options' '
- git tag -l "v1*" "v0*" >actual &&
- test_cmp expect actual &&
- git tag -l "v1*" --list "v0*" >actual &&
- test_cmp expect actual &&
- git tag -l "v1*" "v0*" -l --list >actual &&
- test_cmp expect actual
-'
-
test_expect_success 'listing tags in column' '
COLUMNS=40 git tag -l --column=row >actual &&
cat >expected <<\EOF &&