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

From: Elijah Newren <newren@gmail.com>
Date: Oct 16, 2020, 17:27 UTC
Message-ID: <CABPp-BHkykWh8L_FYhLR1BCCpPDmc_2q+Tccg_yZ7W8ZHZ4WsA@mail.gmail.com>
In-Reply-To: <2b757512-793d-a6e0-0a50-368061e122dd@gmail.com>

Hi Phillip,

On Fri, Oct 16, 2020 at 4:41 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 59 quoted lines

>
> 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, I'm worried we're breaking long-built expectations of other developers by changing IFS here. 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.