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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx> wrote:

Hi Elijah

On 14/10/2020 22:13, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <newren@xxxxxxxxx>

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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux