tests: some touchups related to the --stress feature

If my mistake using --stress= instead of --stress-limit= is any indication, then the current options are very confusing.

This is my attempt at making them less confusing.

Johannes Schindelin (2):
tests: let --stress-limit=<N> imply --stress
tests: introduce --stress-jobs=<N>

 t/test-lib.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

base-commit: 7d661e5ed16dca303d7898f5ab0cc2ffc69e0499
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-155%2Fdscho%2Fstress-test-extra-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-155/dscho/stress-test-extra-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/155

gitgitgadget

From: Johannes Schindelin <[email protected]>

The --stress option currently accepts an argument, but it is confusing to at least this user that the argument does not define the maximal number of stress iterations, but instead the number of jobs to run in parallel per stress iteration.

Let's introduce a separate option for that, whose name makes it more obvious what it is about, and let --stress=<N> error out with a helpful suggestion about the two options tha could possibly have been meant.

---
 t/test-lib.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index ab7f27ec6a..6e557982a2 100644
@@ -142,10 +142,16 @@ do
 	--stress)
 		stress=t ;;
 	--stress=*)
+		echo "error: --stress does not accept an argument: '$opt'" >&2
+		echo "did you mean --stress-jobs=${opt#*=} or --stress-limit=${opt#*=}?" >&2
+		exit 1
+		;;
+	--stress-jobs=*)
+		stress=t;
 		stress=${opt#--*=}
 		case "$stress" in
 		*[!0-9]*|0*|"")
-			echo "error: --stress=<N> requires the number of jobs to run" >&2
+			echo "error: --stress-jobs=<N> requires the number of jobs to run" >&2
 			exit 1
 			;;
 		*)	# Good.

gitgitgadget

From: Johannes Schindelin <[email protected]>

It does not make much sense that running a test with --stress-limit=<N> seemingly ignores that option because it does not stress test at all.

---
 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

index 4e7cb52b57..ab7f27ec6a 100644
@@ -153,6 +153,7 @@ do
 		esac
 		;;
 	--stress-limit=*)
+		stress=t;
 		stress_limit=${opt#--*=}
 		case "$stress_limit" in
 		*[!0-9]*|0*|"")

gitgitgadget

This new option probably deserves documentation in t/README alongside existing documentation for --stress and --stress-limit, and the existing --stress= documentation ought be updated.

"Johannes Schindelin via GitGitGadget" <[email protected]>

"Johannes Schindelin via GitGitGadget" <[email protected]>

Yeah, when there are multiple knobs that can take integral value, and especially when the knobs are of equal usefulness, the users would happen pick the right one 50% of the time by accident, which is not a happy state.

Good.

Making --stress=<value> error out (instead of deprecating with a transition plan) is probably OK given its audience. It is a good trade-off to save our braincycles by not having to worry about transition and forcing everybody to adjust (I am assuming that nobody has a wrapper script to run tXXXX-title.sh scripts that passes the --stress=<value> thing).

t/README must be updated as --stress=<N> is documented to specify the degree of parallelism, though.

I'll queue in the meantime, to reduce the risk of forgetting the topic.

Thanks.

Well, these options' description in 't/README' is quite clear on what they do. If the lack of updates to 't/README' in these patches is any indication, then you haven't read that :) but doing so might very well have avoided your confusion in the first place.

According to my Bash history, I used '--stress=<even-more-cpu-cores>' about 20x more often than '--stress-limit=<N>'. That's not surprising at all, since the main point is to try to trigger rare, hard to reproduce failures, no matter how many repetitions it takes. And even if there is an upper bound, it is usually not the number of repetitions I know in advance, but rather the time I'm willing to sacrifice on it, e.g. how long I'm on lunch break or doing groceries, or when I need my CPUs for more important things, or simply when I give up, and hit ctrl-C.

And with --stress-jobs=<N> I will have to type more :)

If my mistake using --stress= instead of --stress-limit= is any indication, then the current options are very confusing.

This is my attempt at making them less confusing.

Changes since v1:

* Now the patches actually adjust the documentation according to the changes ;-)

Johannes Schindelin (2):
tests: let --stress-limit=<N> imply --stress
tests: introduce --stress-jobs=<N>

 t/README      | 8 +++++---
 t/test-lib.sh | 9 ++++++++-
 2 files changed, 13 insertions(+), 4 deletions(-)

base-commit: 7d661e5ed16dca303d7898f5ab0cc2ffc69e0499
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-155%2Fdscho%2Fstress-test-extra-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-155/dscho/stress-test-extra-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/155

Range-diff vs v1:

1: 16c6508c4b ! 1: fbe773c22a tests: let --stress-limit=<N> imply --stress
@@ -8,6 +8,19 @@

+ diff --git a/t/README b/t/README
+ --- a/t/README
+ +++ b/t/README
+@@
+ --stress-limit=<N>::
+ When combined with --stress run the test script repeatedly
+ this many times in each of the parallel jobs or until one of
+- them fails, whichever comes first.
++ them fails, whichever comes first. Implies --stress.
+
+ You can also set the GIT_TEST_INSTALLED environment variable to
+ the bindir of an existing git installation to test that installation. +
diff --git a/t/test-lib.sh b/t/test-lib.sh
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
2: 281d3f1d19 ! 2: 074628c22b tests: introduce --stress-jobs=<N>
@@ -13,6 +13,33 @@

+ diff --git a/t/README b/t/README
+ --- a/t/README
+ +++ b/t/README
+@@
+ variable to "1" or "0", respectively.
+
+ --stress::
+---stress=<N>::
+ Run the test script repeatedly in multiple parallel jobs until
+ one of them fails. Useful for reproducing rare failures in
+ flaky tests. The number of parallel jobs is, in order of
+- precedence: <N>, or the value of the GIT_TEST_STRESS_LOAD
++ precedence: the value of the GIT_TEST_STRESS_LOAD
+ environment variable, or twice the number of available
+ processors (as shown by the 'getconf' utility), or 8.
+ Implies --verbose -x --immediate to get the most information
+@@
+ '.stress-<nr>' suffix, and the trash directory of the failed
+ test job is renamed to end with a '.stress-failed' suffix.
+
++--stress-jobs=<N>::
++ Override the number of parallel jobs. Implies --stress.
++
+ --stress-limit=<N>::
+ When combined with --stress run the test script repeatedly
+ this many times in each of the parallel jobs or until one of
+
diff --git a/t/test-lib.sh b/t/test-lib.sh
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

gitgitgadget

From: Johannes Schindelin <[email protected]>

It does not make much sense that running a test with --stress-limit=<N> seemingly ignores that option because it does not stress test at all.

---
 t/README      | 2 +-
 t/test-lib.sh | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

index 3aed321248..b61bc930c4 100644
@@ -205,7 +205,7 @@ appropriately before running "make".
 --stress-limit=<N>::
 	When combined with --stress run the test script repeatedly
 	this many times in each of the parallel jobs or until one of
-	them fails, whichever comes first.
+	them fails, whichever comes first. Implies `--stress`.
 
 You can also set the GIT_TEST_INSTALLED environment variable to
 the bindir of an existing git installation to test that installation.

index 4e7cb52b57..ab7f27ec6a 100644
@@ -153,6 +153,7 @@ do
 		esac
 		;;
 	--stress-limit=*)
+		stress=t;
 		stress_limit=${opt#--*=}
 		case "$stress_limit" in
 		*[!0-9]*|0*|"")

gitgitgadget

From: Johannes Schindelin <[email protected]>

The --stress option currently accepts an argument, but it is confusing to at least this user that the argument does not define the maximal number of stress iterations, but instead the number of jobs to run in parallel per stress iteration.

Let's introduce a separate option for that, whose name makes it more obvious what it is about, and let --stress=<N> error out with a helpful suggestion about the two options tha could possibly have been meant.

---
 t/README      | 6 ++++--
 t/test-lib.sh | 8 +++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

index b61bc930c4..a496be56ef 100644
@@ -187,11 +187,10 @@ appropriately before running "make".
 	variable to "1" or "0", respectively.
 
 --stress::
---stress=<N>::
 	Run the test script repeatedly in multiple parallel jobs until
 	one of them fails.  Useful for reproducing rare failures in
 	flaky tests.  The number of parallel jobs is, in order of
-	precedence: <N>, or the value of the GIT_TEST_STRESS_LOAD
+	precedence: the value of the GIT_TEST_STRESS_LOAD
 	environment variable, or twice the number of available
 	processors (as shown by the 'getconf' utility),	or 8.
 	Implies `--verbose -x --immediate` to get the most information

@@ -202,6 +201,9 @@ appropriately before running "make".
 	'.stress-<nr>' suffix, and the trash directory of the failed
 	test job is renamed to end with a '.stress-failed' suffix.
 
+--stress-jobs=<N>::
+	Override the number of parallel jobs. Implies `--stress`.
+
 --stress-limit=<N>::
 	When combined with --stress run the test script repeatedly
 	this many times in each of the parallel jobs or until one of

index ab7f27ec6a..6e557982a2 100644
@@ -142,10 +142,16 @@ do
 	--stress)
 		stress=t ;;
 	--stress=*)
+		echo "error: --stress does not accept an argument: '$opt'" >&2
+		echo "did you mean --stress-jobs=${opt#*=} or --stress-limit=${opt#*=}?" >&2
+		exit 1
+		;;
+	--stress-jobs=*)
+		stress=t;
 		stress=${opt#--*=}
 		case "$stress" in
 		*[!0-9]*|0*|"")
-			echo "error: --stress=<N> requires the number of jobs to run" >&2
+			echo "error: --stress-jobs=<N> requires the number of jobs to run" >&2
 			exit 1
 			;;
 		*)	# Good.

gitgitgadget

Hi,

Yep, hadn't read it, assumed that it was obvious that --stress=<N>
meant: try for at most <N> times to replicate the issue.

Which to me is a good indicator that the UI could be improved...

I don't doubt that *you* need --stress-jobs=<N> more often than --stress-limit=<N>, but I really think that common users will need the latter a lot more often (if they need the former at all).

Sorry ;-)

Ciao,
Dscho

Eyes can easily be tricked by the patch format, but the above hunk does remove that line ;-) I had the same reaction when I saw your message for the first time (before seeing the patch itself).

Yep, my eyes were tricked. Thanks for pointing it out, and sorry for the noise.