git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC PATCH 3/3] test-lib: add the '--stress' option to run a test repeatedly under load

From
Jeff King <peff@peff.net>
Date
Dec 5, 2018, 21:56 UTC
Message-ID
<20181205215621.GE19936@sigill.intra.peff.net>
In-Reply-To
<20181205140106.GM30222@szeder.dev>
On Wed, Dec 05, 2018 at 03:01:06PM +0100, SZEDER Gábor wrote:
Show 21 quoted lines
> > >   - Make '--stress' imply '--verbose-log' and discard the test's
> > >     standard ouput and error; dumping the output of several parallel
> > >     tests to the terminal would create a big ugly mess.
> > 
> > Makes sense. My script just redirects the output to a file, but it
> > predates --verbose-log.
> > 
> > My script always runs with "-x". I guess it's not that hard to add it if
> > you want, but it is annoying to finally get a failure after several
> > minutes only to realize that your are missing some important
> > information. ;)
> > 
> > Ditto for "-i", which leaves more readable output (you know the
> > interesting part is at the end of the log), and leaves a trash directory
> > state that is more likely to be useful for inspecting.
> 
> I wanted to imply only the bare minimum of options that are necessary
> to prevent the tests from stomping on each other.  Other than that I
> agree and wouldn't run '--stress' without '-x -i' myself, and at one
> point considered to recommend '-x -i' in the description of
> '--stress'.

Yeah, it probably is the right thing to make the options as orthogonal as possible, and let the user decide what they want. I was just wondering if I could replace my "stress" script with this. But I think I'd still want it to remember to use a sane set of options (including "--root" in my case).

Show 16 quoted lines
> > > 'wait' for all parallel jobs before exiting (either because a failure
> > > was found or because the user lost patience and aborted the stress
> > > test), allowing the still running tests to finish.  Otherwise the "OK
> > > X.Y" progress output from the last iteration would likely arrive after
> > > the user got back the shell prompt, interfering with typing in the
> > > next command.  OTOH, this waiting might induce a considerable delay
> > > between hitting ctrl-C and the test actually exiting; I'm not sure
> > > this is the right tradeoff.
> > 
> > If there is a delay, it's actually quite annoying to _not_ wait; you
> > start doing something in the terminal, and then a bunch of extra test
> > statuses print at a random time.
> 
> At least the INT/TERM/etc. handler should say something like "Waiting
> for background jobs to finish", to let the user know why it doesn't
> exit right away.

That seems reasonable. There's also no point in waiting for them to finish if we know we're aborting anyway. Could we just kill them all?

I guess it's a little tricky, because $! is going to give us the pid of each subshell. We actually want to kill the test sub-process. That takes a few contortions, but the output is nice (every sub-job immediately says "ABORTED ...", and the final process does not exit until the whole tree is done):

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9b7f687396..357dead3ff 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -98,8 +98,9 @@ done,*)
 	mkdir -p "$(dirname "$TEST_RESULTS_BASE")"
 	stressfail="$TEST_RESULTS_BASE.stress-failed"
 	rm -f "$stressfail"
-	trap 'echo aborted >"$stressfail"' TERM INT HUP
+	trap 'echo aborted >"$stressfail"; kill $job_pids 2>/dev/null; wait' TERM INT HUP
 
+	job_pids=
 	job_nr=0
 	while test $job_nr -lt "$job_count"
 	do
@@ -108,10 +109,13 @@ done,*)
 			GIT_TEST_STRESS_JOB_NR=$job_nr
 			export GIT_TEST_STRESS_STARTED GIT_TEST_STRESS_JOB_NR
 
+			trap 'kill $test_pid 2>/dev/null' TERM
+
 			cnt=0
 			while ! test -e "$stressfail"
 			do
-				if $TEST_SHELL_PATH "$0" "$@" >/dev/null 2>&1
+				$TEST_SHELL_PATH "$0" "$@" >/dev/null 2>&1 & test_pid=$!
+				if wait
 				then
 					printf >&2 "OK   %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
 				elif test -f "$stressfail" &&
@@ -124,16 +128,11 @@ done,*)
 				fi
 				cnt=$(($cnt + 1))
 			done
-		) &
+		) & job_pids="$job_pids $!"
 		job_nr=$(($job_nr + 1))
 	done
 
-	job_nr=0
-	while test $job_nr -lt "$job_count"
-	do
-		wait
-		job_nr=$(($job_nr + 1))
-	done
+	wait
 
 	if test -f "$stressfail" && test "$(cat "$stressfail")" != "aborted"
 	then

> > > +				elif test -f "$stressfail" &&
> > > +				     test "$(cat "$stressfail")" = "aborted"
> > > +				then
> > > +					printf >&2 "ABORTED %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
> > > +				else
> > > +					printf >&2 "FAIL %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
> > > +					echo $GIT_TEST_STRESS_JOB_NR >>"$stressfail"
> > > +				fi
> > 
> > Hmm. What happens if we see a failure _and_ there's an abort? That's
> > actually pretty plausible if you see a failure whiz by, and you want to
> > abort the remaining scripts because they take a long time to run.
> > 
> > If the abort happens first, then the goal is to assume any errors are
> > due to the abort.
> 
> Yeah, that's why I added this ABORTED case.  When I hit ctrl-C, a
> couple of the tests tend to fail, but that's not the rare failure we
> are hoping to find when stress testing, so printing FAIL would be
> misleading.  The test script exits with 1 all the same, so we can't
> tell the difference, but since rare failures are, well, rare, this
> failure is most likely due to the abort.

With the patch as posted I actually see most of them still say "OK",
because the SIGINT does not make it to them (it would be a different
story if I killed the whole process group).

But with the modification above, I get "ABORTED" for most, which is what
I'd want. And it should be pretty race-free, since the "aborted" file is
put into place before triggering the signal cascade.

> > If the fail happens first (which I think is the more likely case), then
> > the abort handler will overwrite the file with "aborted", and we'll
> > forget that there was a real failure. This works in my script because of
> > the symlinking (if a real failure symlinked $fail to a directory, then
> > the attempt to write "aborted" will just be a noop).
> 
> OK, I think the abort handler should append, not overwrite, and then
> the file's contents should be checked e.g. with a case statement
> looking for *aborted*.
> 
> Or use two separate files for signaling abort and test failure.

Two separate files seems a lot simpler to me.

> > In my experience, outputting the failed log saves a step (especially
> > with "-i"), since seeing the failed test and its output is often
> > sufficient.
> 
> I just don't like when test output, especially with '-x', is dumped on
> my terminal :)

Don't you then use your terminal to display the file? ;) (I'm kidding, I
assume you load it in "less" or whatever, which is distinct).

I wish this could be optional somehow, though. I really prefer the dump.
I guess if I continue to use my script as a wrapper, it can dump for me
(after digging in the $stressfail file, I guess).

> > I'm also sad to lose the symlink to the failed trash directory, which
> > saves cutting and pasting when you have to inspect it. But I guess we
> > can't rely on symlinks everywhere.
> 
> You can tab-complete most of the trash directory, and then there are
> only those 1-2-3 digits of the job number that you have to type.  That
> worked out well enough for me so far (but I've only used it for a few
> days, so...).
> 
> We could rename the trash directory of the failed test to something
> short, sweet, and common, but that could be racy in the unlikely case
> that two tests fail at the same time.  And I like the 'trash
> directory.' prefix, because then 'make clean' will delete that one,
> too, without modifying 't/Makefile' (which is also the reason for
> adding the 'stress-N' suffix instead of a prefix, and putting the fail
> file into the 'test-results' directory).

We could rename it to "trash directory.xxx.failed", which is at least
predictable. That can even be done atomically, though I think it's a
little tricky to do portably with shell tools. I guess I can continue to
do the symlink thing in my wrapper script.

-Peff
Previous: SZEDER GáborNext: SZEDER Gábor
Message 27 of 67 in “test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests”
  1. 0/3 test-lib: add the '--stress' option to help reproduce occasional failures in flaky testsSZEDER Gábor, Dec 4, 2018
  2. 1/3 test-lib: consolidate naming of test-results pathsSZEDER Gábor, Dec 4, 2018
  3. Jeff KingDec 5, 2018
  4. 2/3 test-lib-functions: introduce the 'test_set_port' helper functionSZEDER Gábor, Dec 4, 2018
  5. Jeff KingDec 5, 2018
  6. SZEDER GáborDec 5, 2018
  7. Jeff KingDec 5, 2018
  8. 3/3 test-lib: add the '--stress' option to run a test repeatedly under loadSZEDER Gábor, Dec 4, 2018
  9. Ævar Arnfjörð BjarmasonDec 4, 2018
  10. SZEDER GáborDec 4, 2018
  11. Jeff KingDec 5, 2018
  12. Ævar Arnfjörð BjarmasonDec 4, 2018
  13. Jeff KingDec 5, 2018
  14. SZEDER GáborDec 5, 2018
  15. Ævar Arnfjörð BjarmasonDec 5, 2018
  16. SZEDER GáborDec 5, 2018
  17. Ævar Arnfjörð BjarmasonDec 5, 2018
  18. Jeff KingDec 5, 2018
  19. SZEDER GáborDec 5, 2018
  20. Jeff KingDec 5, 2018
  21. Junio C HamanoDec 6, 2018
  22. Jeff KingDec 6, 2018
  23. Junio C HamanoDec 6, 2018
  24. SZEDER GáborDec 6, 2018
  25. Jeff KingDec 7, 2018
  26. SZEDER GáborDec 5, 2018
  27. Jeff KingDec 5, 2018
  28. SZEDER GáborDec 6, 2018
  29. Jeff KingDec 7, 2018
  30. 0/7 test-lib: add the '--stress' option to help reproduce occasional failures in flaky testsSZEDER Gábor, Dec 9, 2018
  31. 2/7 test-lib: parse some --options earlierSZEDER Gábor, Dec 9, 2018
  32. Jeff KingDec 11, 2018
  33. SZEDER GáborDec 11, 2018
  34. Jeff KingDec 17, 2018
  35. SZEDER GáborDec 30, 2018
  36. Jeff KingJan 3, 2019
  37. 1/7 test-lib: translate SIGTERM and SIGHUP to an exitSZEDER Gábor, Dec 9, 2018
  38. Jeff KingDec 11, 2018
  39. 4/7 test-lib: set $TRASH_DIRECTORY earlierSZEDER Gábor, Dec 9, 2018
  40. 5/7 test-lib: extract Bash version check for '-x' tracingSZEDER Gábor, Dec 9, 2018
  41. 3/7 test-lib: consolidate naming of test-results pathsSZEDER Gábor, Dec 9, 2018
  42. 6/7 test-lib-functions: introduce the 'test_set_port' helper functionSZEDER Gábor, Dec 9, 2018
  43. 7/7 test-lib: add the '--stress' option to run a test repeatedly under loadSZEDER Gábor, Dec 9, 2018
  44. fixup! test-lib: add the '--stress' option to run a test repeatedly under loadSZEDER Gábor, Dec 10, 2018
  45. Jeff KingDec 11, 2018
  46. 0/8 test-lib: add the '--stress' option to help reproduce occasional failures in flaky testsSZEDER Gábor, Dec 30, 2018
  47. 1/8 test-lib: translate SIGTERM and SIGHUP to an exitSZEDER Gábor, Dec 30, 2018
  48. 2/8 test-lib: parse options in a for loop to keep $@ intactSZEDER Gábor, Dec 30, 2018
  49. 3/8 test-lib: parse command line options earlierSZEDER Gábor, Dec 30, 2018
  50. 6/8 test-lib: extract Bash version check for '-x' tracingSZEDER Gábor, Dec 30, 2018
  51. Carlo ArenasDec 31, 2018
  52. 7/8 test-lib-functions: introduce the 'test_set_port' helper functionSZEDER Gábor, Dec 30, 2018
  53. 5/8 test-lib: set $TRASH_DIRECTORY earlierSZEDER Gábor, Dec 30, 2018
  54. SZEDER GáborDec 30, 2018
  55. 5/8 test-lib: set $TRASH_DIRECTORY earlierSZEDER Gábor, Dec 30, 2018
  56. 4/8 test-lib: consolidate naming of test-results pathsSZEDER Gábor, Dec 30, 2018
  57. 8/8 test-lib: add the '--stress' option to run a test repeatedly under loadSZEDER Gábor, Dec 30, 2018
  58. 0/8 test-lib: add the '--stress' option to help reproduce occasional failures in flaky testsSZEDER Gábor, Jan 5, 2019
  59. 1/8 test-lib: translate SIGTERM and SIGHUP to an exitSZEDER Gábor, Jan 5, 2019
  60. 2/8 test-lib: extract Bash version check for '-x' tracingSZEDER Gábor, Jan 5, 2019
  61. 3/8 test-lib: parse options in a for loop to keep $@ intactSZEDER Gábor, Jan 5, 2019
  62. 4/8 test-lib: parse command line options earlierSZEDER Gábor, Jan 5, 2019
  63. 5/8 test-lib: consolidate naming of test-results pathsSZEDER Gábor, Jan 5, 2019
  64. 6/8 test-lib: set $TRASH_DIRECTORY earlierSZEDER Gábor, Jan 5, 2019
  65. 8/8 test-lib: add the '--stress' option to run a test repeatedly under loadSZEDER Gábor, Jan 5, 2019
  66. 7/8 test-lib-functions: introduce the 'test_set_port' helper functionSZEDER Gábor, Jan 5, 2019
  67. Jeff KingJan 7, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.