[PATCH 0/2] tests: some touchups related to the --stress feature

[PATCH 0/2] tests: some touchups related to the --stress feature

[Johannes Schindelin via GitGitGadget]

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

[PATCH 1/2] tests: let --stress-limit=<N> imply --stress

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

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

[PATCH 2/2] tests: introduce --stress-jobs=<N>

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index ab7f27ec6a..6e557982a2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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

Re: [PATCH 2/2] tests: introduce --stress-jobs=<N>

[Eric Sunshine]

On Sat, Mar 2, 2019 at 4:19 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.

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.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Re: [PATCH 1/2] tests: let --stress-limit=<N> imply --stress

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/test-lib.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 4e7cb52b57..ab7f27ec6a 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -153,6 +153,7 @@ do
>  		esac
>  		;;
>  	--stress-limit=*)
> +		stress=t;
>  		stress_limit=${opt#--*=}

Good thinking.

>  		case "$stress_limit" in
>  		*[!0-9]*|0*|"")

Re: [PATCH 2/2] tests: introduce --stress-jobs=<N>

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

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.

> 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.

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.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/test-lib.sh | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index ab7f27ec6a..6e557982a2 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -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.

Re: [PATCH 2/2] tests: introduce --stress-jobs=<N>

[SZEDER Gábor]

On Sat, Mar 02, 2019 at 01:19:44PM -0800, Johannes Schindelin via GitGitGadget wrote:
> 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.

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 :)


> 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.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/test-lib.sh | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index ab7f27ec6a..6e557982a2 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -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

[PATCH v2 0/2] tests: some touchups related to the --stress feature

[Johannes Schindelin via GitGitGadget]

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 @@
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     + 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 @@
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     + 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

[PATCH v2 1/2] tests: let --stress-limit=<N> imply --stress

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/README      | 2 +-
 t/test-lib.sh | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 3aed321248..b61bc930c4 100644
--- a/t/README
+++ b/t/README
@@ -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.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4e7cb52b57..ab7f27ec6a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -153,6 +153,7 @@ do
 		esac
 		;;
 	--stress-limit=*)
+		stress=t;
 		stress_limit=${opt#--*=}
 		case "$stress_limit" in
 		*[!0-9]*|0*|"")
-- 
gitgitgadget

[PATCH v2 2/2] tests: introduce --stress-jobs=<N>

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/README      | 6 ++++--
 t/test-lib.sh | 8 +++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/t/README b/t/README
index b61bc930c4..a496be56ef 100644
--- a/t/README
+++ b/t/README
@@ -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
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ab7f27ec6a..6e557982a2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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

Re: [PATCH 2/2] tests: introduce --stress-jobs=<N>

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1717 bytes --]

Hi,

On Sun, 3 Mar 2019, SZEDER Gábor wrote:

> On Sat, Mar 02, 2019 at 01:19:44PM -0800, Johannes Schindelin via GitGitGadget wrote:
> > 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.
> 
> 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.

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...

> 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.

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).

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

Sorry ;-)

Ciao,
Dscho

Re: [PATCH v2 2/2] tests: introduce --stress-jobs=<N>

[Eric Sunshine]

On Sun, Mar 3, 2019 at 9:45 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> [...]
> 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.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/t/README b/t/README
> @@ -187,11 +187,10 @@ appropriately before running "make".
>         variable to "1" or "0", respectively.
>
>  --stress::
> ---stress=<N>::

Shouldn't the "--stress=<N>" line be removed?

>         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.

Re: [PATCH v2 2/2] tests: introduce --stress-jobs=<N>

[Jeff King]

On Sun, Mar 03, 2019 at 06:44:55AM -0800, Johannes Schindelin via GitGitGadget wrote:

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index ab7f27ec6a..6e557982a2 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -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

This seems reasonable. I was all set to argue that "--stress-limit" is
much more common than "--stress-jobs", but I see Gábor just made the
opposite argument. So maybe just informing the user is the right thing. :)

-Peff

Re: [PATCH v2 2/2] tests: introduce --stress-jobs=<N>

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sun, Mar 3, 2019 at 9:45 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> [...]
>> 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.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> diff --git a/t/README b/t/README
>> @@ -187,11 +187,10 @@ appropriately before running "make".
>>         variable to "1" or "0", respectively.
>>
>>  --stress::
>> ---stress=<N>::
>
> Shouldn't the "--stress=<N>" line be removed?

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).

>
>>         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.

Re: [PATCH v2 2/2] tests: introduce --stress-jobs=<N>

[Eric Sunshine]

On Sun, Mar 3, 2019 at 10:22 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On Sun, Mar 3, 2019 at 9:45 AM Johannes Schindelin via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> ---stress=<N>::
> >
> > Shouldn't the "--stress=<N>" line be removed?
>
> 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.