On Wed, Dec 05, 2018 at 03:01:06PM +0100, SZEDER Gábor wrote: > > > - 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). > > > '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
