Re: [PATCH v2 01/18] t7107, t7526: directly test parse_pathspec_file()

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
To: Junio C Hamano <gitster@pobox.com>,
	Alexandr Miloslavskiy via GitGitGadget  <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Phillip Wood" <phillip.wood123@gmail.com>,
	"Emily Shaffer" <emilyshaffer@google.com>,
	"Derrick Stolee" <stolee@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v2 01/18] t7107, t7526: directly test parse_pathspec_file()
Date: Thu, 19 Dec 2019 18:19:21 +0100	[thread overview]
Message-ID: <0cf9d6a8-f804-c828-28f5-e60089190167@syntevo.com> (raw)
In-Reply-To: <xmqqwoatcn5u.fsf@gitster-ct.c.googlers.com>

On 18.12.2019 22:57, Junio C Hamano wrote:

>> In my previous patches, `parse_pathspec_file()` was tested indirectly by
>> invoking `git reset` and `git commit` with properly crafted inputs. This
>> has some disadvantages:
>> 1) A number of tests are copy&pasted for every command where
>>     `--pathspec-from-file` is supported. With just two commands, it
>>     wasn't too bad, but I'm going to extend support to many more
>>     commands, which would make a handful of low-value tests.
>> 2) Tests are located in suboptimal test packages
>> 3) Tests are indirect
> 
> That cuts both ways.  For a developer who is too narrowly focused
> (because s/he spent enough time staring at the code), testing the
> underlying machinery in a more direct way does feel attractive, but
> at the same time, what matters to the end users is how well the
> feature, when integrated into the commands they use (not the test
> scaffolding like the "test-parse-pathspec-file" command), works.
> 
> So "indirect" is not necessarily a bad thing.

I agree that it cuts both ways.

Just recently I had an (unrelated) discussion with Johannes Schindelin 
who forced me to drop 2 of 3 tests (where #3 also by chance covered #1 
#2) in some other PR, because too many tests is also evil.

To verify: I see 13 git commands that could benefit from 
--pathspec-from-file. There are 6 tests that in fact test underlying 
machinery, which can't be easily influenced by bugs in command's code. 
That makes 12*6 = 72 tests that are copy&pasted and doesn't test 
anything new.

Do you suggest to return _all_ tests back into every command? (but also 
keep the new direct tests, I assume)