Re: [PATCH v3 1/3] test-lib: allow selecting tests by substring/glob with --run

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Elijah

On 16/10/2020 18:27, Elijah Newren wrote:
> Hi Phillip,
> 
> On Fri, Oct 16, 2020 at 4:41 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Elijah
>>
>> On 14/10/2020 22:13, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <newren@gmail.com>
>>>
>>> Many of our test scripts have several "setup" tests.  It's a lot easier
>>> to say
>>>
>>>      ./t0050-filesystem.sh --run=setup,9
>>>
>>> in order to run all the setup tests as well as test #9, than it is to
>>> track down what all the setup tests are and enter all their numbers in
>>> the list.  Also, I often find myself wanting to run just one or a couple
>>> tests from the test file, but I don't know the numbering of any of the
>>> tests -- to get it I either have to first run the whole test file (or
>>> start counting by hand or figure out some other clever but non-obvious
>>> tricks).  It's really convenient to be able to just look at the test
>>> description(s) and then run
>>>
>>>      ./t6416-recursive-corner-cases.sh --run=symlink
>>>
>>> or
>>>
>>>      ./t6402-merge-rename.sh --run='setup,unnecessary update'
>>
>> The beginning of match_test_selector_list() looks like
>>
>> match_test_selector_list () {
>>          title="$1"
>>          shift
>>          arg="$1"
>>          shift
>>          test -z "$1" && return 0
>>
>>          # Both commas and whitespace are accepted as separators.
>>          OLDIFS=$IFS
>>          IFS='   ,'
>>          set -- $1
>>          IFS=$OLDIFS
>>
>>          # If the first selector is negative we include by default.
>>          include=
>>          case "$1" in
>>                  !*) include=t ;;
>>          esac
>>
>>          for selector
>>          do
>>
>> If I'm reading it correctly the selectors are split on commas and
>> whitespace which would mean we cannot match on "unnecessary update". I
>> think we definitely want the ability to include whitespace in the
>> selectors in order to be able to narrow down the tests that are run. I'm
>> not sure that there is much value in splitting numbers on whitespace as
>> it would mean the user has to quote them on the command line so we can
>> probably just do 'IFS=,'. We'd also need to keep IFS as ',' in the case
>> statement you add below as well rather than restoring it straight after
>> the 'set' statement above.
> 
> Given that t/README explicitly shows examples of space-separated lists
> of numbers,

That's a shame

> I'm worried we're breaking long-built expectations of
> other developers by changing IFS here. 

I agree

> Perhaps I could instead add
> the following paragraph to t/README:
> 
> Note: The argument to --run is split on commas and whitespace into
> separate strings, numbers, and ranges, and picks all tests that match
> any of individual selection criteria.  If the substring you want to
> match from the description text includes a comma or space, use the
> glob character '?' instead.  For example --run='unnecessary?update
> timing' would match on all tests that match either the glob
> *unnecessary?update* or the glob *timing*.
> 
> Does that address your concern?  The '?' will of course match on
> characters other than a space or comma, but the odds that it actually
> matches anything other than what you want is pretty slim, so I suspect
> that is good enough.

'unnecessary?update' is pretty ugly but I agree false matches are 
unlikely to be a problem it practice. Your suggested paragraph looks 
good to me

Thanks

Phillip