Interpret `--test_filter` in Bash tests as a list of globs instead of test names. Currently, the `--test_filter` argument is treated as a comma-separated list of tests to match, which is redundant to `--test_arg`. Change it to be interpreted as a `:` separated list of [Bash globs](https://www.gnu.org/software/bash/manual/bash.html#Pattern-Matching). PiperOrigin-RevId: 420317559
diff --git a/src/test/shell/unittest.bash b/src/test/shell/unittest.bash index b8b5e4f..75b97d1 100644 --- a/src/test/shell/unittest.bash +++ b/src/test/shell/unittest.bash
@@ -106,7 +106,18 @@ # functions that should be run. By # default, all tests called test_* are # run. -if [ $# -gt 0 ]; then + +_TEST_FILTERS=() # List of globs to use to filter the tests. + # If non-empty, all tests matching at least one + # of the globs are run and test list provided in + # the arguments is ignored if present. + +if (( $# > 0 )); then + ( + IFS=':' + echo "WARNING: Passing test names in arguments (--test_arg) is deprecated, please use --test_filter='$*' instead." >&2 + ) + # Legacy behavior is to ignore missing regexp, but with errexit # the following line fails without || true. # TODO(dmarting): maybe we should revisit the way of selecting @@ -119,8 +130,12 @@ # TESTBRIDGE_TEST_ONLY contains the value of --test_filter, if any. We want to # preferentially use that instead of $@ to determine which tests to run. if [[ ${TESTBRIDGE_TEST_ONLY:-} != "" ]]; then - # Split TESTBRIDGE_TEST_ONLY on comma and put the results into an array. - IFS=',' read -r -a TESTS <<< "$TESTBRIDGE_TEST_ONLY" + if (( ${#TESTS[@]} != 0 )); then + echo "WARNING: Both --test_arg and --test_filter specified, ignoring --test_arg" >&2 + TESTS=() + fi + # Split TESTBRIDGE_TEST_ONLY on colon and store it in `_TEST_FILTERS` array. + IFS=':' read -r -a _TEST_FILTERS <<< "$TESTBRIDGE_TEST_ONLY" fi TEST_verbose="true" # Whether or not to be verbose. A @@ -502,8 +517,9 @@ [ "$TEST_SHARD_INDEX" -lt 0 -o "$TEST_SHARD_INDEX" -ge "$TEST_TOTAL_SHARDS" ] && { echo "Invalid shard $shard_index" >&2; exit 1; } - TESTS=$(for test in "${TESTS[@]}"; do echo "$test"; done | - awk "NR % $TEST_TOTAL_SHARDS == $TEST_SHARD_INDEX") + read -rd $'\0' -a TESTS < <( + for test in "${TESTS[@]}"; do echo "$test"; done | + awk "NR % $TEST_TOTAL_SHARDS == $TEST_SHARD_INDEX" && echo -en '\0') [ -z "${TEST_SHARD_STATUS_FILE-}" ] || touch "$TEST_SHARD_STATUS_FILE" } @@ -652,7 +668,29 @@ # working set), use them all. if [ ${#TESTS[@]} -eq 0 ]; then # Even if there aren't any tests, this needs to succeed. - TESTS=$(declare -F | awk '{print $3}' | grep ^test_ || true) + local all_tests=() + read -d $'\0' -ra all_tests < <( + declare -F | awk '{print $3}' | grep ^test_ || true; echo -en '\0') + + if (( "${#_TEST_FILTERS[@]}" == 0 )); then + # Use ${array[@]+"${array[@]}"} idiom to avoid errors when running with + # Bash version <= 4.4 with `nounset` when `all_tests` is empty ( + # https://github.com/bminor/bash/blob/a0c0a00fc419b7bc08202a79134fcd5bc0427071/CHANGES#L62-L63). + TESTS=("${all_tests[@]+${all_tests[@]}}") + else + for t in "${all_tests[@]+${all_tests[@]}}"; do + local matches=0 + for f in "${_TEST_FILTERS[@]}"; do + # We purposely want to glob match. + # shellcheck disable=SC2053 + [[ "$t" = $f ]] && matches=1 && break + done + if (( matches )); then + TESTS+=("$t") + fi + done + fi + elif [ -n "${TEST_WARNINGS_OUTPUT_FILE:-}" ]; then if grep -q "TESTS=" "$TEST_script" ; then echo "TESTS variable overridden in sh_test. Please remove before submitting" \ @@ -661,7 +699,7 @@ fi # Reset TESTS in the common case where it contains a single empty string. - if [ -z "${TESTS[*]}" ]; then + if [ -z "${TESTS[*]-}" ]; then TESTS=() fi local original_tests_size=${#TESTS[@]}
diff --git a/src/test/shell/unittest_test.py b/src/test/shell/unittest_test.py index 6eabb46..1314290 100644 --- a/src/test/shell/unittest_test.py +++ b/src/test/shell/unittest_test.py
@@ -25,6 +25,7 @@ import stat import subprocess import tempfile +import textwrap import unittest # The test setup for this external test is forwarded to the internal bash test. @@ -137,7 +138,7 @@ # Base on the current dir return "%s/.." % os.getcwd() - def execute_test(self, filename, env=None): + def execute_test(self, filename, env=None, args=()): """Executes the file and stores the results.""" filepath = os.path.join(self.work_dir, filename) @@ -153,7 +154,7 @@ for k, v in env.items(): test_env[k] = str(v) completed = subprocess.run( - [filepath], + [filepath, *args], env=test_env, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, @@ -359,6 +360,206 @@ result.assertSuccess("empty test suite") result.assertNotLogMessage("No tests") + def test_filter_runs_only_matching_test(self): + self.write_file( + "thing.sh", + textwrap.dedent(""" + function test_abc() { + : + } + + function test_def() { + echo "running def" + } + + run_suite "tests to filter" + """)) + + result = self.execute_test( + "thing.sh", env={"TESTBRIDGE_TEST_ONLY": "test_a*"}) + + result.assertSuccess("tests to filter") + result.assertTestPassed("test_abc") + result.assertNotLogMessage("running def") + + def test_filter_prefix_match_only_skips_test(self): + self.write_file( + "thing.sh", + textwrap.dedent(""" + function test_abc() { + echo "running abc" + } + + run_suite "tests to filter" + """)) + + result = self.execute_test( + "thing.sh", env={"TESTBRIDGE_TEST_ONLY": "test_a"}) + + result.assertNotSuccess("tests to filter") + result.assertLogMessage("No tests found.") + + def test_filter_multiple_globs_runs_tests_matching_any(self): + self.write_file( + "thing.sh", + textwrap.dedent(""" + function test_abc() { + echo "running abc" + } + + function test_def() { + echo "running def" + } + + run_suite "tests to filter" + """)) + + result = self.execute_test( + "thing.sh", env={"TESTBRIDGE_TEST_ONLY": "donotmatch:*a*"}) + + result.assertSuccess("tests to filter") + result.assertTestPassed("test_abc") + result.assertNotLogMessage("running def") + + def test_filter_character_group_runs_only_matching_tests(self): + self.write_file( + "thing.sh", + textwrap.dedent(""" + function test_aaa() { + : + } + + function test_daa() { + : + } + + function test_zaa() { + echo "running zaa" + } + + run_suite "tests to filter" + """)) + + result = self.execute_test( + "thing.sh", env={"TESTBRIDGE_TEST_ONLY": "test_[a-f]aa"}) + + result.assertSuccess("tests to filter") + result.assertTestPassed("test_aaa") + result.assertTestPassed("test_daa") + result.assertNotLogMessage("running zaa") + + def test_filter_sharded_runs_subset_of_filtered_tests(self): + for index in range(2): + with self.subTest(index=index): + self.__filter_sharded_runs_subset_of_filtered_tests(index) + + def __filter_sharded_runs_subset_of_filtered_tests(self, index): + self.write_file( + "thing.sh", + textwrap.dedent(""" + function test_a0() { + echo "running a0" + } + + function test_a1() { + echo "running a1" + } + + function test_bb() { + echo "running bb" + } + + run_suite "tests to filter" + """)) + + result = self.execute_test( + "thing.sh", + env={ + "TESTBRIDGE_TEST_ONLY": "test_a*", + "TEST_TOTAL_SHARDS": 2, + "TEST_SHARD_INDEX": index + }) + + result.assertSuccess("tests to filter") + # The sharding logic is shifted by 1, starts with 2nd shard. + result.assertTestPassed("test_a" + str(index ^ 1)) + result.assertLogMessage("running a" + str(index ^ 1)) + result.assertNotLogMessage("running a" + str(index)) + result.assertNotLogMessage("running bb") + + def test_arg_runs_only_matching_test_and_issues_warning(self): + self.write_file( + "thing.sh", + textwrap.dedent(""" + function test_abc() { + : + } + + function test_def() { + echo "running def" + } + + run_suite "tests to filter" + """)) + + result = self.execute_test("thing.sh", args=["test_abc"]) + + result.assertSuccess("tests to filter") + result.assertTestPassed("test_abc") + result.assertNotLogMessage("running def") + result.assertLogMessage( + r"WARNING: Passing test names in arguments \(--test_arg\) is " + "deprecated, please use --test_filter='test_abc' instead.") + + def test_arg_multiple_tests_issues_warning_with_test_filter_command(self): + self.write_file( + "thing.sh", + textwrap.dedent(""" + function test_abc() { + : + } + + function test_def() { + : + } + + run_suite "tests to filter" + """)) + + result = self.execute_test("thing.sh", args=["test_abc", "test_def"]) + + result.assertSuccess("tests to filter") + result.assertTestPassed("test_abc") + result.assertTestPassed("test_def") + result.assertLogMessage( + r"WARNING: Passing test names in arguments \(--test_arg\) is " + "deprecated, please use --test_filter='test_abc:test_def' instead.") + + def test_arg_and_filter_ignores_arg(self): + self.write_file( + "thing.sh", + textwrap.dedent(""" + function test_abc() { + : + } + + function test_def() { + echo "running def" + } + + run_suite "tests to filter" + """)) + + result = self.execute_test( + "thing.sh", args=["test_def"], env={"TESTBRIDGE_TEST_ONLY": "test_a*"}) + + result.assertSuccess("tests to filter") + result.assertTestPassed("test_abc") + result.assertNotLogMessage("running def") + result.assertLogMessage( + "WARNING: Both --test_arg and --test_filter specified, ignoring --test_arg" + ) + if __name__ == "__main__": unittest.main()