| Message ID | 005cc9a695f7f9b17190293821369763e9bae662.1623834515.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | perf: fix when running with TEST_OUTPUT_DIRECTORY | expand |
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?
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/
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
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(-)