{"thread":{"id":"35171","subject":"[PATCH 2/2] Revert \"test-lib: allow prefixing a custom string before \"ok N\" etc.\"","startedAt":"2013-10-19T21:06:06Z","lastAt":"2013-10-19T21:06:08Z","messageCount":3,"participants":["Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"229227","messageId":"cover.1382215973.git.tr@thomasrast.ch","threadId":"35171","inReplyTo":null,"subject":"[PATCH 0/2] Revert --valgrind-parallel test option","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-10-19T21:06:06Z","receivedAt":"2013-10-19T21:06:06Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"These patches remove the --valgrind-parallel=N option that was broken\nfrom the outset (shame on me).  Peff's judgement at the time that its\nusefulness would approximately be \"meh\" turns out to be correct.\n\nWhat's not in the commit message, but drives part of my reasoning in\ndoing a revert instead of a fix: I did fix it up locally only to\nnotice that it was too slow in this case for what I actually wanted to\nuse it for.  The only valgrind-test workflow that I find bearable is\nto run all the tests in the background under prove (takes hours), and\nthen use the prove output (which says exactly which subtests fail) in\n--valgrind-only=<subtest>.  So the latter is -- again Peff was right\n-- the really useful thing.\n\nThe only consolation is that I apparently didn't break any other use\nof the test suite -- otherwise it would presumably have been fixed\nvery quickly.\n\nThomas Rast (2):\n  Revert \"test-lib: support running tests under valgrind in parallel\"\n  Revert \"test-lib: allow prefixing a custom string before \"ok N\" etc.\"\n\n t/test-lib.sh | 133 +++++++++++++++-------------------------------------------\n 1 file changed, 34 insertions(+), 99 deletions(-)\n\n-- \n1.8.4.1.810.g312044e\n"},{"id":"229226","messageId":"e3f3d660882546609aeeda5d5f8ad5ec999494ff.1382215973.git.tr@thomasrast.ch","threadId":"35171","inReplyTo":"cover.1382215973.git.tr@thomasrast.ch","subject":"[PATCH 1/2] Revert \"test-lib: support running tests under valgrind in parallel\"","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-10-19T21:06:07Z","receivedAt":"2013-10-19T21:06:07Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"This reverts commit ad0e6233320b004f0d686f6887c803e508607bd2.\n\n--valgrind-parallel was broken from the start: during review I made\nthe whole valgrind setup code conditional on not being a\n--valgrind-parallel worker child.  But even the children crucially\nneed $GIT_VALGRIND to be set; it should therefore have been set\noutside the conditional.\n\nThe fix would be a two-liner, but since the introduction of the\nfeature, almost four months have passed without anyone noticing that\nit is broken.  So this feature is not worth the about hundred lines of\ntest-lib.sh complexity.  Revert it.\n\nSigned-off-by: Thomas Rast <tr@thomasrast.ch>\n---\n t/test-lib.sh | 106 ++++++++++++----------------------------------------------\n 1 file changed, 22 insertions(+), 84 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 0fa7dfd..eaf6759 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -205,15 +205,6 @@ do\n \t--valgrind-only=*)\n \t\tvalgrind_only=$(expr \"z$1\" : 'z[^=]*=\\(.*\\)')\n \t\tshift ;;\n-\t--valgrind-parallel=*)\n-\t\tvalgrind_parallel=$(expr \"z$1\" : 'z[^=]*=\\(.*\\)')\n-\t\tshift ;;\n-\t--valgrind-only-stride=*)\n-\t\tvalgrind_only_stride=$(expr \"z$1\" : 'z[^=]*=\\(.*\\)')\n-\t\tshift ;;\n-\t--valgrind-only-offset=*)\n-\t\tvalgrind_only_offset=$(expr \"z$1\" : 'z[^=]*=\\(.*\\)')\n-\t\tshift ;;\n \t--tee)\n \t\tshift ;; # was handled already\n \t--root=*)\n@@ -227,7 +218,7 @@ do\n \tesac\n done\n \n-if test -n \"$valgrind_only\" || test -n \"$valgrind_only_stride\"\n+if test -n \"$valgrind_only\"\n then\n \ttest -z \"$valgrind\" && valgrind=memcheck\n \ttest -z \"$verbose\" && verbose_only=\"$valgrind_only\"\n@@ -377,9 +368,7 @@ maybe_teardown_verbose () {\n last_verbose=t\n maybe_setup_verbose () {\n \ttest -z \"$verbose_only\" && return\n-\tif match_pattern_list $test_count $verbose_only ||\n-\t\t{ test -n \"$valgrind_only_stride\" &&\n-\t\texpr $test_count \"%\" $valgrind_only_stride - $valgrind_only_offset = 0 >/dev/null; }\n+\tif match_pattern_list $test_count $verbose_only\n \tthen\n \t\texec 4>&2 3>&1\n \t\t# Emit a delimiting blank line when going from\n@@ -403,7 +392,7 @@ maybe_teardown_valgrind () {\n \n maybe_setup_valgrind () {\n \ttest -z \"$GIT_VALGRIND\" && return\n-\tif test -z \"$valgrind_only\" && test -z \"$valgrind_only_stride\"\n+\tif test -z \"$valgrind_only\"\n \tthen\n \t\tGIT_VALGRIND_ENABLED=t\n \t\treturn\n@@ -412,10 +401,6 @@ maybe_setup_valgrind () {\n \tif match_pattern_list $test_count $valgrind_only\n \tthen\n \t\tGIT_VALGRIND_ENABLED=t\n-\telif test -n \"$valgrind_only_stride\" &&\n-\t\texpr $test_count \"%\" $valgrind_only_stride - $valgrind_only_offset = 0 >/dev/null\n-\tthen\n-\t\tGIT_VALGRIND_ENABLED=t\n \tfi\n }\n \n@@ -568,9 +553,6 @@ test_done () {\n \tesac\n }\n \n-\n-# Set up a directory that we can put in PATH which redirects all git\n-# calls to 'valgrind git ...'.\n if test -n \"$valgrind\"\n then\n \tmake_symlink () {\n@@ -618,42 +600,33 @@ then\n \t\tmake_symlink \"$symlink_target\" \"$GIT_VALGRIND/bin/$base\" || exit\n \t}\n \n-\t# In the case of --valgrind-parallel, we only need to do the\n-\t# wrapping once, in the main script.  The worker children all\n-\t# have $valgrind_only_stride set, so we can skip based on that.\n-\tif test -z \"$valgrind_only_stride\"\n-\tthen\n-\t\t# override all git executables in TEST_DIRECTORY/..\n-\t\tGIT_VALGRIND=$TEST_DIRECTORY/valgrind\n-\t\tmkdir -p \"$GIT_VALGRIND\"/bin\n-\t\tfor file in $GIT_BUILD_DIR/git* $GIT_BUILD_DIR/test-*\n-\t\tdo\n-\t\t\tmake_valgrind_symlink $file\n-\t\tdone\n-\t\t# special-case the mergetools loadables\n-\t\tmake_symlink \"$GIT_BUILD_DIR\"/mergetools \"$GIT_VALGRIND/bin/mergetools\"\n-\t\tOLDIFS=$IFS\n-\t\tIFS=:\n-\t\tfor path in $PATH\n+\t# override all git executables in TEST_DIRECTORY/..\n+\tGIT_VALGRIND=$TEST_DIRECTORY/valgrind\n+\tmkdir -p \"$GIT_VALGRIND\"/bin\n+\tfor file in $GIT_BUILD_DIR/git* $GIT_BUILD_DIR/test-*\n+\tdo\n+\t\tmake_valgrind_symlink $file\n+\tdone\n+\t# special-case the mergetools loadables\n+\tmake_symlink \"$GIT_BUILD_DIR\"/mergetools \"$GIT_VALGRIND/bin/mergetools\"\n+\tOLDIFS=$IFS\n+\tIFS=:\n+\tfor path in $PATH\n+\tdo\n+\t\tls \"$path\"/git-* 2> /dev/null |\n+\t\twhile read file\n \t\tdo\n-\t\t\tls \"$path\"/git-* 2> /dev/null |\n-\t\t\twhile read file\n-\t\t\tdo\n-\t\t\t\tmake_valgrind_symlink \"$file\"\n-\t\t\tdone\n+\t\t\tmake_valgrind_symlink \"$file\"\n \t\tdone\n-\t\tIFS=$OLDIFS\n-\tfi\n+\tdone\n+\tIFS=$OLDIFS\n \tPATH=$GIT_VALGRIND/bin:$PATH\n \tGIT_EXEC_PATH=$GIT_VALGRIND/bin\n \texport GIT_VALGRIND\n \tGIT_VALGRIND_MODE=\"$valgrind\"\n \texport GIT_VALGRIND_MODE\n \tGIT_VALGRIND_ENABLED=t\n-\tif test -n \"$valgrind_only\" || test -n \"$valgrind_only_stride\"\n-\tthen\n-\t\tGIT_VALGRIND_ENABLED=\n-\tfi\n+\ttest -n \"$valgrind_only\" && GIT_VALGRIND_ENABLED=\n \texport GIT_VALGRIND_ENABLED\n elif test -n \"$GIT_TEST_INSTALLED\"\n then\n@@ -730,41 +703,6 @@ then\n else\n \tmkdir -p \"$TRASH_DIRECTORY\"\n fi\n-\n-# Gross hack to spawn N sub-instances of the tests in parallel, and\n-# summarize the results.  Note that if this is enabled, the script\n-# terminates at the end of this 'if' block.\n-if test -n \"$valgrind_parallel\"\n-then\n-\tfor i in $(test_seq 1 $valgrind_parallel)\n-\tdo\n-\t\troot=\"$TRASH_DIRECTORY/vgparallel-$i\"\n-\t\tmkdir \"$root\"\n-\t\tTEST_OUTPUT_DIRECTORY=\"$root\" \\\n-\t\t\t${SHELL_PATH} \"$0\" \\\n-\t\t\t--root=\"$root\" --statusprefix=\"[$i] \" \\\n-\t\t\t--valgrind=\"$valgrind\" \\\n-\t\t\t--valgrind-only-stride=\"$valgrind_parallel\" \\\n-\t\t\t--valgrind-only-offset=\"$i\" &\n-\t\tpids=\"$pids $!\"\n-\tdone\n-\ttrap \"kill $pids\" INT TERM HUP\n-\twait $pids\n-\ttrap - INT TERM HUP\n-\tfor i in $(test_seq 1 $valgrind_parallel)\n-\tdo\n-\t\troot=\"$TRASH_DIRECTORY/vgparallel-$i\"\n-\t\teval \"$(cat \"$root/test-results/$(basename \"$0\" .sh)\"-*.counts |\n-\t\t\tsed 's/^\\([a-z][a-z]*\\) \\([0-9][0-9]*\\)/inner_\\1=\\2/')\"\n-\t\ttest_count=$(expr $test_count + $inner_total)\n-\t\ttest_success=$(expr $test_success + $inner_success)\n-\t\ttest_fixed=$(expr $test_fixed + $inner_fixed)\n-\t\ttest_broken=$(expr $test_broken + $inner_broken)\n-\t\ttest_failure=$(expr $test_failure + $inner_failed)\n-\tdone\n-\ttest_done\n-fi\n-\n # Use -P to resolve symlinks in our working directory so that the cwd\n # in subprocesses like git equals our $PWD (for pathname comparisons).\n cd -P \"$TRASH_DIRECTORY\" || exit 1\n-- \n1.8.4.1.810.g312044e\n"},{"id":"229225","messageId":"6008d45f4e0edc9e9d2c8e82b4f3cd57495c42f6.1382215973.git.tr@thomasrast.ch","threadId":"35171","inReplyTo":"cover.1382215973.git.tr@thomasrast.ch","subject":"[PATCH 2/2] Revert \"test-lib: allow prefixing a custom string before \"ok N\" etc.\"","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-10-19T21:06:08Z","receivedAt":"2013-10-19T21:06:08Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Now that ad0e623 (test-lib: support running tests under valgrind in\nparallel, 2013-06-23) has been reverted, this support code has no\nusers any more.  Revert it, too.\n\nThis reverts commit e939e15d241e942662b9f88f6127ab470ab0a0b9.\n---\n t/test-lib.sh | 27 ++++++++++++---------------\n 1 file changed, 12 insertions(+), 15 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex eaf6759..29c1410 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -210,9 +210,6 @@ do\n \t--root=*)\n \t\troot=$(expr \"z$1\" : 'z[^=]*=\\(.*\\)')\n \t\tshift ;;\n-\t--statusprefix=*)\n-\t\tstatusprefix=$(expr \"z$1\" : 'z[^=]*=\\(.*\\)')\n-\t\tshift ;;\n \t*)\n \t\techo \"error: unknown test option '$1'\" >&2; exit 1 ;;\n \tesac\n@@ -320,12 +317,12 @@ trap 'die' EXIT\n \n test_ok_ () {\n \ttest_success=$(($test_success + 1))\n-\tsay_color \"\" \"${statusprefix}ok $test_count - $@\"\n+\tsay_color \"\" \"ok $test_count - $@\"\n }\n \n test_failure_ () {\n \ttest_failure=$(($test_failure + 1))\n-\tsay_color error \"${statusprefix}not ok $test_count - $1\"\n+\tsay_color error \"not ok $test_count - $1\"\n \tshift\n \techo \"$@\" | sed -e 's/^/#\t/'\n \ttest \"$immediate\" = \"\" || { GIT_EXIT_OK=t; exit 1; }\n@@ -333,12 +330,12 @@ test_failure_ () {\n \n test_known_broken_ok_ () {\n \ttest_fixed=$(($test_fixed+1))\n-\tsay_color error \"${statusprefix}ok $test_count - $@ # TODO known breakage vanished\"\n+\tsay_color error \"ok $test_count - $@ # TODO known breakage vanished\"\n }\n \n test_known_broken_failure_ () {\n \ttest_broken=$(($test_broken+1))\n-\tsay_color warn \"${statusprefix}not ok $test_count - $@ # TODO known breakage\"\n+\tsay_color warn \"not ok $test_count - $@ # TODO known breakage\"\n }\n \n test_debug () {\n@@ -462,8 +459,8 @@ test_skip () {\n \t\t\tof_prereq=\" of $test_prereq\"\n \t\tfi\n \n-\t\tsay_color skip >&3 \"${statusprefix}skipping test: $@\"\n-\t\tsay_color skip \"${statusprefix}ok $test_count # skip $1 (missing $missing_prereq${of_prereq})\"\n+\t\tsay_color skip >&3 \"skipping test: $@\"\n+\t\tsay_color skip \"ok $test_count # skip $1 (missing $missing_prereq${of_prereq})\"\n \t\t: true\n \t\t;;\n \t*)\n@@ -501,11 +498,11 @@ test_done () {\n \n \tif test \"$test_fixed\" != 0\n \tthen\n-\t\tsay_color error \"${statusprefix}# $test_fixed known breakage(s) vanished; please update test(s)\"\n+\t\tsay_color error \"# $test_fixed known breakage(s) vanished; please update test(s)\"\n \tfi\n \tif test \"$test_broken\" != 0\n \tthen\n-\t\tsay_color warn \"${statusprefix}# still have $test_broken known breakage(s)\"\n+\t\tsay_color warn \"# still have $test_broken known breakage(s)\"\n \tfi\n \tif test \"$test_broken\" != 0 || test \"$test_fixed\" != 0\n \tthen\n@@ -528,9 +525,9 @@ test_done () {\n \t\tthen\n \t\t\tif test $test_remaining -gt 0\n \t\t\tthen\n-\t\t\t\tsay_color pass \"${statusprefix}# passed all $msg\"\n+\t\t\t\tsay_color pass \"# passed all $msg\"\n \t\t\tfi\n-\t\t\tsay \"${statusprefix}1..$test_count$skip_all\"\n+\t\t\tsay \"1..$test_count$skip_all\"\n \t\tfi\n \n \t\ttest -d \"$remove_trash\" &&\n@@ -544,8 +541,8 @@ test_done () {\n \t*)\n \t\tif test $test_external_has_tap -eq 0\n \t\tthen\n-\t\t\tsay_color error \"${statusprefix}# failed $test_failure among $msg\"\n-\t\t\tsay \"${statusprefix}1..$test_count\"\n+\t\t\tsay_color error \"# failed $test_failure among $msg\"\n+\t\t\tsay \"1..$test_count\"\n \t\tfi\n \n \t\texit 1 ;;\n-- \n1.8.4.1.810.g312044e\n"}]}