Re: [PATCH v3 1/3] test-lib: allow selecting tests by substring/glob with --run
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Oct 16, 2020, 18:24 UTC
- Message-ID
- <2fd1231c-9df3-623d-9ae1-0d66526db618@gmail.com>
- In-Reply-To
- <CABPp-BHkykWh8L_FYhLR1BCCpPDmc_2q+Tccg_yZ7W8ZHZ4WsA@mail.gmail.com>
Hi Elijah
On 16/10/2020 18:27, Elijah Newren wrote:
Show 65 quoted lines
> 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
Show 15 quoted lines
> 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