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

On Wed, Oct 14, 2020 at 11:09 AM Jeff King [off-list ref] wrote:

On Wed, Oct 14, 2020 at 10:50:03AM -0700, Elijah Newren wrote:

quoted

quoted

quoted

@@ -819,9 +821,8 @@ match_test_selector_list () {
                      *)
                              if expr "z$selector" : "z[0-9]*[^0-9]" >/dev/null
                              then
-                                     echo "error: $title: invalid non-numeric in test" \
-                                             "selector: '$orig_selector'" >&2
-                                     exit 1
+                                     echo "$title" | grep -q "$selector" && return
+                                     continue
                              fi

I like that you allow regexes. It's unfortunate that the skip-check
costs us a process in every test. It may not be that big a deal since we
only pay it if you use a non-numeric selector. But I wonder if there's
any reason not to use "expr" here, as well.

I originally used [[ $title =~ "$selector" ]] in order to avoid the
sub-shell...but that was bash-specific.  I briefly looked to see if
there was a shell portable way to handle regexes, wasn't having much
luck, and decided that this is only likely to arise when people are
passing --run and thus only running a single script and they avoid all
the subprocesses that would have been invoked inside the test, so it's
still a big net win overall.  Does expr handle regexes, portably?  Or
are you suggesting dropping the regex handling and limit it to
substring matching?  In either case, does using expr save us anything
(isn't expr a shell command)?

There's an expr command doing a regex match in the diff context quoted
above. :)

Oh, indeed.  How'd I miss that?

I was wrong that it would save us a process, though. I thought it was a
builtin in modern shells, but it doesn't appear to be in either dash or
bash. So there's little reason to prefer it over grep here (switching to
globs could be done with case and would save a process).

I guess it's at least slightly more consistent with the surrounding code.

Oh, and I found a bug in that my code was not handling negated
substrings correctly, e.g.
   ./test-script.sh --run="!rename"
would run all the tests instead of excluding the ones that had
"rename" as a substring.  So I'll send a reroll fixing that.