* [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY
@ 2021-06-16 9:12 Patrick Steinhardt
2021-06-16 11:33 ` Ævar Arnfjörð Bjarmason
2021-06-18 13:56 ` [PATCH v2] " Patrick Steinhardt
0 siblings, 2 replies; 8+ messages in thread
From: Patrick Steinhardt @ 2021-06-16 9:12 UTC (permalink / raw)
To: git
[-- 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 --]
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY
2021-06-16 9:12 [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY Patrick Steinhardt
@ 2021-06-16 11:33 ` Ævar Arnfjörð Bjarmason
2021-06-16 12:13 ` Patrick Steinhardt
2021-06-18 13:56 ` [PATCH v2] " Patrick Steinhardt
1 sibling, 1 reply; 8+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-16 11:33 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY
2021-06-16 11:33 ` Ævar Arnfjörð Bjarmason
@ 2021-06-16 12:13 ` Patrick Steinhardt
0 siblings, 0 replies; 8+ messages in thread
From: Patrick Steinhardt @ 2021-06-16 12:13 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY
2021-06-16 9:12 [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY Patrick Steinhardt
2021-06-16 11:33 ` Ævar Arnfjörð Bjarmason
@ 2021-06-18 13:56 ` Patrick Steinhardt
2021-06-29 1:12 ` Junio C Hamano
2021-07-19 8:25 ` Jeff King
1 sibling, 2 replies; 8+ messages in thread
From: Patrick Steinhardt @ 2021-06-18 13:56 UTC (permalink / raw)
To: git; +Cc: Ævar Arnfjörð Bjarmason
[-- 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 --]
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY
2021-06-18 13:56 ` [PATCH v2] " Patrick Steinhardt
@ 2021-06-29 1:12 ` Junio C Hamano
2021-07-19 8:25 ` Jeff King
1 sibling, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2021-06-29 1:12 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY
2021-06-18 13:56 ` [PATCH v2] " Patrick Steinhardt
2021-06-29 1:12 ` Junio C Hamano
@ 2021-07-19 8:25 ` Jeff King
2021-07-19 10:52 ` Patrick Steinhardt
1 sibling, 1 reply; 8+ messages in thread
From: Jeff King @ 2021-07-19 8:25 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Junio C Hamano, git, Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY
2021-07-19 8:25 ` Jeff King
@ 2021-07-19 10:52 ` Patrick Steinhardt
2021-07-19 10:53 ` Jeff King
0 siblings, 1 reply; 8+ messages in thread
From: Patrick Steinhardt @ 2021-07-19 10:52 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Ævar Arnfjörð Bjarmason
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY
2021-07-19 10:52 ` Patrick Steinhardt
@ 2021-07-19 10:53 ` Jeff King
0 siblings, 0 replies; 8+ messages in thread
From: Jeff King @ 2021-07-19 10:53 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Junio C Hamano, git, Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-07-19 10:53 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-16 9:12 [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY Patrick Steinhardt
2021-06-16 11:33 ` Ævar Arnfjörð Bjarmason
2021-06-16 12:13 ` Patrick Steinhardt
2021-06-18 13:56 ` [PATCH v2] " Patrick Steinhardt
2021-06-29 1:12 ` Junio C Hamano
2021-07-19 8:25 ` Jeff King
2021-07-19 10:52 ` Patrick Steinhardt
2021-07-19 10:53 ` Jeff King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).