[PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY

[PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY

[Patrick Steinhardt]

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

When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be
written in that directory instead of the default directory located in
"t/". While this works as expected for our normal tests, performance
tests fail to locate and aggregate performance data because they don't
know to handle TEST_OUTPUT_DIRECTORY correctly and always look at the
default location.

Fix the issue by adding a `--results-dir` parameter to "aggregate.perl"
which identifies the directory where results are and by making the "run"
script awake of the TEST_OUTPUT_DIRECTORY variable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/perf/aggregate.perl |  8 ++++++--
 t/perf/perf-lib.sh    |  4 ++--
 t/perf/run            | 25 ++++++++++++++++---------
 3 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index 14e4cda287..5d4964c5c6 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -58,6 +58,7 @@ sub usage {
   Options:
     --codespeed          * Format output for Codespeed
     --reponame    <str>  * Send given reponame to codespeed
+    --results-dir <str>  * Directory where test results are located
     --sort-by     <str>  * Sort output (only "regression" criteria is supported)
     --subsection  <str>  * Use results from given subsection
 
@@ -90,12 +91,13 @@ sub sane_backticks {
 }
 
 my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
-    $codespeed, $sortby, $subsection, $reponame);
+    $codespeed, $sortby, $subsection, $reponame, $resultsdir);
 
 Getopt::Long::Configure qw/ require_order /;
 
 my $rc = GetOptions("codespeed"     => \$codespeed,
 		    "reponame=s"    => \$reponame,
+		    "results-dir=s" => \$resultsdir,
 		    "sort-by=s"     => \$sortby,
 		    "subsection=s"  => \$subsection);
 usage() unless $rc;
@@ -137,7 +139,9 @@ sub sane_backticks {
 	@tests = glob "p????-*.sh";
 }
 
-my $resultsdir = "test-results";
+if (not $resultsdir) {
+	$resultsdir = "test-results";
+}
 
 if (! $subsection and
     exists $ENV{GIT_PERF_SUBSECTION} and
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 601d9f67dd..8ca6dd0297 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP
 MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git
 export MODERN_GIT
 
-perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results
+perf_results_dir=$TEST_RESULTS_DIR
 test -n "$GIT_PERF_SUBSECTION" && perf_results_dir="$perf_results_dir/$GIT_PERF_SUBSECTION"
 mkdir -p "$perf_results_dir"
 rm -f "$perf_results_dir"/$(basename "$0" .sh).subtests
@@ -253,7 +253,7 @@ test_size () {
 # and does it after running everything)
 test_at_end_hook_ () {
 	if test -z "$GIT_PERF_AGGREGATING_LATER"; then
-		( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl $(basename "$0") )
+		( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $(basename "$0") )
 	fi
 }
 
diff --git a/t/perf/run b/t/perf/run
index c7b86104e1..03128d440a 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -188,10 +188,10 @@ run_subsection () {
 
 	if test -z "$GIT_PERF_SEND_TO_CODESPEED"
 	then
-		./aggregate.perl $codespeed_opt "$@"
+		./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $codespeed_opt "$@"
 	else
-		json_res_file="test-results/$GIT_PERF_SUBSECTION/aggregate.json"
-		./aggregate.perl --codespeed "$@" | tee "$json_res_file"
+		json_res_file=""$TEST_RESULTS_DIR"/$GIT_PERF_SUBSECTION/aggregate.json"
+		./aggregate.perl --results-dir="$TEST_RESULTS_DIR" --codespeed "$@" | tee "$json_res_file"
 		send_data_url="$GIT_PERF_SEND_TO_CODESPEED/result/add/json/"
 		curl -v --request POST --data-urlencode "json=$(cat "$json_res_file")" "$send_data_url"
 	fi
@@ -203,10 +203,17 @@ get_var_from_env_or_config "GIT_PERF_SEND_TO_CODESPEED" "perf" "sendToCodespeed"
 cd "$(dirname $0)"
 . ../../GIT-BUILD-OPTIONS
 
-mkdir -p test-results
-get_subsections "perf" >test-results/run_subsections.names
+if test -n "$TEST_OUTPUT_DIRECTORY"
+then
+    TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
+else
+    TEST_RESULTS_DIR=test-results
+fi
 
-if test $(wc -l <test-results/run_subsections.names) -eq 0
+mkdir -p "$TEST_RESULTS_DIR"
+get_subsections "perf" >"$TEST_RESULTS_DIR"/run_subsections.names
+
+if test $(wc -l <"$TEST_RESULTS_DIR"/run_subsections.names) -eq 0
 then
 	if test -n "$GIT_PERF_SUBSECTION"
 	then
@@ -222,10 +229,10 @@ then
 	)
 elif test -n "$GIT_PERF_SUBSECTION"
 then
-	egrep "^$GIT_PERF_SUBSECTION\$" test-results/run_subsections.names >/dev/null ||
+	egrep "^$GIT_PERF_SUBSECTION\$" "$TEST_RESULTS_DIR"/run_subsections.names >/dev/null ||
 		die "subsection '$GIT_PERF_SUBSECTION' not found in '$GIT_PERF_CONFIG_FILE'"
 
-	egrep "^$GIT_PERF_SUBSECTION\$" test-results/run_subsections.names | while read -r subsec
+	egrep "^$GIT_PERF_SUBSECTION\$" "$TEST_RESULTS_DIR"/run_subsections.names | while read -r subsec
 	do
 		(
 			GIT_PERF_SUBSECTION="$subsec"
@@ -243,5 +250,5 @@ else
 			echo "======== Run for subsection '$GIT_PERF_SUBSECTION' ========"
 			run_subsection "$@"
 		)
-	done <test-results/run_subsections.names
+	done <"$TEST_RESULTS_DIR"/run_subsections.names
 fi
-- 
2.32.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 16 2021, Patrick Steinhardt wrote:

> When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be
> written in that directory instead of the default directory located in

Is the timing of this patch a coincidence, or did you run into this
related to the other patches related to this variable now,
i.e. https://lore.kernel.org/git/20210609170520.67014-1-felipe.contreras@gmail.com/
and related.

> Fix the issue by adding a `--results-dir` parameter to "aggregate.perl"
> which identifies the directory where results are and by making the "run"
> script awake of the TEST_OUTPUT_DIRECTORY variable.

Makes sense.

> [...]
>  my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
> -    $codespeed, $sortby, $subsection, $reponame);
> +    $codespeed, $sortby, $subsection, $reponame, $resultsdir);
>  
>  Getopt::Long::Configure qw/ require_order /;
>  
>  my $rc = GetOptions("codespeed"     => \$codespeed,
>  		    "reponame=s"    => \$reponame,
> +		    "results-dir=s" => \$resultsdir,
>  		    "sort-by=s"     => \$sortby,
>  		    "subsection=s"  => \$subsection);
>  usage() unless $rc;
> @@ -137,7 +139,9 @@ sub sane_backticks {
>  	@tests = glob "p????-*.sh";
>  }
>  
> -my $resultsdir = "test-results";
> +if (not $resultsdir) {
> +	$resultsdir = "test-results";
> +}

Works, but FWIW in git.git's perl scripts it's usual to do:

my $resultsdir = "test-results";
GetOptions(...);

Which serves the same purpose. You can also inline this in the GetOptions call:
	
	GetOptions(...,
		"results-dir=s" => \(my $resultsdir = "test-results"),
	);

But maybe that's too obscurely Perl-ish, i.e. declaring a variable for
our scope inline in another function's argument list, and we should just
use the first idiom we use in most other places.


>  	if test -z "$GIT_PERF_AGGREGATING_LATER"; then
> -		( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl $(basename "$0") )
> +		( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $(basename "$0") )
>  	fi
>  }

Makes sense to split up the overly long line at this point probably...

>[...]
> -mkdir -p test-results
> -get_subsections "perf" >test-results/run_subsections.names
> +if test -n "$TEST_OUTPUT_DIRECTORY"
> +then
> +    TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
> +else
> +    TEST_RESULTS_DIR=test-results
> +fi

Indending with spaces?

Re: [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY

[Patrick Steinhardt]

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

On Wed, Jun 16, 2021 at 01:33:32PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jun 16 2021, Patrick Steinhardt wrote:
> 
> > When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be
> > written in that directory instead of the default directory located in
> 
> Is the timing of this patch a coincidence, or did you run into this
> related to the other patches related to this variable now,
> i.e. https://lore.kernel.org/git/20210609170520.67014-1-felipe.contreras@gmail.com/
> and related.

Coincidence. In fact, I already sent this patch a few weeks ago as part
of [1]. But given that this patch set turned out to not be an easy sell
and that I didn't yet find the time to work on it again, I decided to
split out this patch and upstream it standalone.

Your remarks all make sense to me, will address them in v2. Thanks!

Patrick

[1]: http://public-inbox.org/git/cover.1621451532.git.ps@pks.im/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY

[Patrick Steinhardt]

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

When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be
written in that directory instead of the default directory located in
"t/". While this works as expected for our normal tests, performance
tests fail to locate and aggregate performance data because they don't
know to handle TEST_OUTPUT_DIRECTORY correctly and always look at the
default location.

Fix the issue by adding a `--results-dir` parameter to "aggregate.perl"
which identifies the directory where results are and by making the "run"
script awake of the TEST_OUTPUT_DIRECTORY variable.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Range-diff against v1:
1:  8f73bd5d38 ! 1:  cb9d948646 perf: fix when running with TEST_OUTPUT_DIRECTORY
    @@ t/perf/aggregate.perl: sub usage {
          --subsection  <str>  * Use results from given subsection
      
     @@ t/perf/aggregate.perl: sub sane_backticks {
    - }
      
      my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
    --    $codespeed, $sortby, $subsection, $reponame);
    -+    $codespeed, $sortby, $subsection, $reponame, $resultsdir);
    +     $codespeed, $sortby, $subsection, $reponame);
    ++my $resultsdir = "test-results";
      
      Getopt::Long::Configure qw/ require_order /;
      
    @@ t/perf/aggregate.perl: sub sane_backticks {
      }
      
     -my $resultsdir = "test-results";
    -+if (not $resultsdir) {
    -+	$resultsdir = "test-results";
    -+}
    - 
    +-
      if (! $subsection and
          exists $ENV{GIT_PERF_SUBSECTION} and
    +     $ENV{GIT_PERF_SUBSECTION} ne "") {
     
      ## t/perf/perf-lib.sh ##
     @@ t/perf/perf-lib.sh: export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP
    @@ t/perf/perf-lib.sh: test_size () {
      test_at_end_hook_ () {
      	if test -z "$GIT_PERF_AGGREGATING_LATER"; then
     -		( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl $(basename "$0") )
    -+		( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $(basename "$0") )
    ++		(
    ++			cd "$TEST_DIRECTORY"/perf &&
    ++			./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $(basename "$0")
    ++		)
      	fi
      }
      
    @@ t/perf/run: get_var_from_env_or_config "GIT_PERF_SEND_TO_CODESPEED" "perf" "send
     -get_subsections "perf" >test-results/run_subsections.names
     +if test -n "$TEST_OUTPUT_DIRECTORY"
     +then
    -+    TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
    ++	TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
     +else
    -+    TEST_RESULTS_DIR=test-results
    ++	TEST_RESULTS_DIR=test-results
     +fi
      
     -if test $(wc -l <test-results/run_subsections.names) -eq 0

 t/perf/aggregate.perl |  5 +++--
 t/perf/perf-lib.sh    |  7 +++++--
 t/perf/run            | 25 ++++++++++++++++---------
 3 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index 14e4cda287..82c0df4553 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -58,6 +58,7 @@ sub usage {
   Options:
     --codespeed          * Format output for Codespeed
     --reponame    <str>  * Send given reponame to codespeed
+    --results-dir <str>  * Directory where test results are located
     --sort-by     <str>  * Sort output (only "regression" criteria is supported)
     --subsection  <str>  * Use results from given subsection
 
@@ -91,11 +92,13 @@ sub sane_backticks {
 
 my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
     $codespeed, $sortby, $subsection, $reponame);
+my $resultsdir = "test-results";
 
 Getopt::Long::Configure qw/ require_order /;
 
 my $rc = GetOptions("codespeed"     => \$codespeed,
 		    "reponame=s"    => \$reponame,
+		    "results-dir=s" => \$resultsdir,
 		    "sort-by=s"     => \$sortby,
 		    "subsection=s"  => \$subsection);
 usage() unless $rc;
@@ -137,8 +140,6 @@ sub sane_backticks {
 	@tests = glob "p????-*.sh";
 }
 
-my $resultsdir = "test-results";
-
 if (! $subsection and
     exists $ENV{GIT_PERF_SUBSECTION} and
     $ENV{GIT_PERF_SUBSECTION} ne "") {
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 601d9f67dd..f5ed092ee5 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP
 MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git
 export MODERN_GIT
 
-perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results
+perf_results_dir=$TEST_RESULTS_DIR
 test -n "$GIT_PERF_SUBSECTION" && perf_results_dir="$perf_results_dir/$GIT_PERF_SUBSECTION"
 mkdir -p "$perf_results_dir"
 rm -f "$perf_results_dir"/$(basename "$0" .sh).subtests
@@ -253,7 +253,10 @@ test_size () {
 # and does it after running everything)
 test_at_end_hook_ () {
 	if test -z "$GIT_PERF_AGGREGATING_LATER"; then
-		( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl $(basename "$0") )
+		(
+			cd "$TEST_DIRECTORY"/perf &&
+			./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $(basename "$0")
+		)
 	fi
 }
 
diff --git a/t/perf/run b/t/perf/run
index c7b86104e1..d19dec258a 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -188,10 +188,10 @@ run_subsection () {
 
 	if test -z "$GIT_PERF_SEND_TO_CODESPEED"
 	then
-		./aggregate.perl $codespeed_opt "$@"
+		./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $codespeed_opt "$@"
 	else
-		json_res_file="test-results/$GIT_PERF_SUBSECTION/aggregate.json"
-		./aggregate.perl --codespeed "$@" | tee "$json_res_file"
+		json_res_file=""$TEST_RESULTS_DIR"/$GIT_PERF_SUBSECTION/aggregate.json"
+		./aggregate.perl --results-dir="$TEST_RESULTS_DIR" --codespeed "$@" | tee "$json_res_file"
 		send_data_url="$GIT_PERF_SEND_TO_CODESPEED/result/add/json/"
 		curl -v --request POST --data-urlencode "json=$(cat "$json_res_file")" "$send_data_url"
 	fi
@@ -203,10 +203,17 @@ get_var_from_env_or_config "GIT_PERF_SEND_TO_CODESPEED" "perf" "sendToCodespeed"
 cd "$(dirname $0)"
 . ../../GIT-BUILD-OPTIONS
 
-mkdir -p test-results
-get_subsections "perf" >test-results/run_subsections.names
+if test -n "$TEST_OUTPUT_DIRECTORY"
+then
+	TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
+else
+	TEST_RESULTS_DIR=test-results
+fi
 
-if test $(wc -l <test-results/run_subsections.names) -eq 0
+mkdir -p "$TEST_RESULTS_DIR"
+get_subsections "perf" >"$TEST_RESULTS_DIR"/run_subsections.names
+
+if test $(wc -l <"$TEST_RESULTS_DIR"/run_subsections.names) -eq 0
 then
 	if test -n "$GIT_PERF_SUBSECTION"
 	then
@@ -222,10 +229,10 @@ then
 	)
 elif test -n "$GIT_PERF_SUBSECTION"
 then
-	egrep "^$GIT_PERF_SUBSECTION\$" test-results/run_subsections.names >/dev/null ||
+	egrep "^$GIT_PERF_SUBSECTION\$" "$TEST_RESULTS_DIR"/run_subsections.names >/dev/null ||
 		die "subsection '$GIT_PERF_SUBSECTION' not found in '$GIT_PERF_CONFIG_FILE'"
 
-	egrep "^$GIT_PERF_SUBSECTION\$" test-results/run_subsections.names | while read -r subsec
+	egrep "^$GIT_PERF_SUBSECTION\$" "$TEST_RESULTS_DIR"/run_subsections.names | while read -r subsec
 	do
 		(
 			GIT_PERF_SUBSECTION="$subsec"
@@ -243,5 +250,5 @@ else
 			echo "======== Run for subsection '$GIT_PERF_SUBSECTION' ========"
 			run_subsection "$@"
 		)
-	done <test-results/run_subsections.names
+	done <"$TEST_RESULTS_DIR"/run_subsections.names
 fi
-- 
2.32.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be
> written in that directory instead of the default directory located in
> "t/". While this works as expected for our normal tests, performance
> tests fail to locate and aggregate performance data because they don't
> know to handle TEST_OUTPUT_DIRECTORY correctly and always look at the
> default location.
>
> Fix the issue by adding a `--results-dir` parameter to "aggregate.perl"
> which identifies the directory where results are and by making the "run"
> script awake of the TEST_OUTPUT_DIRECTORY variable.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

This looks sensible to me, but it is not exactly my area of
expertise.  An Acked-by or Reviewed-by from those who have stake in
t/perf/ would be appreciated.

Thanks.

> Range-diff against v1:
> 1:  8f73bd5d38 ! 1:  cb9d948646 perf: fix when running with TEST_OUTPUT_DIRECTORY
>     @@ t/perf/aggregate.perl: sub usage {
>           --subsection  <str>  * Use results from given subsection
>       
>      @@ t/perf/aggregate.perl: sub sane_backticks {
>     - }
>       
>       my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
>     --    $codespeed, $sortby, $subsection, $reponame);
>     -+    $codespeed, $sortby, $subsection, $reponame, $resultsdir);
>     +     $codespeed, $sortby, $subsection, $reponame);
>     ++my $resultsdir = "test-results";
>       
>       Getopt::Long::Configure qw/ require_order /;
>       
>     @@ t/perf/aggregate.perl: sub sane_backticks {
>       }
>       
>      -my $resultsdir = "test-results";
>     -+if (not $resultsdir) {
>     -+	$resultsdir = "test-results";
>     -+}
>     - 
>     +-
>       if (! $subsection and
>           exists $ENV{GIT_PERF_SUBSECTION} and
>     +     $ENV{GIT_PERF_SUBSECTION} ne "") {
>      
>       ## t/perf/perf-lib.sh ##
>      @@ t/perf/perf-lib.sh: export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP
>     @@ t/perf/perf-lib.sh: test_size () {
>       test_at_end_hook_ () {
>       	if test -z "$GIT_PERF_AGGREGATING_LATER"; then
>      -		( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl $(basename "$0") )
>     -+		( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $(basename "$0") )
>     ++		(
>     ++			cd "$TEST_DIRECTORY"/perf &&
>     ++			./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $(basename "$0")
>     ++		)
>       	fi
>       }
>       
>     @@ t/perf/run: get_var_from_env_or_config "GIT_PERF_SEND_TO_CODESPEED" "perf" "send
>      -get_subsections "perf" >test-results/run_subsections.names
>      +if test -n "$TEST_OUTPUT_DIRECTORY"
>      +then
>     -+    TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
>     ++	TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
>      +else
>     -+    TEST_RESULTS_DIR=test-results
>     ++	TEST_RESULTS_DIR=test-results
>      +fi
>       
>      -if test $(wc -l <test-results/run_subsections.names) -eq 0
>
>  t/perf/aggregate.perl |  5 +++--
>  t/perf/perf-lib.sh    |  7 +++++--
>  t/perf/run            | 25 ++++++++++++++++---------
>  3 files changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
> index 14e4cda287..82c0df4553 100755
> --- a/t/perf/aggregate.perl
> +++ b/t/perf/aggregate.perl
> @@ -58,6 +58,7 @@ sub usage {
>    Options:
>      --codespeed          * Format output for Codespeed
>      --reponame    <str>  * Send given reponame to codespeed
> +    --results-dir <str>  * Directory where test results are located
>      --sort-by     <str>  * Sort output (only "regression" criteria is supported)
>      --subsection  <str>  * Use results from given subsection
>  
> @@ -91,11 +92,13 @@ sub sane_backticks {
>  
>  my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
>      $codespeed, $sortby, $subsection, $reponame);
> +my $resultsdir = "test-results";
>  
>  Getopt::Long::Configure qw/ require_order /;
>  
>  my $rc = GetOptions("codespeed"     => \$codespeed,
>  		    "reponame=s"    => \$reponame,
> +		    "results-dir=s" => \$resultsdir,
>  		    "sort-by=s"     => \$sortby,
>  		    "subsection=s"  => \$subsection);
>  usage() unless $rc;
> @@ -137,8 +140,6 @@ sub sane_backticks {
>  	@tests = glob "p????-*.sh";
>  }
>  
> -my $resultsdir = "test-results";
> -
>  if (! $subsection and
>      exists $ENV{GIT_PERF_SUBSECTION} and
>      $ENV{GIT_PERF_SUBSECTION} ne "") {
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 601d9f67dd..f5ed092ee5 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP
>  MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git
>  export MODERN_GIT
>  
> -perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results
> +perf_results_dir=$TEST_RESULTS_DIR
>  test -n "$GIT_PERF_SUBSECTION" && perf_results_dir="$perf_results_dir/$GIT_PERF_SUBSECTION"
>  mkdir -p "$perf_results_dir"
>  rm -f "$perf_results_dir"/$(basename "$0" .sh).subtests
> @@ -253,7 +253,10 @@ test_size () {
>  # and does it after running everything)
>  test_at_end_hook_ () {
>  	if test -z "$GIT_PERF_AGGREGATING_LATER"; then
> -		( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl $(basename "$0") )
> +		(
> +			cd "$TEST_DIRECTORY"/perf &&
> +			./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $(basename "$0")
> +		)
>  	fi
>  }
>  
> diff --git a/t/perf/run b/t/perf/run
> index c7b86104e1..d19dec258a 100755
> --- a/t/perf/run
> +++ b/t/perf/run
> @@ -188,10 +188,10 @@ run_subsection () {
>  
>  	if test -z "$GIT_PERF_SEND_TO_CODESPEED"
>  	then
> -		./aggregate.perl $codespeed_opt "$@"
> +		./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $codespeed_opt "$@"
>  	else
> -		json_res_file="test-results/$GIT_PERF_SUBSECTION/aggregate.json"
> -		./aggregate.perl --codespeed "$@" | tee "$json_res_file"
> +		json_res_file=""$TEST_RESULTS_DIR"/$GIT_PERF_SUBSECTION/aggregate.json"
> +		./aggregate.perl --results-dir="$TEST_RESULTS_DIR" --codespeed "$@" | tee "$json_res_file"
>  		send_data_url="$GIT_PERF_SEND_TO_CODESPEED/result/add/json/"
>  		curl -v --request POST --data-urlencode "json=$(cat "$json_res_file")" "$send_data_url"
>  	fi
> @@ -203,10 +203,17 @@ get_var_from_env_or_config "GIT_PERF_SEND_TO_CODESPEED" "perf" "sendToCodespeed"
>  cd "$(dirname $0)"
>  . ../../GIT-BUILD-OPTIONS
>  
> -mkdir -p test-results
> -get_subsections "perf" >test-results/run_subsections.names
> +if test -n "$TEST_OUTPUT_DIRECTORY"
> +then
> +	TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
> +else
> +	TEST_RESULTS_DIR=test-results
> +fi
>  
> -if test $(wc -l <test-results/run_subsections.names) -eq 0
> +mkdir -p "$TEST_RESULTS_DIR"
> +get_subsections "perf" >"$TEST_RESULTS_DIR"/run_subsections.names
> +
> +if test $(wc -l <"$TEST_RESULTS_DIR"/run_subsections.names) -eq 0
>  then
>  	if test -n "$GIT_PERF_SUBSECTION"
>  	then
> @@ -222,10 +229,10 @@ then
>  	)
>  elif test -n "$GIT_PERF_SUBSECTION"
>  then
> -	egrep "^$GIT_PERF_SUBSECTION\$" test-results/run_subsections.names >/dev/null ||
> +	egrep "^$GIT_PERF_SUBSECTION\$" "$TEST_RESULTS_DIR"/run_subsections.names >/dev/null ||
>  		die "subsection '$GIT_PERF_SUBSECTION' not found in '$GIT_PERF_CONFIG_FILE'"
>  
> -	egrep "^$GIT_PERF_SUBSECTION\$" test-results/run_subsections.names | while read -r subsec
> +	egrep "^$GIT_PERF_SUBSECTION\$" "$TEST_RESULTS_DIR"/run_subsections.names | while read -r subsec
>  	do
>  		(
>  			GIT_PERF_SUBSECTION="$subsec"
> @@ -243,5 +250,5 @@ else
>  			echo "======== Run for subsection '$GIT_PERF_SUBSECTION' ========"
>  			run_subsection "$@"
>  		)
> -	done <test-results/run_subsections.names
> +	done <"$TEST_RESULTS_DIR"/run_subsections.names
>  fi

Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY

[Jeff King]

On Fri, Jun 18, 2021 at 03:56:08PM +0200, Patrick Steinhardt wrote:

> When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be
> written in that directory instead of the default directory located in
> "t/". While this works as expected for our normal tests, performance
> tests fail to locate and aggregate performance data because they don't
> know to handle TEST_OUTPUT_DIRECTORY correctly and always look at the
> default location.
> 
> Fix the issue by adding a `--results-dir` parameter to "aggregate.perl"
> which identifies the directory where results are and by making the "run"
> script awake of the TEST_OUTPUT_DIRECTORY variable.

OK, that makes sense. My first thought is that the aggregation script
could simply use $TEST_OUTPUT_DIRECTORY itself, but:

  - we don't actually export that (and changing that is likely to have
    undesirable secondary effects)

  - people may run aggregate separately anyway (I know I do in order to
    produce nice tables without having to pointlessly re-run all tests)

So the new option makes sense. It is a little less convenient when
running aggregate manually (you have to say "--results-dir" manually),
but it's better than not being able to do it at all. :)

A few notes / questions below:

> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 601d9f67dd..f5ed092ee5 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP
>  MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git
>  export MODERN_GIT
>  
> -perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results
> +perf_results_dir=$TEST_RESULTS_DIR

This line puzzled me a bit. Isn't $TEST_RESULTS_DIR already defined to
be $TEST_OUTPUT_DIRECTORY/test-results? If the change here is just for
clarity / readability that's OK by me. I just want to make sure I'm not
missing something.

> @@ -253,7 +253,10 @@ test_size () {
>  # and does it after running everything)
>  test_at_end_hook_ () {
>  	if test -z "$GIT_PERF_AGGREGATING_LATER"; then
> -		( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl $(basename "$0") )
> +		(
> +			cd "$TEST_DIRECTORY"/perf &&
> +			./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $(basename "$0")
> +		)
>  	fi
>  }

OK, and we pass it unconditionally, which should work because it will
always be set, even if you aren't overriding the default. Good.

> diff --git a/t/perf/run b/t/perf/run
> index c7b86104e1..d19dec258a 100755
> --- a/t/perf/run
> +++ b/t/perf/run
> @@ -188,10 +188,10 @@ run_subsection () {
>  
>  	if test -z "$GIT_PERF_SEND_TO_CODESPEED"
>  	then
> -		./aggregate.perl $codespeed_opt "$@"
> +		./aggregate.perl --results-dir="$TEST_RESULTS_DIR" $codespeed_opt "$@"
>  	else
> -		json_res_file="test-results/$GIT_PERF_SUBSECTION/aggregate.json"
> -		./aggregate.perl --codespeed "$@" | tee "$json_res_file"
> +		json_res_file=""$TEST_RESULTS_DIR"/$GIT_PERF_SUBSECTION/aggregate.json"
> +		./aggregate.perl --results-dir="$TEST_RESULTS_DIR" --codespeed "$@" | tee "$json_res_file"
>  		send_data_url="$GIT_PERF_SEND_TO_CODESPEED/result/add/json/"
>  		curl -v --request POST --data-urlencode "json=$(cat "$json_res_file")" "$send_data_url"
>  	fi

In the earlier hunks from perf-lib.sh, we got $TEST_RESULTS_DIR by
sourcing test-lib.sh. But in the "run" script, we don't. So who sets
it? Looks like...

> @@ -203,10 +203,17 @@ get_var_from_env_or_config "GIT_PERF_SEND_TO_CODESPEED" "perf" "sendToCodespeed"
>  cd "$(dirname $0)"
>  . ../../GIT-BUILD-OPTIONS
>  
> -mkdir -p test-results
> -get_subsections "perf" >test-results/run_subsections.names
> +if test -n "$TEST_OUTPUT_DIRECTORY"
> +then
> +	TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
> +else
> +	TEST_RESULTS_DIR=test-results
> +fi

...we now do. Makes sense. And we will get TEST_OUTPUT_DIRECTORY
directly from GIT-BUILD-OPTIONS (or it won't be set at all if
unspecified).

So the whole thing looks good to me, assuming there is nothing confusing
about the one assignment I mentioned.

-Peff

Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY

[Patrick Steinhardt]

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

On Mon, Jul 19, 2021 at 04:25:29AM -0400, Jeff King wrote:
> On Fri, Jun 18, 2021 at 03:56:08PM +0200, Patrick Steinhardt wrote:
[snip]
> > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> > index 601d9f67dd..f5ed092ee5 100644
> > --- a/t/perf/perf-lib.sh
> > +++ b/t/perf/perf-lib.sh
> > @@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP
> >  MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git
> >  export MODERN_GIT
> >  
> > -perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results
> > +perf_results_dir=$TEST_RESULTS_DIR
> 
> This line puzzled me a bit. Isn't $TEST_RESULTS_DIR already defined to
> be $TEST_OUTPUT_DIRECTORY/test-results? If the change here is just for
> clarity / readability that's OK by me. I just want to make sure I'm not
> missing something.

Yes it is, and IIRC the change was just for clarity as you assume. The
logic to derive the results directory was essentially duplicated across
perf-lib.sh and test-lib.sh without much of a reason, given that we
always set TEST_RESULTS_DIR in test-lib.sh.

Thanks for your review!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY

[Jeff King]

On Mon, Jul 19, 2021 at 12:52:22PM +0200, Patrick Steinhardt wrote:

> On Mon, Jul 19, 2021 at 04:25:29AM -0400, Jeff King wrote:
> > On Fri, Jun 18, 2021 at 03:56:08PM +0200, Patrick Steinhardt wrote:
> [snip]
> > > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> > > index 601d9f67dd..f5ed092ee5 100644
> > > --- a/t/perf/perf-lib.sh
> > > +++ b/t/perf/perf-lib.sh
> > > @@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP
> > >  MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git
> > >  export MODERN_GIT
> > >  
> > > -perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results
> > > +perf_results_dir=$TEST_RESULTS_DIR
> > 
> > This line puzzled me a bit. Isn't $TEST_RESULTS_DIR already defined to
> > be $TEST_OUTPUT_DIRECTORY/test-results? If the change here is just for
> > clarity / readability that's OK by me. I just want to make sure I'm not
> > missing something.
> 
> Yes it is, and IIRC the change was just for clarity as you assume. The
> logic to derive the results directory was essentially duplicated across
> perf-lib.sh and test-lib.sh without much of a reason, given that we
> always set TEST_RESULTS_DIR in test-lib.sh.

OK, thanks. Then your patch looks great to me. :)

-Peff