{"thread":{"id":"55943","subject":"[PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY","startedAt":"2021-06-16T09:10:06Z","lastAt":"2021-07-19T10:53:36Z","messageCount":8,"participants":["Patrick Steinhardt","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"427592","messageId":"005cc9a695f7f9b17190293821369763e9bae662.1623834515.git.ps@pks.im","threadId":"55943","inReplyTo":null,"subject":"[PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-06-16T09:12:05Z","receivedAt":"2021-06-16T09:10:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be\nwritten in that directory instead of the default directory located in\n\"t/\". While this works as expected for our normal tests, performance\ntests fail to locate and aggregate performance data because they don't\nknow to handle TEST_OUTPUT_DIRECTORY correctly and always look at the\ndefault location.\n\nFix the issue by adding a `--results-dir` parameter to \"aggregate.perl\"\nwhich identifies the directory where results are and by making the \"run\"\nscript awake of the TEST_OUTPUT_DIRECTORY variable.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/perf/aggregate.perl |  8 ++++++--\n t/perf/perf-lib.sh    |  4 ++--\n t/perf/run            | 25 ++++++++++++++++---------\n 3 files changed, 24 insertions(+), 13 deletions(-)\n\ndiff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\nindex 14e4cda287..5d4964c5c6 100755\n--- a/t/perf/aggregate.perl\n+++ b/t/perf/aggregate.perl\n@@ -58,6 +58,7 @@ sub usage {\n   Options:\n     --codespeed          * Format output for Codespeed\n     --reponame    <str>  * Send given reponame to codespeed\n+    --results-dir <str>  * Directory where test results are located\n     --sort-by     <str>  * Sort output (only \"regression\" criteria is supported)\n     --subsection  <str>  * Use results from given subsection\n \n@@ -90,12 +91,13 @@ sub sane_backticks {\n }\n \n my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,\n-    $codespeed, $sortby, $subsection, $reponame);\n+    $codespeed, $sortby, $subsection, $reponame, $resultsdir);\n \n Getopt::Long::Configure qw/ require_order /;\n \n my $rc = GetOptions(\"codespeed\"     => \\$codespeed,\n \t\t    \"reponame=s\"    => \\$reponame,\n+\t\t    \"results-dir=s\" => \\$resultsdir,\n \t\t    \"sort-by=s\"     => \\$sortby,\n \t\t    \"subsection=s\"  => \\$subsection);\n usage() unless $rc;\n@@ -137,7 +139,9 @@ sub sane_backticks {\n \t@tests = glob \"p????-*.sh\";\n }\n \n-my $resultsdir = \"test-results\";\n+if (not $resultsdir) {\n+\t$resultsdir = \"test-results\";\n+}\n \n if (! $subsection and\n     exists $ENV{GIT_PERF_SUBSECTION} and\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 601d9f67dd..8ca6dd0297 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP\n MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git\n export MODERN_GIT\n \n-perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results\n+perf_results_dir=$TEST_RESULTS_DIR\n test -n \"$GIT_PERF_SUBSECTION\" && perf_results_dir=\"$perf_results_dir/$GIT_PERF_SUBSECTION\"\n mkdir -p \"$perf_results_dir\"\n rm -f \"$perf_results_dir\"/$(basename \"$0\" .sh).subtests\n@@ -253,7 +253,7 @@ test_size () {\n # and does it after running everything)\n test_at_end_hook_ () {\n \tif test -z \"$GIT_PERF_AGGREGATING_LATER\"; then\n-\t\t( cd \"$TEST_DIRECTORY\"/perf && ./aggregate.perl $(basename \"$0\") )\n+\t\t( cd \"$TEST_DIRECTORY\"/perf && ./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $(basename \"$0\") )\n \tfi\n }\n \ndiff --git a/t/perf/run b/t/perf/run\nindex c7b86104e1..03128d440a 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -188,10 +188,10 @@ run_subsection () {\n \n \tif test -z \"$GIT_PERF_SEND_TO_CODESPEED\"\n \tthen\n-\t\t./aggregate.perl $codespeed_opt \"$@\"\n+\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $codespeed_opt \"$@\"\n \telse\n-\t\tjson_res_file=\"test-results/$GIT_PERF_SUBSECTION/aggregate.json\"\n-\t\t./aggregate.perl --codespeed \"$@\" | tee \"$json_res_file\"\n+\t\tjson_res_file=\"\"$TEST_RESULTS_DIR\"/$GIT_PERF_SUBSECTION/aggregate.json\"\n+\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" --codespeed \"$@\" | tee \"$json_res_file\"\n \t\tsend_data_url=\"$GIT_PERF_SEND_TO_CODESPEED/result/add/json/\"\n \t\tcurl -v --request POST --data-urlencode \"json=$(cat \"$json_res_file\")\" \"$send_data_url\"\n \tfi\n@@ -203,10 +203,17 @@ get_var_from_env_or_config \"GIT_PERF_SEND_TO_CODESPEED\" \"perf\" \"sendToCodespeed\"\n cd \"$(dirname $0)\"\n . ../../GIT-BUILD-OPTIONS\n \n-mkdir -p test-results\n-get_subsections \"perf\" >test-results/run_subsections.names\n+if test -n \"$TEST_OUTPUT_DIRECTORY\"\n+then\n+    TEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n+else\n+    TEST_RESULTS_DIR=test-results\n+fi\n \n-if test $(wc -l <test-results/run_subsections.names) -eq 0\n+mkdir -p \"$TEST_RESULTS_DIR\"\n+get_subsections \"perf\" >\"$TEST_RESULTS_DIR\"/run_subsections.names\n+\n+if test $(wc -l <\"$TEST_RESULTS_DIR\"/run_subsections.names) -eq 0\n then\n \tif test -n \"$GIT_PERF_SUBSECTION\"\n \tthen\n@@ -222,10 +229,10 @@ then\n \t)\n elif test -n \"$GIT_PERF_SUBSECTION\"\n then\n-\tegrep \"^$GIT_PERF_SUBSECTION\\$\" test-results/run_subsections.names >/dev/null ||\n+\tegrep \"^$GIT_PERF_SUBSECTION\\$\" \"$TEST_RESULTS_DIR\"/run_subsections.names >/dev/null ||\n \t\tdie \"subsection '$GIT_PERF_SUBSECTION' not found in '$GIT_PERF_CONFIG_FILE'\"\n \n-\tegrep \"^$GIT_PERF_SUBSECTION\\$\" test-results/run_subsections.names | while read -r subsec\n+\tegrep \"^$GIT_PERF_SUBSECTION\\$\" \"$TEST_RESULTS_DIR\"/run_subsections.names | while read -r subsec\n \tdo\n \t\t(\n \t\t\tGIT_PERF_SUBSECTION=\"$subsec\"\n@@ -243,5 +250,5 @@ else\n \t\t\techo \"======== Run for subsection '$GIT_PERF_SUBSECTION' ========\"\n \t\t\trun_subsection \"$@\"\n \t\t)\n-\tdone <test-results/run_subsections.names\n+\tdone <\"$TEST_RESULTS_DIR\"/run_subsections.names\n fi\n-- \n2.32.0\n\n"},{"id":"427601","messageId":"87mtrqxcq1.fsf@evledraar.gmail.com","threadId":"55943","inReplyTo":"005cc9a695f7f9b17190293821369763e9bae662.1623834515.git.ps@pks.im","subject":"Re: [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-16T11:33:32Z","receivedAt":"2021-06-16T11:36:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 16 2021, Patrick Steinhardt wrote:\n\n> When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be\n> written in that directory instead of the default directory located in\n\nIs the timing of this patch a coincidence, or did you run into this\nrelated to the other patches related to this variable now,\ni.e. https://lore.kernel.org/git/20210609170520.67014-1-felipe.contreras@gmail.com/\nand related.\n\n> Fix the issue by adding a `--results-dir` parameter to \"aggregate.perl\"\n> which identifies the directory where results are and by making the \"run\"\n> script awake of the TEST_OUTPUT_DIRECTORY variable.\n\nMakes sense.\n\n> [...]\n>  my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,\n> -    $codespeed, $sortby, $subsection, $reponame);\n> +    $codespeed, $sortby, $subsection, $reponame, $resultsdir);\n>  \n>  Getopt::Long::Configure qw/ require_order /;\n>  \n>  my $rc = GetOptions(\"codespeed\"     => \\$codespeed,\n>  \t\t    \"reponame=s\"    => \\$reponame,\n> +\t\t    \"results-dir=s\" => \\$resultsdir,\n>  \t\t    \"sort-by=s\"     => \\$sortby,\n>  \t\t    \"subsection=s\"  => \\$subsection);\n>  usage() unless $rc;\n> @@ -137,7 +139,9 @@ sub sane_backticks {\n>  \t@tests = glob \"p????-*.sh\";\n>  }\n>  \n> -my $resultsdir = \"test-results\";\n> +if (not $resultsdir) {\n> +\t$resultsdir = \"test-results\";\n> +}\n\nWorks, but FWIW in git.git's perl scripts it's usual to do:\n\nmy $resultsdir = \"test-results\";\nGetOptions(...);\n\nWhich serves the same purpose. You can also inline this in the GetOptions call:\n\t\n\tGetOptions(...,\n\t\t\"results-dir=s\" => \\(my $resultsdir = \"test-results\"),\n\t);\n\nBut maybe that's too obscurely Perl-ish, i.e. declaring a variable for\nour scope inline in another function's argument list, and we should just\nuse the first idiom we use in most other places.\n\n\n>  \tif test -z \"$GIT_PERF_AGGREGATING_LATER\"; then\n> -\t\t( cd \"$TEST_DIRECTORY\"/perf && ./aggregate.perl $(basename \"$0\") )\n> +\t\t( cd \"$TEST_DIRECTORY\"/perf && ./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $(basename \"$0\") )\n>  \tfi\n>  }\n\nMakes sense to split up the overly long line at this point probably...\n\n>[...]\n> -mkdir -p test-results\n> -get_subsections \"perf\" >test-results/run_subsections.names\n> +if test -n \"$TEST_OUTPUT_DIRECTORY\"\n> +then\n> +    TEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n> +else\n> +    TEST_RESULTS_DIR=test-results\n> +fi\n\nIndending with spaces?\n"},{"id":"427606","messageId":"YMnq9uPnmjotC5w7@ncase","threadId":"55943","inReplyTo":"87mtrqxcq1.fsf@evledraar.gmail.com","subject":"Re: [PATCH] perf: fix when running with TEST_OUTPUT_DIRECTORY","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-06-16T12:13:42Z","receivedAt":"2021-06-16T12:13:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jun 16, 2021 at 01:33:32PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Jun 16 2021, Patrick Steinhardt wrote:\n> \n> > When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be\n> > written in that directory instead of the default directory located in\n> \n> Is the timing of this patch a coincidence, or did you run into this\n> related to the other patches related to this variable now,\n> i.e. https://lore.kernel.org/git/20210609170520.67014-1-felipe.contreras@gmail.com/\n> and related.\n\nCoincidence. In fact, I already sent this patch a few weeks ago as part\nof [1]. But given that this patch set turned out to not be an easy sell\nand that I didn't yet find the time to work on it again, I decided to\nsplit out this patch and upstream it standalone.\n\nYour remarks all make sense to me, will address them in v2. Thanks!\n\nPatrick\n\n[1]: http://public-inbox.org/git/cover.1621451532.git.ps@pks.im/\n"},{"id":"427828","messageId":"cb9d948646e8181e9c9c8dc35b37d48c692a92fb.1624024532.git.ps@pks.im","threadId":"55943","inReplyTo":"005cc9a695f7f9b17190293821369763e9bae662.1623834515.git.ps@pks.im","subject":"[PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-06-18T13:56:08Z","receivedAt":"2021-06-18T13:51:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be\nwritten in that directory instead of the default directory located in\n\"t/\". While this works as expected for our normal tests, performance\ntests fail to locate and aggregate performance data because they don't\nknow to handle TEST_OUTPUT_DIRECTORY correctly and always look at the\ndefault location.\n\nFix the issue by adding a `--results-dir` parameter to \"aggregate.perl\"\nwhich identifies the directory where results are and by making the \"run\"\nscript awake of the TEST_OUTPUT_DIRECTORY variable.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nRange-diff against v1:\n1:  8f73bd5d38 ! 1:  cb9d948646 perf: fix when running with TEST_OUTPUT_DIRECTORY\n    @@ t/perf/aggregate.perl: sub usage {\n          --subsection  <str>  * Use results from given subsection\n      \n     @@ t/perf/aggregate.perl: sub sane_backticks {\n    - }\n      \n      my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,\n    --    $codespeed, $sortby, $subsection, $reponame);\n    -+    $codespeed, $sortby, $subsection, $reponame, $resultsdir);\n    +     $codespeed, $sortby, $subsection, $reponame);\n    ++my $resultsdir = \"test-results\";\n      \n      Getopt::Long::Configure qw/ require_order /;\n      \n    @@ t/perf/aggregate.perl: sub sane_backticks {\n      }\n      \n     -my $resultsdir = \"test-results\";\n    -+if (not $resultsdir) {\n    -+\t$resultsdir = \"test-results\";\n    -+}\n    - \n    +-\n      if (! $subsection and\n          exists $ENV{GIT_PERF_SUBSECTION} and\n    +     $ENV{GIT_PERF_SUBSECTION} ne \"\") {\n     \n      ## t/perf/perf-lib.sh ##\n     @@ t/perf/perf-lib.sh: export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP\n    @@ t/perf/perf-lib.sh: test_size () {\n      test_at_end_hook_ () {\n      \tif test -z \"$GIT_PERF_AGGREGATING_LATER\"; then\n     -\t\t( cd \"$TEST_DIRECTORY\"/perf && ./aggregate.perl $(basename \"$0\") )\n    -+\t\t( cd \"$TEST_DIRECTORY\"/perf && ./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $(basename \"$0\") )\n    ++\t\t(\n    ++\t\t\tcd \"$TEST_DIRECTORY\"/perf &&\n    ++\t\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $(basename \"$0\")\n    ++\t\t)\n      \tfi\n      }\n      \n    @@ t/perf/run: get_var_from_env_or_config \"GIT_PERF_SEND_TO_CODESPEED\" \"perf\" \"send\n     -get_subsections \"perf\" >test-results/run_subsections.names\n     +if test -n \"$TEST_OUTPUT_DIRECTORY\"\n     +then\n    -+    TEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n    ++\tTEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n     +else\n    -+    TEST_RESULTS_DIR=test-results\n    ++\tTEST_RESULTS_DIR=test-results\n     +fi\n      \n     -if test $(wc -l <test-results/run_subsections.names) -eq 0\n\n t/perf/aggregate.perl |  5 +++--\n t/perf/perf-lib.sh    |  7 +++++--\n t/perf/run            | 25 ++++++++++++++++---------\n 3 files changed, 24 insertions(+), 13 deletions(-)\n\ndiff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\nindex 14e4cda287..82c0df4553 100755\n--- a/t/perf/aggregate.perl\n+++ b/t/perf/aggregate.perl\n@@ -58,6 +58,7 @@ sub usage {\n   Options:\n     --codespeed          * Format output for Codespeed\n     --reponame    <str>  * Send given reponame to codespeed\n+    --results-dir <str>  * Directory where test results are located\n     --sort-by     <str>  * Sort output (only \"regression\" criteria is supported)\n     --subsection  <str>  * Use results from given subsection\n \n@@ -91,11 +92,13 @@ sub sane_backticks {\n \n my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,\n     $codespeed, $sortby, $subsection, $reponame);\n+my $resultsdir = \"test-results\";\n \n Getopt::Long::Configure qw/ require_order /;\n \n my $rc = GetOptions(\"codespeed\"     => \\$codespeed,\n \t\t    \"reponame=s\"    => \\$reponame,\n+\t\t    \"results-dir=s\" => \\$resultsdir,\n \t\t    \"sort-by=s\"     => \\$sortby,\n \t\t    \"subsection=s\"  => \\$subsection);\n usage() unless $rc;\n@@ -137,8 +140,6 @@ sub sane_backticks {\n \t@tests = glob \"p????-*.sh\";\n }\n \n-my $resultsdir = \"test-results\";\n-\n if (! $subsection and\n     exists $ENV{GIT_PERF_SUBSECTION} and\n     $ENV{GIT_PERF_SUBSECTION} ne \"\") {\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 601d9f67dd..f5ed092ee5 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP\n MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git\n export MODERN_GIT\n \n-perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results\n+perf_results_dir=$TEST_RESULTS_DIR\n test -n \"$GIT_PERF_SUBSECTION\" && perf_results_dir=\"$perf_results_dir/$GIT_PERF_SUBSECTION\"\n mkdir -p \"$perf_results_dir\"\n rm -f \"$perf_results_dir\"/$(basename \"$0\" .sh).subtests\n@@ -253,7 +253,10 @@ test_size () {\n # and does it after running everything)\n test_at_end_hook_ () {\n \tif test -z \"$GIT_PERF_AGGREGATING_LATER\"; then\n-\t\t( cd \"$TEST_DIRECTORY\"/perf && ./aggregate.perl $(basename \"$0\") )\n+\t\t(\n+\t\t\tcd \"$TEST_DIRECTORY\"/perf &&\n+\t\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $(basename \"$0\")\n+\t\t)\n \tfi\n }\n \ndiff --git a/t/perf/run b/t/perf/run\nindex c7b86104e1..d19dec258a 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -188,10 +188,10 @@ run_subsection () {\n \n \tif test -z \"$GIT_PERF_SEND_TO_CODESPEED\"\n \tthen\n-\t\t./aggregate.perl $codespeed_opt \"$@\"\n+\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $codespeed_opt \"$@\"\n \telse\n-\t\tjson_res_file=\"test-results/$GIT_PERF_SUBSECTION/aggregate.json\"\n-\t\t./aggregate.perl --codespeed \"$@\" | tee \"$json_res_file\"\n+\t\tjson_res_file=\"\"$TEST_RESULTS_DIR\"/$GIT_PERF_SUBSECTION/aggregate.json\"\n+\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" --codespeed \"$@\" | tee \"$json_res_file\"\n \t\tsend_data_url=\"$GIT_PERF_SEND_TO_CODESPEED/result/add/json/\"\n \t\tcurl -v --request POST --data-urlencode \"json=$(cat \"$json_res_file\")\" \"$send_data_url\"\n \tfi\n@@ -203,10 +203,17 @@ get_var_from_env_or_config \"GIT_PERF_SEND_TO_CODESPEED\" \"perf\" \"sendToCodespeed\"\n cd \"$(dirname $0)\"\n . ../../GIT-BUILD-OPTIONS\n \n-mkdir -p test-results\n-get_subsections \"perf\" >test-results/run_subsections.names\n+if test -n \"$TEST_OUTPUT_DIRECTORY\"\n+then\n+\tTEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n+else\n+\tTEST_RESULTS_DIR=test-results\n+fi\n \n-if test $(wc -l <test-results/run_subsections.names) -eq 0\n+mkdir -p \"$TEST_RESULTS_DIR\"\n+get_subsections \"perf\" >\"$TEST_RESULTS_DIR\"/run_subsections.names\n+\n+if test $(wc -l <\"$TEST_RESULTS_DIR\"/run_subsections.names) -eq 0\n then\n \tif test -n \"$GIT_PERF_SUBSECTION\"\n \tthen\n@@ -222,10 +229,10 @@ then\n \t)\n elif test -n \"$GIT_PERF_SUBSECTION\"\n then\n-\tegrep \"^$GIT_PERF_SUBSECTION\\$\" test-results/run_subsections.names >/dev/null ||\n+\tegrep \"^$GIT_PERF_SUBSECTION\\$\" \"$TEST_RESULTS_DIR\"/run_subsections.names >/dev/null ||\n \t\tdie \"subsection '$GIT_PERF_SUBSECTION' not found in '$GIT_PERF_CONFIG_FILE'\"\n \n-\tegrep \"^$GIT_PERF_SUBSECTION\\$\" test-results/run_subsections.names | while read -r subsec\n+\tegrep \"^$GIT_PERF_SUBSECTION\\$\" \"$TEST_RESULTS_DIR\"/run_subsections.names | while read -r subsec\n \tdo\n \t\t(\n \t\t\tGIT_PERF_SUBSECTION=\"$subsec\"\n@@ -243,5 +250,5 @@ else\n \t\t\techo \"======== Run for subsection '$GIT_PERF_SUBSECTION' ========\"\n \t\t\trun_subsection \"$@\"\n \t\t)\n-\tdone <test-results/run_subsections.names\n+\tdone <\"$TEST_RESULTS_DIR\"/run_subsections.names\n fi\n-- \n2.32.0\n\n"},{"id":"428655","messageId":"xmqqbl7p4goq.fsf@gitster.g","threadId":"55943","inReplyTo":"cb9d948646e8181e9c9c8dc35b37d48c692a92fb.1624024532.git.ps@pks.im","subject":"Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-29T01:12:37Z","receivedAt":"2021-06-29T01:12:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be\n> written in that directory instead of the default directory located in\n> \"t/\". While this works as expected for our normal tests, performance\n> tests fail to locate and aggregate performance data because they don't\n> know to handle TEST_OUTPUT_DIRECTORY correctly and always look at the\n> default location.\n>\n> Fix the issue by adding a `--results-dir` parameter to \"aggregate.perl\"\n> which identifies the directory where results are and by making the \"run\"\n> script awake of the TEST_OUTPUT_DIRECTORY variable.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n\nThis looks sensible to me, but it is not exactly my area of\nexpertise.  An Acked-by or Reviewed-by from those who have stake in\nt/perf/ would be appreciated.\n\nThanks.\n\n> Range-diff against v1:\n> 1:  8f73bd5d38 ! 1:  cb9d948646 perf: fix when running with TEST_OUTPUT_DIRECTORY\n>     @@ t/perf/aggregate.perl: sub usage {\n>           --subsection  <str>  * Use results from given subsection\n>       \n>      @@ t/perf/aggregate.perl: sub sane_backticks {\n>     - }\n>       \n>       my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,\n>     --    $codespeed, $sortby, $subsection, $reponame);\n>     -+    $codespeed, $sortby, $subsection, $reponame, $resultsdir);\n>     +     $codespeed, $sortby, $subsection, $reponame);\n>     ++my $resultsdir = \"test-results\";\n>       \n>       Getopt::Long::Configure qw/ require_order /;\n>       \n>     @@ t/perf/aggregate.perl: sub sane_backticks {\n>       }\n>       \n>      -my $resultsdir = \"test-results\";\n>     -+if (not $resultsdir) {\n>     -+\t$resultsdir = \"test-results\";\n>     -+}\n>     - \n>     +-\n>       if (! $subsection and\n>           exists $ENV{GIT_PERF_SUBSECTION} and\n>     +     $ENV{GIT_PERF_SUBSECTION} ne \"\") {\n>      \n>       ## t/perf/perf-lib.sh ##\n>      @@ t/perf/perf-lib.sh: export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP\n>     @@ t/perf/perf-lib.sh: test_size () {\n>       test_at_end_hook_ () {\n>       \tif test -z \"$GIT_PERF_AGGREGATING_LATER\"; then\n>      -\t\t( cd \"$TEST_DIRECTORY\"/perf && ./aggregate.perl $(basename \"$0\") )\n>     -+\t\t( cd \"$TEST_DIRECTORY\"/perf && ./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $(basename \"$0\") )\n>     ++\t\t(\n>     ++\t\t\tcd \"$TEST_DIRECTORY\"/perf &&\n>     ++\t\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $(basename \"$0\")\n>     ++\t\t)\n>       \tfi\n>       }\n>       \n>     @@ t/perf/run: get_var_from_env_or_config \"GIT_PERF_SEND_TO_CODESPEED\" \"perf\" \"send\n>      -get_subsections \"perf\" >test-results/run_subsections.names\n>      +if test -n \"$TEST_OUTPUT_DIRECTORY\"\n>      +then\n>     -+    TEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n>     ++\tTEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n>      +else\n>     -+    TEST_RESULTS_DIR=test-results\n>     ++\tTEST_RESULTS_DIR=test-results\n>      +fi\n>       \n>      -if test $(wc -l <test-results/run_subsections.names) -eq 0\n>\n>  t/perf/aggregate.perl |  5 +++--\n>  t/perf/perf-lib.sh    |  7 +++++--\n>  t/perf/run            | 25 ++++++++++++++++---------\n>  3 files changed, 24 insertions(+), 13 deletions(-)\n>\n> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\n> index 14e4cda287..82c0df4553 100755\n> --- a/t/perf/aggregate.perl\n> +++ b/t/perf/aggregate.perl\n> @@ -58,6 +58,7 @@ sub usage {\n>    Options:\n>      --codespeed          * Format output for Codespeed\n>      --reponame    <str>  * Send given reponame to codespeed\n> +    --results-dir <str>  * Directory where test results are located\n>      --sort-by     <str>  * Sort output (only \"regression\" criteria is supported)\n>      --subsection  <str>  * Use results from given subsection\n>  \n> @@ -91,11 +92,13 @@ sub sane_backticks {\n>  \n>  my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,\n>      $codespeed, $sortby, $subsection, $reponame);\n> +my $resultsdir = \"test-results\";\n>  \n>  Getopt::Long::Configure qw/ require_order /;\n>  \n>  my $rc = GetOptions(\"codespeed\"     => \\$codespeed,\n>  \t\t    \"reponame=s\"    => \\$reponame,\n> +\t\t    \"results-dir=s\" => \\$resultsdir,\n>  \t\t    \"sort-by=s\"     => \\$sortby,\n>  \t\t    \"subsection=s\"  => \\$subsection);\n>  usage() unless $rc;\n> @@ -137,8 +140,6 @@ sub sane_backticks {\n>  \t@tests = glob \"p????-*.sh\";\n>  }\n>  \n> -my $resultsdir = \"test-results\";\n> -\n>  if (! $subsection and\n>      exists $ENV{GIT_PERF_SUBSECTION} and\n>      $ENV{GIT_PERF_SUBSECTION} ne \"\") {\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index 601d9f67dd..f5ed092ee5 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP\n>  MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git\n>  export MODERN_GIT\n>  \n> -perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results\n> +perf_results_dir=$TEST_RESULTS_DIR\n>  test -n \"$GIT_PERF_SUBSECTION\" && perf_results_dir=\"$perf_results_dir/$GIT_PERF_SUBSECTION\"\n>  mkdir -p \"$perf_results_dir\"\n>  rm -f \"$perf_results_dir\"/$(basename \"$0\" .sh).subtests\n> @@ -253,7 +253,10 @@ test_size () {\n>  # and does it after running everything)\n>  test_at_end_hook_ () {\n>  \tif test -z \"$GIT_PERF_AGGREGATING_LATER\"; then\n> -\t\t( cd \"$TEST_DIRECTORY\"/perf && ./aggregate.perl $(basename \"$0\") )\n> +\t\t(\n> +\t\t\tcd \"$TEST_DIRECTORY\"/perf &&\n> +\t\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $(basename \"$0\")\n> +\t\t)\n>  \tfi\n>  }\n>  \n> diff --git a/t/perf/run b/t/perf/run\n> index c7b86104e1..d19dec258a 100755\n> --- a/t/perf/run\n> +++ b/t/perf/run\n> @@ -188,10 +188,10 @@ run_subsection () {\n>  \n>  \tif test -z \"$GIT_PERF_SEND_TO_CODESPEED\"\n>  \tthen\n> -\t\t./aggregate.perl $codespeed_opt \"$@\"\n> +\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $codespeed_opt \"$@\"\n>  \telse\n> -\t\tjson_res_file=\"test-results/$GIT_PERF_SUBSECTION/aggregate.json\"\n> -\t\t./aggregate.perl --codespeed \"$@\" | tee \"$json_res_file\"\n> +\t\tjson_res_file=\"\"$TEST_RESULTS_DIR\"/$GIT_PERF_SUBSECTION/aggregate.json\"\n> +\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" --codespeed \"$@\" | tee \"$json_res_file\"\n>  \t\tsend_data_url=\"$GIT_PERF_SEND_TO_CODESPEED/result/add/json/\"\n>  \t\tcurl -v --request POST --data-urlencode \"json=$(cat \"$json_res_file\")\" \"$send_data_url\"\n>  \tfi\n> @@ -203,10 +203,17 @@ get_var_from_env_or_config \"GIT_PERF_SEND_TO_CODESPEED\" \"perf\" \"sendToCodespeed\"\n>  cd \"$(dirname $0)\"\n>  . ../../GIT-BUILD-OPTIONS\n>  \n> -mkdir -p test-results\n> -get_subsections \"perf\" >test-results/run_subsections.names\n> +if test -n \"$TEST_OUTPUT_DIRECTORY\"\n> +then\n> +\tTEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n> +else\n> +\tTEST_RESULTS_DIR=test-results\n> +fi\n>  \n> -if test $(wc -l <test-results/run_subsections.names) -eq 0\n> +mkdir -p \"$TEST_RESULTS_DIR\"\n> +get_subsections \"perf\" >\"$TEST_RESULTS_DIR\"/run_subsections.names\n> +\n> +if test $(wc -l <\"$TEST_RESULTS_DIR\"/run_subsections.names) -eq 0\n>  then\n>  \tif test -n \"$GIT_PERF_SUBSECTION\"\n>  \tthen\n> @@ -222,10 +229,10 @@ then\n>  \t)\n>  elif test -n \"$GIT_PERF_SUBSECTION\"\n>  then\n> -\tegrep \"^$GIT_PERF_SUBSECTION\\$\" test-results/run_subsections.names >/dev/null ||\n> +\tegrep \"^$GIT_PERF_SUBSECTION\\$\" \"$TEST_RESULTS_DIR\"/run_subsections.names >/dev/null ||\n>  \t\tdie \"subsection '$GIT_PERF_SUBSECTION' not found in '$GIT_PERF_CONFIG_FILE'\"\n>  \n> -\tegrep \"^$GIT_PERF_SUBSECTION\\$\" test-results/run_subsections.names | while read -r subsec\n> +\tegrep \"^$GIT_PERF_SUBSECTION\\$\" \"$TEST_RESULTS_DIR\"/run_subsections.names | while read -r subsec\n>  \tdo\n>  \t\t(\n>  \t\t\tGIT_PERF_SUBSECTION=\"$subsec\"\n> @@ -243,5 +250,5 @@ else\n>  \t\t\techo \"======== Run for subsection '$GIT_PERF_SUBSECTION' ========\"\n>  \t\t\trun_subsection \"$@\"\n>  \t\t)\n> -\tdone <test-results/run_subsections.names\n> +\tdone <\"$TEST_RESULTS_DIR\"/run_subsections.names\n>  fi\n"},{"id":"430484","messageId":"YPU2+bG8Nge5FKWv@coredump.intra.peff.net","threadId":"55943","inReplyTo":"cb9d948646e8181e9c9c8dc35b37d48c692a92fb.1624024532.git.ps@pks.im","subject":"Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-19T08:25:29Z","receivedAt":"2021-07-19T09:25:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 18, 2021 at 03:56:08PM +0200, Patrick Steinhardt wrote:\n\n> When the TEST_OUTPUT_DIRECTORY is defined, then all test data will be\n> written in that directory instead of the default directory located in\n> \"t/\". While this works as expected for our normal tests, performance\n> tests fail to locate and aggregate performance data because they don't\n> know to handle TEST_OUTPUT_DIRECTORY correctly and always look at the\n> default location.\n> \n> Fix the issue by adding a `--results-dir` parameter to \"aggregate.perl\"\n> which identifies the directory where results are and by making the \"run\"\n> script awake of the TEST_OUTPUT_DIRECTORY variable.\n\nOK, that makes sense. My first thought is that the aggregation script\ncould simply use $TEST_OUTPUT_DIRECTORY itself, but:\n\n  - we don't actually export that (and changing that is likely to have\n    undesirable secondary effects)\n\n  - people may run aggregate separately anyway (I know I do in order to\n    produce nice tables without having to pointlessly re-run all tests)\n\nSo the new option makes sense. It is a little less convenient when\nrunning aggregate manually (you have to say \"--results-dir\" manually),\nbut it's better than not being able to do it at all. :)\n\nA few notes / questions below:\n\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index 601d9f67dd..f5ed092ee5 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP\n>  MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git\n>  export MODERN_GIT\n>  \n> -perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results\n> +perf_results_dir=$TEST_RESULTS_DIR\n\nThis line puzzled me a bit. Isn't $TEST_RESULTS_DIR already defined to\nbe $TEST_OUTPUT_DIRECTORY/test-results? If the change here is just for\nclarity / readability that's OK by me. I just want to make sure I'm not\nmissing something.\n\n> @@ -253,7 +253,10 @@ test_size () {\n>  # and does it after running everything)\n>  test_at_end_hook_ () {\n>  \tif test -z \"$GIT_PERF_AGGREGATING_LATER\"; then\n> -\t\t( cd \"$TEST_DIRECTORY\"/perf && ./aggregate.perl $(basename \"$0\") )\n> +\t\t(\n> +\t\t\tcd \"$TEST_DIRECTORY\"/perf &&\n> +\t\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $(basename \"$0\")\n> +\t\t)\n>  \tfi\n>  }\n\nOK, and we pass it unconditionally, which should work because it will\nalways be set, even if you aren't overriding the default. Good.\n\n> diff --git a/t/perf/run b/t/perf/run\n> index c7b86104e1..d19dec258a 100755\n> --- a/t/perf/run\n> +++ b/t/perf/run\n> @@ -188,10 +188,10 @@ run_subsection () {\n>  \n>  \tif test -z \"$GIT_PERF_SEND_TO_CODESPEED\"\n>  \tthen\n> -\t\t./aggregate.perl $codespeed_opt \"$@\"\n> +\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" $codespeed_opt \"$@\"\n>  \telse\n> -\t\tjson_res_file=\"test-results/$GIT_PERF_SUBSECTION/aggregate.json\"\n> -\t\t./aggregate.perl --codespeed \"$@\" | tee \"$json_res_file\"\n> +\t\tjson_res_file=\"\"$TEST_RESULTS_DIR\"/$GIT_PERF_SUBSECTION/aggregate.json\"\n> +\t\t./aggregate.perl --results-dir=\"$TEST_RESULTS_DIR\" --codespeed \"$@\" | tee \"$json_res_file\"\n>  \t\tsend_data_url=\"$GIT_PERF_SEND_TO_CODESPEED/result/add/json/\"\n>  \t\tcurl -v --request POST --data-urlencode \"json=$(cat \"$json_res_file\")\" \"$send_data_url\"\n>  \tfi\n\nIn the earlier hunks from perf-lib.sh, we got $TEST_RESULTS_DIR by\nsourcing test-lib.sh. But in the \"run\" script, we don't. So who sets\nit? Looks like...\n\n> @@ -203,10 +203,17 @@ get_var_from_env_or_config \"GIT_PERF_SEND_TO_CODESPEED\" \"perf\" \"sendToCodespeed\"\n>  cd \"$(dirname $0)\"\n>  . ../../GIT-BUILD-OPTIONS\n>  \n> -mkdir -p test-results\n> -get_subsections \"perf\" >test-results/run_subsections.names\n> +if test -n \"$TEST_OUTPUT_DIRECTORY\"\n> +then\n> +\tTEST_RESULTS_DIR=\"$TEST_OUTPUT_DIRECTORY/test-results\"\n> +else\n> +\tTEST_RESULTS_DIR=test-results\n> +fi\n\n...we now do. Makes sense. And we will get TEST_OUTPUT_DIRECTORY\ndirectly from GIT-BUILD-OPTIONS (or it won't be set at all if\nunspecified).\n\nSo the whole thing looks good to me, assuming there is nothing confusing\nabout the one assignment I mentioned.\n\n-Peff\n"},{"id":"430488","messageId":"YPVZZl1FI0D8kK6C@ncase","threadId":"55943","inReplyTo":"YPU2+bG8Nge5FKWv@coredump.intra.peff.net","subject":"Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-07-19T10:52:22Z","receivedAt":"2021-07-19T10:52:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 19, 2021 at 04:25:29AM -0400, Jeff King wrote:\n> On Fri, Jun 18, 2021 at 03:56:08PM +0200, Patrick Steinhardt wrote:\n[snip]\n> > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> > index 601d9f67dd..f5ed092ee5 100644\n> > --- a/t/perf/perf-lib.sh\n> > +++ b/t/perf/perf-lib.sh\n> > @@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP\n> >  MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git\n> >  export MODERN_GIT\n> >  \n> > -perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results\n> > +perf_results_dir=$TEST_RESULTS_DIR\n> \n> This line puzzled me a bit. Isn't $TEST_RESULTS_DIR already defined to\n> be $TEST_OUTPUT_DIRECTORY/test-results? If the change here is just for\n> clarity / readability that's OK by me. I just want to make sure I'm not\n> missing something.\n\nYes it is, and IIRC the change was just for clarity as you assume. The\nlogic to derive the results directory was essentially duplicated across\nperf-lib.sh and test-lib.sh without much of a reason, given that we\nalways set TEST_RESULTS_DIR in test-lib.sh.\n\nThanks for your review!\n\nPatrick\n"},{"id":"430490","messageId":"YPVZqn0Lm6m6GC2Z@coredump.intra.peff.net","threadId":"55943","inReplyTo":"YPVZZl1FI0D8kK6C@ncase","subject":"Re: [PATCH v2] perf: fix when running with TEST_OUTPUT_DIRECTORY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-19T10:53:30Z","receivedAt":"2021-07-19T10:53:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 19, 2021 at 12:52:22PM +0200, Patrick Steinhardt wrote:\n\n> On Mon, Jul 19, 2021 at 04:25:29AM -0400, Jeff King wrote:\n> > On Fri, Jun 18, 2021 at 03:56:08PM +0200, Patrick Steinhardt wrote:\n> [snip]\n> > > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> > > index 601d9f67dd..f5ed092ee5 100644\n> > > --- a/t/perf/perf-lib.sh\n> > > +++ b/t/perf/perf-lib.sh\n> > > @@ -45,7 +45,7 @@ export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP\n> > >  MODERN_GIT=$GIT_BUILD_DIR/bin-wrappers/git\n> > >  export MODERN_GIT\n> > >  \n> > > -perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results\n> > > +perf_results_dir=$TEST_RESULTS_DIR\n> > \n> > This line puzzled me a bit. Isn't $TEST_RESULTS_DIR already defined to\n> > be $TEST_OUTPUT_DIRECTORY/test-results? If the change here is just for\n> > clarity / readability that's OK by me. I just want to make sure I'm not\n> > missing something.\n> \n> Yes it is, and IIRC the change was just for clarity as you assume. The\n> logic to derive the results directory was essentially duplicated across\n> perf-lib.sh and test-lib.sh without much of a reason, given that we\n> always set TEST_RESULTS_DIR in test-lib.sh.\n\nOK, thanks. Then your patch looks great to me. :)\n\n-Peff\n"}]}