[v2,0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests
mbox series

Message ID 20181209225628.22216-1-szeder.dev@gmail.com
Headers show
Series
  • test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests
Related show

Message

SZEDER Gábor Dec. 9, 2018, 10:56 p.m. UTC
This patch series tries to make reproducing rare failures in flaky
tests easier: it adds the '--stress' option to our test library to run
the test script repeatedly in multiple parallel jobs, in the hope that
the increased load creates enough variance in the timing of the test's
commands that such a failure is eventually triggered.


Notable changes since v1:

  - Made it more Peff-friendly :), namely it will now show the log of
    the failed test and will rename its trash directory.

    Furthermore, '--stress' will now imply '--verbose -x --immediate'.

  - Improved abort handling based on the discussion of the previous
    version.  (As a result, the patch is so heavily modified, that
    'range-diff' with default parameters consideres it a different
    patch; increasing the creation factor then results in one big ugly
    diff of a diff, so I left it as-is.)

  - Added a few new patches, mostly preparatory refactorings, though
    the first one might be considered a bugfix.

  - Other minor cleanups suggested in the previous discussion.


v1: https://public-inbox.org/git/20181204163457.15717-1-szeder.dev@gmail.com/T/


SZEDER Gábor (7):
  test-lib: translate SIGTERM and SIGHUP to an exit
  test-lib: parse some --options earlier
  test-lib: consolidate naming of test-results paths
  test-lib: set $TRASH_DIRECTORY earlier
  test-lib: extract Bash version check for '-x' tracing
  test-lib-functions: introduce the 'test_set_port' helper function
  test-lib: add the '--stress' option to run a test repeatedly under
    load

 t/README                 |  19 +++-
 t/lib-git-daemon.sh      |   2 +-
 t/lib-git-p4.sh          |   9 +-
 t/lib-git-svn.sh         |   2 +-
 t/lib-httpd.sh           |   2 +-
 t/t0410-partial-clone.sh |   1 -
 t/t5512-ls-remote.sh     |   2 +-
 t/test-lib-functions.sh  |  39 +++++++
 t/test-lib.sh            | 227 +++++++++++++++++++++++++++++----------
 9 files changed, 230 insertions(+), 73 deletions(-)

Range-diff:
-:  ---------- > 1:  3a5c926167 test-lib: translate SIGTERM and SIGHUP to an exit
-:  ---------- > 2:  8eee8d7fba test-lib: parse some --options earlier
1:  f4bb53e676 ! 3:  dd20ae5e9a test-lib: consolidate naming of test-results paths
    @@ -4,8 +4,9 @@
     
         There are two places where we strip off any leading path components
         and the '.sh' suffix from the test script's pathname, and there are
    -    two places where we construct the filename of test output files in
    -    't/test-results/'.  The last patch in this series will add even more.
    +    four places where we construct the name of the 't/test-results'
    +    directory or the name of various test-specific files in there.  The
    +    last patch in this series will add even more.
     
         Factor these out into helper variables to avoid repeating ourselves.
     
    @@ -15,22 +16,23 @@
      --- a/t/test-lib.sh
      +++ b/t/test-lib.sh
     @@
    - 	exit 1
    - fi
    + 	esac
    + done
      
     +TEST_NAME="$(basename "$0" .sh)"
    -+TEST_RESULTS_BASE="$TEST_OUTPUT_DIRECTORY/test-results/$TEST_NAME"
    ++TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
    ++TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
     +
      # if --tee was passed, write the output not only to the terminal, but
      # additionally to the file test-results/$BASENAME.out, too.
    - case "$GIT_TEST_TEE_STARTED, $* " in
    + if test "$GIT_TEST_TEE_STARTED" = "done"
     @@
    - 	# do not redirect again
    - 	;;
    - *' --tee '*|*' --va'*|*' -V '*|*' --verbose-log '*)
    + elif test -n "$tee" || test -n "$verbose_log" ||
    +      test -n "$valgrind" || test -n "$valgrind_only"
    + then
     -	mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
     -	BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
    -+	mkdir -p "$(dirname "$TEST_RESULTS_BASE")"
    ++	mkdir -p "$TEST_RESULTS_DIR"
      
      	# Make this filename available to the sub-process in case it is using
      	# --verbose-log.
    @@ -48,8 +50,8 @@
     +	 echo $? >"$TEST_RESULTS_BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
     +	test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
      	exit
    - 	;;
    - esac
    + fi
    + 
     @@
      
      	if test -z "$HARNESS_ACTIVE"
    @@ -58,7 +60,7 @@
     -		mkdir -p "$test_results_dir"
     -		base=${0##*/}
     -		test_results_path="$test_results_dir/${base%.sh}.counts"
    -+		mkdir -p "$(dirname "$TEST_RESULTS_BASE")"
    ++		mkdir -p "$TEST_RESULTS_DIR"
      
     -		cat >"$test_results_path" <<-EOF
     +		cat >"$TEST_RESULTS_BASE.counts" <<-EOF
-:  ---------- > 4:  f3941077e6 test-lib: set $TRASH_DIRECTORY earlier
-:  ---------- > 5:  fefbca96ee test-lib: extract Bash version check for '-x' tracing
2:  9aec8662f9 ! 6:  b4b6844a3e test-lib-functions: introduce the 'test_set_port' helper function
    @@ -27,7 +27,7 @@
             containing the digits 8 or 9 will trigger an error.  Remove all
             leading zeros from the test numbers to prevent this.
     
    -    Note that the Perforce tests are unlike the other tests involving
    +    Note that the 'git p4' tests are unlike the other tests involving
         daemons in that:
     
           - 'lib-git-p4.sh' doesn't use the test's number for unique port as
    @@ -35,7 +35,7 @@
     
           - The port is not overridable via an environment variable.
     
    -    With this patch even Perforce tests will use the test's number as
    +    With this patch even 'git p4' tests will use the test's number as
         default port, and it will be overridable via the P4DPORT environment
         variable.
     
    @@ -161,7 +161,7 @@
     +		BUG "test_set_port requires a variable name"
     +	fi
     +
    -+	eval port=\"\${$var}\"
    ++	eval port=\$$var
     +	case "$port" in
     +	"")
     +		# No port is set in the given env var, use the test
3:  a5aa71f20c < -:  ---------- test-lib: add the '--stress' option to run a test repeatedly under load
-:  ---------- > 7:  8470b55f65 test-lib: add the '--stress' option to run a test repeatedly under load

Comments

Jeff King Dec. 11, 2018, 11:16 a.m. UTC | #1
On Sun, Dec 09, 2018 at 11:56:21PM +0100, SZEDER Gábor wrote:

> This patch series tries to make reproducing rare failures in flaky
> tests easier: it adds the '--stress' option to our test library to run
> the test script repeatedly in multiple parallel jobs, in the hope that
> the increased load creates enough variance in the timing of the test's
> commands that such a failure is eventually triggered.
> 
> Notable changes since v1:
> 
>   - Made it more Peff-friendly :), namely it will now show the log of
>     the failed test and will rename its trash directory.
> 
>     Furthermore, '--stress' will now imply '--verbose -x --immediate'.

:) Thanks. I do sympathize with the notion of keeping orthogonal things
orthogonal, but I hope that this will make the tool a little more
pleasant to use out of the box. We can always tweak the behavior later,
too. It's not like this is something user-facing that we've promised as
a scripting interface.

>   - Improved abort handling based on the discussion of the previous
>     version.  (As a result, the patch is so heavily modified, that
>     'range-diff' with default parameters consideres it a different
>     patch; increasing the creation factor then results in one big ugly
>     diff of a diff, so I left it as-is.)

Yeah, this all looked good to me.

>   - Added a few new patches, mostly preparatory refactorings, though
>     the first one might be considered a bugfix.

I left a few minor comments, but these all looked good to me.

-Peff