git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Jonathan Tan <jonathantanmy@google.com>,
	git@vger.kernel.org, steadmon@google.com
Subject: Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches
Date: Thu, 07 Feb 2019 11:49:42 +0100	[thread overview]
Message-ID: <877eeblvp5.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20190206230813.GC19901@sigill.intra.peff.net>


On Thu, Feb 07 2019, Jeff King wrote:

> On Wed, Feb 06, 2019 at 11:20:32PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> >> So far we've had the convention that these GIT_TEST_* variables,
>> >> e.g. the one for the commit graph, work the same way. Thus we guarantee
>> >> that we get (in theory) 100% coverage even when running the tests in
>> >> this special mode. I think it's better to keep it as-is.
>> >
>> > But what's the point of that? Don't you always have to run the test
>> > suite _twice_, once with the special variable and once without?
>> > Otherwise, you are not testing one case or the other.
>> >
>> > Or are you arguing that one might set many special variables in one go
>> > (to prefer running the suite only twice, instead of 2^N times). In which
>> > case we are better off running the test (as opposed to skipping it), as
>> > it might use one of the _other_ special variables besides
>> > GIT_TEST_PROTOCOL_VERSION.
>> >
>> > I can buy that line of reasoning. It still doesn't cover all cases that
>> > a true 2^N test would, but that clearly isn't going to be practical.
>>
>> Maybe I'm misunderstanding what you're proposing, but as an example,
>> let's say the test suite is just these two tests:
>>
>>     test_expect_success 'some unrelated thing' '...'
>>     test_expect_success 'test protocol v2' 'GIT_TEST_PROTOCOL_VERSION=2 ...'
>>
>> And GIT_TEST_PROTOCOL_VERSION=0 is the default, let's say I want to test
>> with GIT_TEST_PROTOCOL_VERSION=1 for whatever reason,
>>
>> I'd still like both tests to be run, not just 1/2 with
>> GIT_TEST_PROTOCOL_VERSION=1 and 2/2 skipped because it's explicitly
>> testing for the GIT_TEST_PROTOCOL_VERSION=2 case, whereas I asked for a
>> GIT_TEST_PROTOCOL_VERSION=1.
>
> But that's my "why". The second test will run identically in both runs,
> regardless of your setting of GIT_TEST_PROTOCOL_VERSION. So there's
> value if you're only running the suite once in getting full coverage,
> but if you are literally going to run it with and without, then you're
> running the exact same code twice for no reason. And you have to run it
> both with and without, since otherwise all of the _other_ tests aren't
> seeing both options.

Yeah, by always running the 2nd test regardless of what
GIT_TEST_PROTOCOL_VERSION=* is set to we're wasting CPU if we know we're
going to run both with GIT_TEST_PROTOCOL_VERSION=1 and
GIT_TEST_PROTOCOL_VERSION=2.

But we don't know that, and in terms of CPU & time the tests that rely
on any given GIT_TEST_* flag are such a tiny part of the test suite,
that I think it's fine to run them twice in such a scenario to guard
against the case when we just run in one more or the other, and not
both.

>> IOW the point of these tests is to piggy-back on the tests that *aren't*
>> aware of the feature you're trying to test. So
>> e.g. GIT_TEST_COMMIT_GRAPH=true should run our whole test suite with the
>> commit graph, and *also* those tests that are explicitly aware of the
>> commit graph, including those that for some reason would want to test
>> for the case where it isn't enabled (to e.g. test that --contains works
>> without the graph).
>>
>> Otherwise I can't say "I care more about GIT_TEST_COMMIT_GRAPH=true than
>> not", and run just one test run with that, because I'll have blind spots
>> in the commit graph tests themselves, and would then need to do another
>> run with GIT_TEST_COMMIT_GRAPH= set to make sure I have 100% coverage.
>
> So if we are still talking about the same 2-test setup above, which only
> has a GIT_TEST_PROTOCOL_VERSION override, then yeah, I get the point of
> being able to run a "stock" test, and then one with _both_ of the flags
> (GIT_TEST_PROTOCOL_VERSION and GIT_TEST_COMMIT_GRAPH) because you don't
> quite know how each test will interact with all of the "other" flags.

Yeah that's another reason not to skip them, although we could imagine a
prereq where we skip the v2 tests if GIT_TEST_PROTOCOL_VERSION=1 is set
*and* no other GIT_TEST_*=* flag is set, but I think that would also be
a bad idea.

> So now I'm not 100% sure I understand what you're talking about, but I
> think maybe we are actually in agreement. ;)

I think there's two ways to view these GIT_TEST_FOO=BAR facilities:

 1. Run all tests with "FOO" set to "BAR", skip those (via prereq) we
    know would break in that mode.

 2. Ditto, but if a test says "I'm a test for FOO=!BAR" leave it alone.

#2 is what we have now. I read your
<20190206213458.GC12737@sigill.intra.peff.net> as suggesting that we
should move to #1.

You're correct that moving to #1 would force us to more exhaustively
mark things so that if the default moves to "BAR" we just have to flip a
switch.

      reply	other threads:[~2019-02-07 10:49 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-06  0:21 [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches Jonathan Tan
2019-02-06  0:21 ` [PATCH 1/8] tests: define GIT_TEST_PROTOCOL_VERSION Jonathan Tan
2019-02-06 21:58   ` Ævar Arnfjörð Bjarmason
2019-02-07  0:01     ` Jonathan Tan
2019-02-11 20:20   ` Jeff King
2019-02-06  0:21 ` [PATCH 2/8] tests: always test fetch of unreachable with v0 Jonathan Tan
2019-02-11 20:30   ` Jeff King
2019-02-14 19:58     ` Jonathan Tan
2019-02-21 13:49       ` Jeff King
2019-02-22 20:47         ` Junio C Hamano
2019-02-23 13:25           ` Jeff King
2019-02-06  0:21 ` [PATCH 3/8] t5503: fix overspecification of trace expectation Jonathan Tan
2019-02-06  0:21 ` [PATCH 4/8] t5512: compensate for v0 only sending HEAD symrefs Jonathan Tan
2019-02-06  0:21 ` [PATCH 5/8] t5700: only run with protocol version 1 Jonathan Tan
2019-02-06  0:21 ` [PATCH 6/8] tests: fix protocol version for overspecifications Jonathan Tan
2019-02-06  0:21 ` [PATCH 7/8] t5552: compensate for v2 filtering ref adv Jonathan Tan
2019-02-06  0:21 ` [PATCH 8/8] remote-curl: in v2, fill credentials if needed Jonathan Tan
2019-02-06 21:29   ` Jeff King
2019-02-11 19:20     ` Jonathan Tan
2019-02-11 20:38       ` Jeff King
2019-02-06 21:34 ` [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches Jeff King
2019-02-06 21:52   ` Ævar Arnfjörð Bjarmason
2019-02-06 22:10     ` Jeff King
2019-02-06 22:20       ` Ævar Arnfjörð Bjarmason
2019-02-06 23:08         ` Jeff King
2019-02-07 10:49           ` Ævar Arnfjörð Bjarmason [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877eeblvp5.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=peff@peff.net \
    --cc=steadmon@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).