| Message ID | 9c8b6a1a943261635fa09bed22ae36e368686f15.1602710025.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make test selection easier by specifying description substrings instead of just numeric counters | expand |
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. Best Wishes Phillip > Add such an ability to test selection which relies on merely matching > against the test description. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > t/README | 29 +++++++++++++++++++---------- > t/t0000-basic.sh | 41 +++++++++++++++++++++++++---------------- > t/test-lib.sh | 16 ++++++++++------ > 3 files changed, 54 insertions(+), 32 deletions(-) > > diff --git a/t/README b/t/README > index 2adaf7c2d2..0a8b60b5c7 100644 > --- a/t/README > +++ b/t/README > @@ -258,16 +258,13 @@ For an individual test suite --run could be used to specify that > only some tests should be run or that some tests should be > excluded from a run. > > -The argument for --run is a list of individual test numbers or > -ranges with an optional negation prefix that define what tests in > -a test suite to include in the run. A range is two numbers > -separated with a dash and matches a range of tests with both ends > -been included. You may omit the first or the second number to > -mean "from the first test" or "up to the very last test" > -respectively. > - > -Optional prefix of '!' means that the test or a range of tests > -should be excluded from the run. > +The argument for --run, <test-selector>, is a list of description > +substrings or globs or individual test numbers or ranges with an > +optional negation prefix (of '!') that define what tests in a test > +suite to include (or exclude, if negated) in the run. A range is two > +numbers separated with a dash and matches a range of tests with both > +ends been included. You may omit the first or the second number to > +mean "from the first test" or "up to the very last test" respectively. > > If --run starts with an unprefixed number or range the initial > set of tests to run is empty. If the first item starts with '!' > @@ -317,6 +314,18 @@ test in the test suite except from 7 up to 11: > > $ sh ./t9200-git-cvsexport-commit.sh --run='!7-11' > > +Sometimes there may be multiple tests with e.g. "setup" in their name > +that are needed and rather than figuring out the number for all of them > +we can just use "setup" as a substring/glob to match against the test > +description: > + > + $ sh ./t0050-filesystem.sh --run=setup,9-11 > + > +or one could select both the setup tests and the rename ones (assuming all > +relevant tests had those words in their descriptions): > + > + $ sh ./t0050-filesystem.sh --run=setup,rename > + > Some tests in a test suite rely on the previous tests performing > certain actions, specifically some tests are designated as > "setup" test, so you cannot _arbitrarily_ disable one test and > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh > index 923281af93..07105b2078 100755 > --- a/t/t0000-basic.sh > +++ b/t/t0000-basic.sh > @@ -705,7 +705,31 @@ test_expect_success '--run empty selectors' " > EOF > " > > -test_expect_success '--run invalid range start' " > +test_expect_success '--run substring selector' " > + run_sub_test_lib_test run-substring-selector \ > + '--run empty selectors' \ > + --run='relevant' <<-\\EOF && > + test_expect_success \"relevant test\" 'true' > + for i in 1 2 3 4 5 6 > + do > + test_expect_success \"other test #\$i\" 'true' > + done > + test_done > + EOF > + check_sub_test_lib_test run-substring-selector <<-\\EOF > + > ok 1 - relevant test > + > ok 2 # skip other test #1 (--run) > + > ok 3 # skip other test #2 (--run) > + > ok 4 # skip other test #3 (--run) > + > ok 5 # skip other test #4 (--run) > + > ok 6 # skip other test #5 (--run) > + > ok 7 # skip other test #6 (--run) > + > # passed all 7 test(s) > + > 1..7 > + EOF > +" > + > +test_expect_success '--run keyword selection' " > run_sub_test_lib_test_err run-inv-range-start \ > '--run invalid range start' \ > --run='a-5' <<-\\EOF && > @@ -735,21 +759,6 @@ test_expect_success '--run invalid range end' " > EOF_ERR > " > > -test_expect_success '--run invalid selector' " > - run_sub_test_lib_test_err run-inv-selector \ > - '--run invalid selector' \ > - --run='1?' <<-\\EOF && > - test_expect_success \"passing test #1\" 'true' > - test_done > - EOF > - check_sub_test_lib_test_err run-inv-selector \ > - <<-\\EOF_OUT 3<<-\\EOF_ERR > - > FATAL: Unexpected exit with code 1 > - EOF_OUT > - > error: --run: invalid non-numeric in test selector: '1?' > - EOF_ERR > -" > - > > test_set_prereq HAVEIT > haveit=no > diff --git a/t/test-lib.sh b/t/test-lib.sh > index ef31f40037..a040d54a76 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -769,6 +769,8 @@ match_pattern_list () { > } > > match_test_selector_list () { > + operation="$1" > + shift > title="$1" > shift > arg="$1" > @@ -805,13 +807,13 @@ match_test_selector_list () { > *-*) > if expr "z${selector%%-*}" : "z[0-9]*[^0-9]" >/dev/null > then > - echo "error: $title: invalid non-numeric in range" \ > + echo "error: $operation: invalid non-numeric in range" \ > "start: '$orig_selector'" >&2 > exit 1 > fi > if expr "z${selector#*-}" : "z[0-9]*[^0-9]" >/dev/null > then > - echo "error: $title: invalid non-numeric in range" \ > + echo "error: $operation: invalid non-numeric in range" \ > "end: '$orig_selector'" >&2 > exit 1 > fi > @@ -819,9 +821,11 @@ 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 > + case "$title" in *${selector}*) > + include=$positive > + ;; > + esac > + continue > fi > esac > > @@ -1031,7 +1035,7 @@ test_skip () { > skipped_reason="GIT_SKIP_TESTS" > fi > if test -z "$to_skip" && test -n "$run_list" && > - ! match_test_selector_list '--run' $test_count "$run_list" > + ! match_test_selector_list '--run' "$1" $test_count "$run_list" > then > to_skip=t > skipped_reason="--run" >
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, 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.
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@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 > 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
Elijah Newren <newren@gmail.com> writes: > 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: Unlike something that would affect end-users, I think it is OK to break backward compatibility one-way. If you suddenly forbid spaces and force our developers to use comma and nothing else, in muscle memory of their fingers and/or in their scripts, in a version merged to 'master', as long as their new habit and updated scripts use comma consistently, they work fine on 'maint', right? If there is no such "works on both sides of the flag day" choice, it is a different story, of course, but comma should work fine for us in this case. Thanks.
On Fri, Oct 16, 2020 at 01:02:16PM -0700, Junio C Hamano wrote: > Elijah Newren <newren@gmail.com> writes: > > > 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: > > Unlike something that would affect end-users, I think it is OK to > break backward compatibility one-way. If you suddenly forbid spaces > and force our developers to use comma and nothing else, in muscle > memory of their fingers and/or in their scripts, in a version merged > to 'master', as long as their new habit and updated scripts use > comma consistently, they work fine on 'maint', right? > > If there is no such "works on both sides of the flag day" choice, it > is a different story, of course, but comma should work fine for us > in this case. I was just about to write the same argument. :) -Peff
diff --git a/t/README b/t/README index 2adaf7c2d2..0a8b60b5c7 100644 --- a/t/README +++ b/t/README @@ -258,16 +258,13 @@ For an individual test suite --run could be used to specify that only some tests should be run or that some tests should be excluded from a run. -The argument for --run is a list of individual test numbers or -ranges with an optional negation prefix that define what tests in -a test suite to include in the run. A range is two numbers -separated with a dash and matches a range of tests with both ends -been included. You may omit the first or the second number to -mean "from the first test" or "up to the very last test" -respectively. - -Optional prefix of '!' means that the test or a range of tests -should be excluded from the run. +The argument for --run, <test-selector>, is a list of description +substrings or globs or individual test numbers or ranges with an +optional negation prefix (of '!') that define what tests in a test +suite to include (or exclude, if negated) in the run. A range is two +numbers separated with a dash and matches a range of tests with both +ends been included. You may omit the first or the second number to +mean "from the first test" or "up to the very last test" respectively. If --run starts with an unprefixed number or range the initial set of tests to run is empty. If the first item starts with '!' @@ -317,6 +314,18 @@ test in the test suite except from 7 up to 11: $ sh ./t9200-git-cvsexport-commit.sh --run='!7-11' +Sometimes there may be multiple tests with e.g. "setup" in their name +that are needed and rather than figuring out the number for all of them +we can just use "setup" as a substring/glob to match against the test +description: + + $ sh ./t0050-filesystem.sh --run=setup,9-11 + +or one could select both the setup tests and the rename ones (assuming all +relevant tests had those words in their descriptions): + + $ sh ./t0050-filesystem.sh --run=setup,rename + Some tests in a test suite rely on the previous tests performing certain actions, specifically some tests are designated as "setup" test, so you cannot _arbitrarily_ disable one test and diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 923281af93..07105b2078 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -705,7 +705,31 @@ test_expect_success '--run empty selectors' " EOF " -test_expect_success '--run invalid range start' " +test_expect_success '--run substring selector' " + run_sub_test_lib_test run-substring-selector \ + '--run empty selectors' \ + --run='relevant' <<-\\EOF && + test_expect_success \"relevant test\" 'true' + for i in 1 2 3 4 5 6 + do + test_expect_success \"other test #\$i\" 'true' + done + test_done + EOF + check_sub_test_lib_test run-substring-selector <<-\\EOF + > ok 1 - relevant test + > ok 2 # skip other test #1 (--run) + > ok 3 # skip other test #2 (--run) + > ok 4 # skip other test #3 (--run) + > ok 5 # skip other test #4 (--run) + > ok 6 # skip other test #5 (--run) + > ok 7 # skip other test #6 (--run) + > # passed all 7 test(s) + > 1..7 + EOF +" + +test_expect_success '--run keyword selection' " run_sub_test_lib_test_err run-inv-range-start \ '--run invalid range start' \ --run='a-5' <<-\\EOF && @@ -735,21 +759,6 @@ test_expect_success '--run invalid range end' " EOF_ERR " -test_expect_success '--run invalid selector' " - run_sub_test_lib_test_err run-inv-selector \ - '--run invalid selector' \ - --run='1?' <<-\\EOF && - test_expect_success \"passing test #1\" 'true' - test_done - EOF - check_sub_test_lib_test_err run-inv-selector \ - <<-\\EOF_OUT 3<<-\\EOF_ERR - > FATAL: Unexpected exit with code 1 - EOF_OUT - > error: --run: invalid non-numeric in test selector: '1?' - EOF_ERR -" - test_set_prereq HAVEIT haveit=no diff --git a/t/test-lib.sh b/t/test-lib.sh index ef31f40037..a040d54a76 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -769,6 +769,8 @@ match_pattern_list () { } match_test_selector_list () { + operation="$1" + shift title="$1" shift arg="$1" @@ -805,13 +807,13 @@ match_test_selector_list () { *-*) if expr "z${selector%%-*}" : "z[0-9]*[^0-9]" >/dev/null then - echo "error: $title: invalid non-numeric in range" \ + echo "error: $operation: invalid non-numeric in range" \ "start: '$orig_selector'" >&2 exit 1 fi if expr "z${selector#*-}" : "z[0-9]*[^0-9]" >/dev/null then - echo "error: $title: invalid non-numeric in range" \ + echo "error: $operation: invalid non-numeric in range" \ "end: '$orig_selector'" >&2 exit 1 fi @@ -819,9 +821,11 @@ 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 + case "$title" in *${selector}*) + include=$positive + ;; + esac + continue fi esac @@ -1031,7 +1035,7 @@ test_skip () { skipped_reason="GIT_SKIP_TESTS" fi if test -z "$to_skip" && test -n "$run_list" && - ! match_test_selector_list '--run' $test_count "$run_list" + ! match_test_selector_list '--run' "$1" $test_count "$run_list" then to_skip=t skipped_reason="--run"