Re: [PATCH 08/29] grep: add tests for --threads=N and grep.threads

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, May 11, 2017 at 8:36 PM, Brandon Williams <bmwill@google.com> wrote:
> On 05/11, Ævar Arnfjörð Bjarmason wrote:
>> Add tests for --threads=N being supplied on the command-line, or when
>> grep.threads=N being supplied in the configuration.
>>
>> When the threading support was made run-time configurable in commit
>> 89f09dd34e ("grep: add --threads=<num> option and grep.threads
>> configuration", 2015-12-15) no tests were added for it.
>>
>> In developing a change to the grep code I was able to make
>> '--threads=1 <pat>` segfault, while the test suite still passed. This
>> change fixes that blind spot in the tests.
>>
>> In addition to asserting that asking for N threads shouldn't segfault,
>> test that the grep output given any N is the same.
>>
>> The choice to test only 1..10 as opposed to 1..8 or 1..16 or whatever
>> is arbitrary. Testing 1..1024 works locally for me (but gets
>> noticeably slower as more threads are spawned). Given the structure of
>> the code there's no reason to test an arbitrary number of threads,
>> only 0, 1 and >=2 are special modes of operation.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/t7810-grep.sh | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
>> index daa906b9b0..561709ef6a 100755
>> --- a/t/t7810-grep.sh
>> +++ b/t/t7810-grep.sh
>> @@ -775,6 +775,22 @@ test_expect_success 'grep -W with userdiff' '
>>       test_cmp expected actual
>>  '
>>
>> +for threads in $(test_seq 0 10)
>> +do
>> +     test_expect_success "grep --threads=$threads & -c grep.threads=$threads" "
>> +             git grep --threads=$threads . >actual.$threads &&
>> +             if test $threads -ge 1
>> +             then
>> +                     test_cmp actual.\$(($threads - 1)) actual.$threads
>> +             fi &&
>> +             git -c grep.threads=$threads grep . >actual.$threads &&
>> +             if test $threads -ge 1
>> +             then
>> +                     test_cmp actual.\$(($threads - 1)) actual.$threads
>> +             fi
>> +     "
>> +done
>> +
>
> Is there a test condition to require PTHREADS?  Otherwise this might
> break if git is compiled with NO_PTHREADS.

Good catch, this test works and I'll leave it like it is in a v2, but
explain it better in the commit message.

We just ignore --threads= with NO_PTHREADS, however later in this
series I introduce a warning for --threads when no threads are
supported, see "grep: given --threads with NO_PTHREADS=YesPlease,
warn".

But --threads=N still works, so this as a side-benefit tests that
--threads=N still works with NO_PTHREADS, and tests don't fail if we
spew to stderr.

The commit that adds the warning then tests for --threads getting a
warning with NO_THREADS=Y.