From: Phillip Wood Date: Fri, 16 Oct 2020 18:24:15 GMT Subject: Re: [PATCH v3 1/3] test-lib: allow selecting tests by substring/glob with --run Message-ID: <2fd1231c-9df3-623d-9ae1-0d66526db618@gmail.com> In-Reply-To: Hi Elijah On 16/10/2020 18:27, Elijah Newren wrote: > Hi Phillip, > > On Fri, Oct 16, 2020 at 4:41 AM Phillip Wood wrote: >> >> Hi Elijah >> >> On 14/10/2020 22:13, Elijah Newren via GitGitGadget wrote: >>> From: Elijah Newren >>> >>> 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