{"thread":{"id":"50349","subject":"[PATCH 0/1] Makefile: add prove and coverage-prove targets","startedAt":"2019-01-29T14:56:11Z","lastAt":"2019-01-31T07:24:24Z","messageCount":22,"participants":["Derrick Stolee via GitGitGadget","Johannes Schindelin","SZEDER Gábor","Jeff King","Derrick Stolee","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"367985","messageId":"pull.114.git.gitgitgadget@gmail.com","threadId":"50349","inReplyTo":null,"subject":"[PATCH 0/1] Makefile: add prove and coverage-prove targets","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-01-29T14:56:07Z","receivedAt":"2019-01-29T14:56:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Sometimes there are test failures in the 'pu' branch. This is somewhat\nexpected for a branch that takes the very latest topics under development,\nand those sometimes have semantic conflicts that only show up during test\nruns. This also can happen when running the test suite with different\nGIT_TEST_* environment variables that interact in unexpected ways.\n\nThis causes a problem for the test coverage reports, as the typical 'make\ncoverage-test coverage-report' run halts at the first failed test. If that\ntest is early in the suite, then many valuable tests are not exercising the\ncode and the coverage report becomes noisy with false positives.\n\nThis patch adds two targets to the Makefile: 'prove' and 'coverage-prove'.\nThe idea is to use the 'prove' tool to run the test suite, as it will track\nfailed tests but continue running the full suite even with a failure.\n\nIf/when this merges down, I will use this new target for the automation\naround the test coverage reports.\n\nThanks, -Stolee\n\nDerrick Stolee (1):\n  Makefile: add prove and coverage-prove targets\n\n Makefile | 7 +++++++\n 1 file changed, 7 insertions(+)\n\n\nbase-commit: 0d0ac3826a3bbb9247e39e12623bbcfdd722f24c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-114%2Fderrickstolee%2Fcoverage-prove-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-114/derrickstolee/coverage-prove-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/114\n-- \ngitgitgadget\n"},{"id":"367986","messageId":"294187c6968eff952e78bcea808c66fbedbf1f90.1548773766.git.gitgitgadget@gmail.com","threadId":"50349","inReplyTo":"pull.114.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-01-29T14:56:08Z","receivedAt":"2019-01-29T14:56:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nWhen running the test suite for code coverage using\n'make coverage-test', a single test failure stops the\ntest suite from completing. This leads to significant\nundercounting of covered blocks.\n\nAdd two new targets to the Makefile:\n\n* 'prove' runs the test suite using 'prove'.\n\n* 'coverage-prove' compiles the source using the\n  coverage flags, then runs the test suite using\n  'prove'.\n\nThese targets are modeled after the 'test' and\n'coverage-test' targets.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n Makefile | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 1a44c811aa..ec886635ae 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2665,6 +2665,9 @@ export TEST_NO_MALLOC_CHECK\n test: all\n \t$(MAKE) -C t/ all\n \n+prove: all\n+\t$(MAKE) -C t/ prove\n+\n perf: all\n \t$(MAKE) -C t/perf/ all\n \n@@ -3077,6 +3080,10 @@ coverage-test: coverage-clean-results coverage-compile\n \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n \t\tDEFAULT_TEST_TARGET=test -j1 test\n \n+coverage-prove: coverage-clean-results coverage-compile\n+\t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n+\t\tDEFAULT_TEST_TARGET=prove -j1 prove\n+\n coverage-report:\n \t$(QUIET_GCOV)for dir in $(object_dirs); do \\\n \t\t$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \\\n-- \ngitgitgadget\n"},{"id":"368004","messageId":"nycvar.QRO.7.76.6.1901291616370.41@tvgsbejvaqbjf.bet","threadId":"50349","inReplyTo":"294187c6968eff952e78bcea808c66fbedbf1f90.1548773766.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-29T15:20:03Z","receivedAt":"2019-01-29T15:20:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stolee,\n\nOn Tue, 29 Jan 2019, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n> \n> When running the test suite for code coverage using\n> 'make coverage-test', a single test failure stops the\n> test suite from completing. This leads to significant\n> undercounting of covered blocks.\n> \n> Add two new targets to the Makefile:\n> \n> * 'prove' runs the test suite using 'prove'.\n> \n> * 'coverage-prove' compiles the source using the\n>   coverage flags, then runs the test suite using\n>   'prove'.\n> \n> These targets are modeled after the 'test' and\n> 'coverage-test' targets.\n\nThe rationale, and the diff (after reading what `test` and `coverage-test`\ndo), make a lot of sense to me.\n\nThanks,\nDscho\n\n> \n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  Makefile | 7 +++++++\n>  1 file changed, 7 insertions(+)\n> \n> diff --git a/Makefile b/Makefile\n> index 1a44c811aa..ec886635ae 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2665,6 +2665,9 @@ export TEST_NO_MALLOC_CHECK\n>  test: all\n>  \t$(MAKE) -C t/ all\n>  \n> +prove: all\n> +\t$(MAKE) -C t/ prove\n> +\n>  perf: all\n>  \t$(MAKE) -C t/perf/ all\n>  \n> @@ -3077,6 +3080,10 @@ coverage-test: coverage-clean-results coverage-compile\n>  \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n>  \t\tDEFAULT_TEST_TARGET=test -j1 test\n>  \n> +coverage-prove: coverage-clean-results coverage-compile\n> +\t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n> +\t\tDEFAULT_TEST_TARGET=prove -j1 prove\n> +\n>  coverage-report:\n>  \t$(QUIET_GCOV)for dir in $(object_dirs); do \\\n>  \t\t$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \\\n> -- \n> gitgitgadget\n> \n"},{"id":"368009","messageId":"20190129155827.GC13764@szeder.dev","threadId":"50349","inReplyTo":"294187c6968eff952e78bcea808c66fbedbf1f90.1548773766.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-29T15:58:27Z","receivedAt":"2019-01-29T15:58:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:\n> When running the test suite for code coverage using\n> 'make coverage-test', a single test failure stops the\n> test suite from completing. This leads to significant\n> undercounting of covered blocks.\n> \n> Add two new targets to the Makefile:\n> \n> * 'prove' runs the test suite using 'prove'.\n> \n> * 'coverage-prove' compiles the source using the\n>   coverage flags, then runs the test suite using\n>   'prove'.\n> \n> These targets are modeled after the 'test' and\n> 'coverage-test' targets.\n\nI think the cover letter would be a better commit message.\n\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  Makefile | 7 +++++++\n>  1 file changed, 7 insertions(+)\n> \n> diff --git a/Makefile b/Makefile\n> index 1a44c811aa..ec886635ae 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2665,6 +2665,9 @@ export TEST_NO_MALLOC_CHECK\n>  test: all\n>  \t$(MAKE) -C t/ all\n>  \n> +prove: all\n> +\t$(MAKE) -C t/ prove\n> +\n\nYou don't need this 'prove' target in the \"main\" Makefile, because\n'make test' will run the test suite using DEFAULT_TEST_TARGET anyway.\n\n>  perf: all\n>  \t$(MAKE) -C t/perf/ all\n>  \n> @@ -3077,6 +3080,10 @@ coverage-test: coverage-clean-results coverage-compile\n>  \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n>  \t\tDEFAULT_TEST_TARGET=test -j1 test\n>  \n> +coverage-prove: coverage-clean-results coverage-compile\n> +\t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n> +\t\tDEFAULT_TEST_TARGET=prove -j1 prove\n\nFirst I was wondering why do you need a dedicated 'coverage-prove'\ntarget, instead of letting DEFAULT_TEST_TARGET from the environment or\nfrom 'config.mak' do its thing.  But then I noticed in the hunk\ncontext, that, for some reason, the 'coverage-test' target hardcoded\n'DEFAULT_TEST_TARGET=test -j1'.  Then I was wondering why would it\nwant to do that, and stumbled upon commit c14cc77c11:\n\n    coverage: set DEFAULT_TEST_TARGET to avoid using prove\n    \n    If the user sets DEFAULT_TEST_TARGET=prove in his config.mak, that\n    carries over into the coverage tests.  Which is really bad if he also\n    sets GIT_PROVE_OPTS=-j<..> as that completely breaks the coverage\n    runs.\n    \n    Instead of attempting to mess with the GIT_PROVE_OPTS, just force the\n    test target to 'test' so that we run under make, like we intended all\n    along.\n\nI'm afraid that this issue would badly affect 'coverage-prove' as well\n(I didn't try).  Or if doesn't (anymore?), then that should be\nmentioned in the commit message, and then perhaps it's time to remove\nthat '-j1' from the 'coverage-test' target as well.\n\n\n"},{"id":"368010","messageId":"20190129160030.GA7083@sigill.intra.peff.net","threadId":"50349","inReplyTo":"294187c6968eff952e78bcea808c66fbedbf1f90.1548773766.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-29T16:00:30Z","receivedAt":"2019-01-29T16:00:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n> \n> When running the test suite for code coverage using\n> 'make coverage-test', a single test failure stops the\n> test suite from completing. This leads to significant\n> undercounting of covered blocks.\n> \n> Add two new targets to the Makefile:\n> \n> * 'prove' runs the test suite using 'prove'.\n> \n> * 'coverage-prove' compiles the source using the\n>   coverage flags, then runs the test suite using\n>   'prove'.\n> \n> These targets are modeled after the 'test' and\n> 'coverage-test' targets.\n\nI think these are reasonable to have (and I personally much prefer\n\"prove\" to the raw \"make test\" output anyway).\n\nFor people who don't have \"prove\" available, I think they could just do\n\"make -k test\" to make sure the full suite runs. Should we perhaps be\ndoing that automatically in the sub-make run by coverage-test?\n\n> @@ -3077,6 +3080,10 @@ coverage-test: coverage-clean-results coverage-compile\n>  \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n>  \t\tDEFAULT_TEST_TARGET=test -j1 test\n>  \n> +coverage-prove: coverage-clean-results coverage-compile\n> +\t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n> +\t\tDEFAULT_TEST_TARGET=prove -j1 prove\n> +\n\nYou probably don't need to override DEFAULT_TEST_TARGET here, since the\n\"prove\" target doesn't look at it. Likewise, \"-j1\" probably does nothing\nhere, since prove itself is a single target.\n\nI'm not sure why we want to enforce -j1 for these targets, but if it's\nimportant to do so for the prove case, as well, you'd need to add it to\nGIT_PROVE_OPTS.\n\n-Peff\n"},{"id":"368011","messageId":"bb17112d-2869-741f-de13-3971a995032e@gmail.com","threadId":"50349","inReplyTo":"20190129160030.GA7083@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-01-29T16:35:41Z","receivedAt":"2019-01-29T16:35:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/29/2019 11:00 AM, Jeff King wrote:\n> On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee <dstolee@microsoft.com>\n>>\n>> When running the test suite for code coverage using\n>> 'make coverage-test', a single test failure stops the\n>> test suite from completing. This leads to significant\n>> undercounting of covered blocks.\n>>\n>> Add two new targets to the Makefile:\n>>\n>> * 'prove' runs the test suite using 'prove'.\n>>\n>> * 'coverage-prove' compiles the source using the\n>>   coverage flags, then runs the test suite using\n>>   'prove'.\n>>\n>> These targets are modeled after the 'test' and\n>> 'coverage-test' targets.\n> \n> I think these are reasonable to have (and I personally much prefer\n> \"prove\" to the raw \"make test\" output anyway).\n> \n> For people who don't have \"prove\" available, I think they could just do\n> \"make -k test\" to make sure the full suite runs. Should we perhaps be\n> doing that automatically in the sub-make run by coverage-test?\n\nI wanted to avoid changing the existing behavior, if I could. But, if\nwe can reasonably assume that anyone running 'make coverage-test' wants\nto run the full suite even with failures, then that's fine by me.\n\nI see from the make docs that '-k' will still result in an error code\nat the end of the command, so no automation would result in an incorrect\nresponse to a failed test. Am I correct?\n\n>> @@ -3077,6 +3080,10 @@ coverage-test: coverage-clean-results coverage-compile\n>>  \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n>>  \t\tDEFAULT_TEST_TARGET=test -j1 test\n>>  \n>> +coverage-prove: coverage-clean-results coverage-compile\n>> +\t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n>> +\t\tDEFAULT_TEST_TARGET=prove -j1 prove\n>> +\n> \n> You probably don't need to override DEFAULT_TEST_TARGET here, since the\n> \"prove\" target doesn't look at it. Likewise, \"-j1\" probably does nothing\n> here, since prove itself is a single target.\n\nAs Szeder mentioned, I can probably just drop the 'prove' target and use\nDEFAULT_TEST_TARGET instead. Or do we think anyone will want to use\n'make prove' from root?\n\n> I'm not sure why we want to enforce -j1 for these targets, but if it's\n> important to do so for the prove case, as well, you'd need to add it to\n> GIT_PROVE_OPTS.\n\nThe '-j1' is necessary because the coverage data is collected in a way that\nis not thread-safe. Our compile options also force single-threaded behavior.\n\nI'll specifically override GIT_PROVE_OPTS here to force -j1, but also send\n-j1 to the 'make' command, too.\n\nThanks,\n-Stolee\n"},{"id":"368012","messageId":"a5ca8985-f877-b250-d186-32bef40092f5@gmail.com","threadId":"50349","inReplyTo":"20190129155827.GC13764@szeder.dev","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-01-29T16:37:34Z","receivedAt":"2019-01-29T16:37:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/29/2019 10:58 AM, SZEDER Gábor wrote:\n> On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:\n>> +prove: all\n>> +\t$(MAKE) -C t/ prove\n>> +\n> \n> You don't need this 'prove' target in the \"main\" Makefile, because\n> 'make test' will run the test suite using DEFAULT_TEST_TARGET anyway.\n\nThanks!\n\n>> +coverage-prove: coverage-clean-results coverage-compile\n>> +\t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n>> +\t\tDEFAULT_TEST_TARGET=prove -j1 prove\n> \n> First I was wondering why do you need a dedicated 'coverage-prove'\n> target, instead of letting DEFAULT_TEST_TARGET from the environment or\n> from 'config.mak' do its thing.  But then I noticed in the hunk\n> context, that, for some reason, the 'coverage-test' target hardcoded\n> 'DEFAULT_TEST_TARGET=test -j1'.  Then I was wondering why would it\n> want to do that, and stumbled upon commit c14cc77c11:\n> \n>     coverage: set DEFAULT_TEST_TARGET to avoid using prove\n>     \n>     If the user sets DEFAULT_TEST_TARGET=prove in his config.mak, that\n>     carries over into the coverage tests.  Which is really bad if he also\n>     sets GIT_PROVE_OPTS=-j<..> as that completely breaks the coverage\n>     runs.\n>     \n>     Instead of attempting to mess with the GIT_PROVE_OPTS, just force the\n>     test target to 'test' so that we run under make, like we intended all\n>     along.\n\nThanks for finding this!\n \n> I'm afraid that this issue would badly affect 'coverage-prove' as well\n> (I didn't try).  Or if doesn't (anymore?), then that should be\n> mentioned in the commit message, and then perhaps it's time to remove\n> that '-j1' from the 'coverage-test' target as well.\n\nI'll fix this by forcing an update to GIT_PROVE_OPTS. It does limit our\nability to use GIT_PROVE_OPTS as a pass-through, but at least this new\ntarget will have that assumption built in.\n\nThanks,\n-Stolee\n"},{"id":"368013","messageId":"20190129164654.GA6991@sigill.intra.peff.net","threadId":"50349","inReplyTo":"bb17112d-2869-741f-de13-3971a995032e@gmail.com","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-29T16:46:54Z","receivedAt":"2019-01-29T16:46:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 29, 2019 at 11:35:41AM -0500, Derrick Stolee wrote:\n\n> > For people who don't have \"prove\" available, I think they could just do\n> > \"make -k test\" to make sure the full suite runs. Should we perhaps be\n> > doing that automatically in the sub-make run by coverage-test?\n> \n> I wanted to avoid changing the existing behavior, if I could. But, if\n> we can reasonably assume that anyone running 'make coverage-test' wants\n> to run the full suite even with failures, then that's fine by me.\n\nAnother option would be to relay \"-k\" from the caller. I think it's not\nenough to just use $(MAKE), but if you use $(MAKE) $(MAKEFLAGS), then\nrunning \"make -k coverage-test\" from your coverage script would (I\nthink) do what you want.\n\n> I see from the make docs that '-k' will still result in an error code\n> at the end of the command, so no automation would result in an incorrect\n> response to a failed test. Am I correct?\n\nYeah, that matches my understanding. I don't think you'd have to deal\nwith that failure code manually for coverage-report because it does not\ndepend on coverage-test (but obviously if you did \"make coverage-test &&\nmake coverage-report\", the \"&&\" needs to become a semicolon).\n\n> >> +coverage-prove: coverage-clean-results coverage-compile\n> >> +\t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n> >> +\t\tDEFAULT_TEST_TARGET=prove -j1 prove\n> >> +\n> > \n> > You probably don't need to override DEFAULT_TEST_TARGET here, since the\n> > \"prove\" target doesn't look at it. Likewise, \"-j1\" probably does nothing\n> > here, since prove itself is a single target.\n> \n> As Szeder mentioned, I can probably just drop the 'prove' target and use\n> DEFAULT_TEST_TARGET instead. Or do we think anyone will want to use\n> 'make prove' from root?\n\nYeah, that works. I typically do run prove, and I do run the tests from\nthe root, but DEFAULT_TEST_TARGET already makes it all work for me. So\nyeah, I think it's fine to leave off the prove target until somebody\nactually wants it.\n\n> > I'm not sure why we want to enforce -j1 for these targets, but if it's\n> > important to do so for the prove case, as well, you'd need to add it to\n> > GIT_PROVE_OPTS.\n> \n> The '-j1' is necessary because the coverage data is collected in a way that\n> is not thread-safe. Our compile options also force single-threaded behavior.\n\nAh, right, I vaguely recall that now.\n\n> I'll specifically override GIT_PROVE_OPTS here to force -j1, but also send\n> -j1 to the 'make' command, too.\n\nMakes sense.\n\n-Peff\n"},{"id":"368014","messageId":"20190129164947.GB6991@sigill.intra.peff.net","threadId":"50349","inReplyTo":"a5ca8985-f877-b250-d186-32bef40092f5@gmail.com","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-29T16:49:48Z","receivedAt":"2019-01-29T16:49:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 29, 2019 at 11:37:34AM -0500, Derrick Stolee wrote:\n\n> > I'm afraid that this issue would badly affect 'coverage-prove' as well\n> > (I didn't try).  Or if doesn't (anymore?), then that should be\n> > mentioned in the commit message, and then perhaps it's time to remove\n> > that '-j1' from the 'coverage-test' target as well.\n> \n> I'll fix this by forcing an update to GIT_PROVE_OPTS. It does limit our\n> ability to use GIT_PROVE_OPTS as a pass-through, but at least this new\n> target will have that assumption built in.\n\nI think doing:\n\n  make GIT_PROVE_OPTS=\"$(GIT_PROVE_OPTS) -j1\"\n\nshould be sufficient to pass through most of the options but override\n-j, since prove does the usual left-to-right \"last one wins\" behavior\nfor its options.\n\n-Peff\n"},{"id":"368017","messageId":"pull.114.v2.git.gitgitgadget@gmail.com","threadId":"50349","inReplyTo":"pull.114.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] Makefile: add prove and coverage-prove targets","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-01-29T17:05:17Z","receivedAt":"2019-01-29T17:05:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Sometimes there are test failures in the 'pu' branch. This is somewhat\nexpected for a branch that takes the very latest topics under development,\nand those sometimes have semantic conflicts that only show up during test\nruns. This also can happen when running the test suite with different\nGIT_TEST_* environment variables that interact in unexpected ways.\n\nThis causes a problem for the test coverage reports, as the typical 'make\ncoverage-test coverage-report' run halts at the first failed test. If that\ntest is early in the suite, then many valuable tests are not exercising the\ncode and the coverage report becomes noisy with false positives.\n\nThis patch adds two targets to the Makefile: 'prove' and 'coverage-prove'.\nThe idea is to use the 'prove' tool to run the test suite, as it will track\nfailed tests but continue running the full suite even with a failure.\n\nIf/when this merges down, I will use this new target for the automation\naround the test coverage reports.\n\nUpdates in V2:\n\n * Dropped the 'prove' target\n   \n   \n * Append '-j1' to GIT_PROVE_OPTS\n   \n   \n * Commit message tweaks.\n\nDerrick Stolee (1):\n  Makefile: add coverage-prove target\n\n Makefile | 5 +++++\n 1 file changed, 5 insertions(+)\n\n\nbase-commit: 0d0ac3826a3bbb9247e39e12623bbcfdd722f24c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-114%2Fderrickstolee%2Fcoverage-prove-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-114/derrickstolee/coverage-prove-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/114\n\nRange-diff vs v1:\n\n 1:  294187c696 < -:  ---------- Makefile: add prove and coverage-prove targets\n -:  ---------- > 1:  af810fad97 Makefile: add coverage-prove target\n\n-- \ngitgitgadget\n"},{"id":"368018","messageId":"af810fad9765a09d598981b4244945bc74404609.1548781516.git.gitgitgadget@gmail.com","threadId":"50349","inReplyTo":"pull.114.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] Makefile: add coverage-prove target","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-01-29T17:05:18Z","receivedAt":"2019-01-29T17:05:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nSometimes there are test failures in the 'pu' branch. This\nis somewhat expected for a branch that takes the very latest\ntopics under development, and those sometimes have semantic\nconflicts that only show up during test runs. This also can\nhappen when running the test suite with different GIT_TEST_*\nenvironment variables that interact in unexpected ways\n\nThis causes a problem for the test coverage reports, as\nthe typical 'make coverage-test coverage-report' run halts\nat the first failed test. If that test is early in the\nsuite, then many valuable tests are not exercising the code\nand the coverage report becomes noisy with false positives.\n\nAdd a new 'coverage-prove' target to the Makefile,\nmodeled after the 'coverage-test' target. This compiles\nthe source using the coverage flags, then runs the test\nsuite using the 'prove' tool. Since the coverage\nmachinery is not thread-safe, enforce that the tests\nare run in sequence by appending '-j1' to GIT_PROVE_OPTS.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n Makefile | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 1a44c811aa..23d8730482 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3077,6 +3077,11 @@ coverage-test: coverage-clean-results coverage-compile\n \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n \t\tDEFAULT_TEST_TARGET=test -j1 test\n \n+coverage-prove: coverage-clean-results coverage-compile\n+\t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n+\t\tDEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS=\"$(GIT_PROVE_OPTS) -j1\" \\\n+\t\t-j1 test\n+\n coverage-report:\n \t$(QUIET_GCOV)for dir in $(object_dirs); do \\\n \t\t$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \\\n-- \ngitgitgadget\n"},{"id":"368019","messageId":"20190129173409.GD13764@szeder.dev","threadId":"50349","inReplyTo":"20190129155827.GC13764@szeder.dev","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-29T17:34:09Z","receivedAt":"2019-01-29T17:34:15Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jan 29, 2019 at 04:58:27PM +0100, SZEDER Gábor wrote:\n> On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:\n> > @@ -3077,6 +3080,10 @@ coverage-test: coverage-clean-results coverage-compile\n> >  \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n> >  \t\tDEFAULT_TEST_TARGET=test -j1 test\n> >  \n> > +coverage-prove: coverage-clean-results coverage-compile\n> > +\t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n> > +\t\tDEFAULT_TEST_TARGET=prove -j1 prove\n> \n> First I was wondering why do you need a dedicated 'coverage-prove'\n> target, instead of letting DEFAULT_TEST_TARGET from the environment or\n> from 'config.mak' do its thing.  But then I noticed in the hunk\n> context, that, for some reason, the 'coverage-test' target hardcoded\n> 'DEFAULT_TEST_TARGET=test -j1'.  Then I was wondering why would it\n> want to do that, and stumbled upon commit c14cc77c11:\n> \n>     coverage: set DEFAULT_TEST_TARGET to avoid using prove\n>     \n>     If the user sets DEFAULT_TEST_TARGET=prove in his config.mak, that\n>     carries over into the coverage tests.  Which is really bad if he also\n>     sets GIT_PROVE_OPTS=-j<..> as that completely breaks the coverage\n>     runs.\n\nSo this text is really dramatic and implies (to me, at least), that\nparallelized coverage builds just don't work.  I've just run a\ncoverage build with this patch and my usual GIT_PROVE_OPTS containing\n'-j4', and the tests went well and the generated report looks good,\ntoo (I don't know how it's supposed to look, but at least I didn't\nnotice anything obviously wrong with it).  However, this might mean\nnothing, because further digging turned up the follow paragraph in\n901c369af5 (Support coverage testing with GCC/gcov, 2009-02-19):\n\n    The tests are run serially (with -j1).  The coverage code should\n    theoretically allow concurrent access to its data files, but the\n    author saw random test failures.  Obviously this could be\n    improved.\n\nAnd in the related email discussion [1]:\n\n  But even though the docs claim it [-j<N>] should be possible,\n  I've been getting \"random\" test failures when compiled with coverage\n  support, that went away with -j1.  So the tests still run with -j1, as\n  with the first version of the series.\n\nSo it doesn't seem to be that bad after all, because it's not\n\"completely breaks\" but \"random test failures\".  Still far from ideal,\nbut the original coverage patch is just about 3 weeks short of a\ndecade old, so maybe things have improved since then, and it'd be\nworth a try to leave GIT_PROVE_OPTS as is and see what happens.\n\n\n[1] https://public-inbox.org/git/200902191512.16755.trast@student.ethz.ch/\n\n\n"},{"id":"368026","messageId":"95d30af1-1940-b4db-99e8-2ccca5210bbd@gmail.com","threadId":"50349","inReplyTo":"20190129173409.GD13764@szeder.dev","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-01-29T18:10:33Z","receivedAt":"2019-01-29T18:10:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/29/2019 12:34 PM, SZEDER Gábor wrote:\n> On Tue, Jan 29, 2019 at 04:58:27PM +0100, SZEDER Gábor wrote:\n> And in the related email discussion [1]:\n> \n>   But even though the docs claim it [-j<N>] should be possible,\n>   I've been getting \"random\" test failures when compiled with coverage\n>   support, that went away with -j1.  So the tests still run with -j1, as\n>   with the first version of the series.\n> \n> So it doesn't seem to be that bad after all, because it's not\n> \"completely breaks\" but \"random test failures\".  Still far from ideal,\n> but the original coverage patch is just about 3 weeks short of a\n> decade old, so maybe things have improved since then, and it'd be\n> worth a try to leave GIT_PROVE_OPTS as is and see what happens.\n\nIt would certainly be nice if the build time could be reduced through\nparallel test runs. I've kicked off a build using GIT_PROVE_OPTS=\"-j12\"\nto see what happens.\n\nThanks!\n-Stolee\n"},{"id":"368043","messageId":"795f816d-d870-068a-5595-2fe617339a79@gmail.com","threadId":"50349","inReplyTo":"95d30af1-1940-b4db-99e8-2ccca5210bbd@gmail.com","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-01-29T20:49:58Z","receivedAt":"2019-01-29T20:50:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/29/2019 1:10 PM, Derrick Stolee wrote:\n> On 1/29/2019 12:34 PM, SZEDER Gábor wrote:\n>> On Tue, Jan 29, 2019 at 04:58:27PM +0100, SZEDER Gábor wrote:\n>> And in the related email discussion [1]:\n>>\n>>   But even though the docs claim it [-j<N>] should be possible,\n>>   I've been getting \"random\" test failures when compiled with coverage\n>>   support, that went away with -j1.  So the tests still run with -j1, as\n>>   with the first version of the series.\n>>\n>> So it doesn't seem to be that bad after all, because it's not\n>> \"completely breaks\" but \"random test failures\".  Still far from ideal,\n>> but the original coverage patch is just about 3 weeks short of a\n>> decade old, so maybe things have improved since then, and it'd be\n>> worth a try to leave GIT_PROVE_OPTS as is and see what happens.\n> \n> It would certainly be nice if the build time could be reduced through\n> parallel test runs. I've kicked off a build using GIT_PROVE_OPTS=\"-j12\"\n> to see what happens.\n\nI did get a failed test with this run:\n\nt0025-crlf-renormalize.sh                 (Wstat: 256 Tests: 3 Failed: 1)\n  Failed test:  2\n  Non-zero exit status: 1\n\nThis was on the 'jch' branch, and an equivalent build with sequential\nexecution did not have this failure. That's flaky enough for me to stick\nto sequential runs.\n\nThanks,\n-Stolee\n \n"},{"id":"368045","messageId":"87zhrj2n2l.fsf@evledraar.gmail.com","threadId":"50349","inReplyTo":"20190129160030.GA7083@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-01-29T21:03:46Z","receivedAt":"2019-01-29T21:03:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 29 2019, Jeff King wrote:\n\n> On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:\n>\n>> From: Derrick Stolee <dstolee@microsoft.com>\n>>\n>> When running the test suite for code coverage using\n>> 'make coverage-test', a single test failure stops the\n>> test suite from completing. This leads to significant\n>> undercounting of covered blocks.\n>>\n>> Add two new targets to the Makefile:\n>>\n>> * 'prove' runs the test suite using 'prove'.\n>>\n>> * 'coverage-prove' compiles the source using the\n>>   coverage flags, then runs the test suite using\n>>   'prove'.\n>>\n>> These targets are modeled after the 'test' and\n>> 'coverage-test' targets.\n>\n> I think these are reasonable to have (and I personally much prefer\n> \"prove\" to the raw \"make test\" output anyway).\n\nI wonder if anyone would mind if we removed the non-prove path.\n\nWhen I added it in 5099b99d25 (\"test-lib: Adjust output to be valid TAP\nformat\", 2010-06-24) there were still some commonly shipped OS's that\nhad a crappy old \"prove\", but now almost a decade later that's not a\npractical problem, and it's installed by default with perl, and we\nalready depend on perl for the tests.\n\nI don't feel strongly about it, but it would allow us to prune some\nlogin in the test library / Makefile.\n\nMaybe something for a show of hands at the contributor summit?\n"},{"id":"368054","messageId":"20190129215827.GF13764@szeder.dev","threadId":"50349","inReplyTo":"795f816d-d870-068a-5595-2fe617339a79@gmail.com","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-29T21:58:27Z","receivedAt":"2019-01-29T21:58:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jan 29, 2019 at 03:49:58PM -0500, Derrick Stolee wrote:\n> On 1/29/2019 1:10 PM, Derrick Stolee wrote:\n> > On 1/29/2019 12:34 PM, SZEDER Gábor wrote:\n> >> On Tue, Jan 29, 2019 at 04:58:27PM +0100, SZEDER Gábor wrote:\n> >> And in the related email discussion [1]:\n> >>\n> >>   But even though the docs claim it [-j<N>] should be possible,\n> >>   I've been getting \"random\" test failures when compiled with coverage\n> >>   support, that went away with -j1.  So the tests still run with -j1, as\n> >>   with the first version of the series.\n> >>\n> >> So it doesn't seem to be that bad after all, because it's not\n> >> \"completely breaks\" but \"random test failures\".  Still far from ideal,\n> >> but the original coverage patch is just about 3 weeks short of a\n> >> decade old, so maybe things have improved since then, and it'd be\n> >> worth a try to leave GIT_PROVE_OPTS as is and see what happens.\n> > \n> > It would certainly be nice if the build time could be reduced through\n> > parallel test runs. I've kicked off a build using GIT_PROVE_OPTS=\"-j12\"\n> > to see what happens.\n> \n> I did get a failed test with this run:\n> \n> t0025-crlf-renormalize.sh                 (Wstat: 256 Tests: 3 Failed: 1)\n>   Failed test:  2\n>   Non-zero exit status: 1\n> \n> This was on the 'jch' branch, and an equivalent build with sequential\n> execution did not have this failure. That's flaky enough for me to stick\n> to sequential runs.\n\nThat failure is not coverage-related, but as it turned out 9e5da3d055\n(add: use separate ADD_CACHE_RENORMALIZE flag, 2019-01-17) made t0025\nrather flaky:\n\n  https://public-inbox.org/git/20190129213533.GE13764@szeder.dev/\n\nWhen reading those old commit messages and discussions in the\nafternoon, I was wondering what \"random test failures\" actually meant,\nsince it was not stated explicitly that it was coverage-related.  For\nall we know it could have been \"general\" test flakiness that happened\nto manifest under the higher load of a parallel test run.\n\n"},{"id":"368059","messageId":"20190129223807.GC1895@sigill.intra.peff.net","threadId":"50349","inReplyTo":"87zhrj2n2l.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-29T22:38:07Z","receivedAt":"2019-01-29T22:38:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 29, 2019 at 10:03:46PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > I think these are reasonable to have (and I personally much prefer\n> > \"prove\" to the raw \"make test\" output anyway).\n> \n> I wonder if anyone would mind if we removed the non-prove path.\n> \n> When I added it in 5099b99d25 (\"test-lib: Adjust output to be valid TAP\n> format\", 2010-06-24) there were still some commonly shipped OS's that\n> had a crappy old \"prove\", but now almost a decade later that's not a\n> practical problem, and it's installed by default with perl, and we\n> already depend on perl for the tests.\n> \n> I don't feel strongly about it, but it would allow us to prune some\n> login in the test library / Makefile.\n> \n> Maybe something for a show of hands at the contributor summit?\n\nCertainly no argument from me personally, but I do wonder if anybody\nuses the old one. Maybe we could mark it with a deprecation notice or\nsomething. I'd be happy to do a straw poll at the contributor summit (or\non the list, if anybody reads this ;) ).\n\n-Peff\n"},{"id":"368121","messageId":"nycvar.QRO.7.76.6.1901301317120.41@tvgsbejvaqbjf.bet","threadId":"50349","inReplyTo":"87zhrj2n2l.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-30T12:20:14Z","receivedAt":"2019-01-30T12:20:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Tue, 29 Jan 2019, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Jan 29 2019, Jeff King wrote:\n> \n> > On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via\n> > GitGitGadget wrote:\n> >\n> >> From: Derrick Stolee <dstolee@microsoft.com>\n> >>\n> >> When running the test suite for code coverage using\n> >> 'make coverage-test', a single test failure stops the\n> >> test suite from completing. This leads to significant\n> >> undercounting of covered blocks.\n> >>\n> >> Add two new targets to the Makefile:\n> >>\n> >> * 'prove' runs the test suite using 'prove'.\n> >>\n> >> * 'coverage-prove' compiles the source using the\n> >>   coverage flags, then runs the test suite using\n> >>   'prove'.\n> >>\n> >> These targets are modeled after the 'test' and\n> >> 'coverage-test' targets.\n> >\n> > I think these are reasonable to have (and I personally much prefer\n> > \"prove\" to the raw \"make test\" output anyway).\n> \n> I wonder if anyone would mind if we removed the non-prove path.\n> \n> When I added it in 5099b99d25 (\"test-lib: Adjust output to be valid TAP\n> format\", 2010-06-24) there were still some commonly shipped OS's that\n> had a crappy old \"prove\", but now almost a decade later that's not a\n> practical problem, and it's installed by default with perl, and we\n> already depend on perl for the tests.\n\nIt's not only about crappy old `prove`, it is also about requiring Perl\n(and remember, Perl is not really native in Git for Windows' case; I still\nhave a hunch that we could save on time *dramatically* by simply running\nthrough regular `make` rather than through `prove`).\n\nI did start to implement a parallel test runner for use with BusyBox-based\nMinGit, but dropped the ball on that front before I could satisfy myself\nthat this is robust enough. Once it *is* robust enough, we could even\nreplace the entire `prove` support with a native, test-tool driven test\nframework.\n\n> I don't feel strongly about it, but it would allow us to prune some\n> login in the test library / Makefile.\n> \n> Maybe something for a show of hands at the contributor summit?\n\nSure, let's put it up for discussion.\n\nCiao,\nDscho"},{"id":"368130","messageId":"87y3722sz7.fsf@evledraar.gmail.com","threadId":"50349","inReplyTo":"nycvar.QRO.7.76.6.1901301317120.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-01-30T13:08:28Z","receivedAt":"2019-01-30T13:08:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 30 2019, Johannes Schindelin wrote:\n\n> Hi Ævar,\n>\n> On Tue, 29 Jan 2019, Ævar Arnfjörð Bjarmason wrote:\n>\n>> On Tue, Jan 29 2019, Jeff King wrote:\n>>\n>> > On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via\n>> > GitGitGadget wrote:\n>> >\n>> >> From: Derrick Stolee <dstolee@microsoft.com>\n>> >>\n>> >> When running the test suite for code coverage using\n>> >> 'make coverage-test', a single test failure stops the\n>> >> test suite from completing. This leads to significant\n>> >> undercounting of covered blocks.\n>> >>\n>> >> Add two new targets to the Makefile:\n>> >>\n>> >> * 'prove' runs the test suite using 'prove'.\n>> >>\n>> >> * 'coverage-prove' compiles the source using the\n>> >>   coverage flags, then runs the test suite using\n>> >>   'prove'.\n>> >>\n>> >> These targets are modeled after the 'test' and\n>> >> 'coverage-test' targets.\n>> >\n>> > I think these are reasonable to have (and I personally much prefer\n>> > \"prove\" to the raw \"make test\" output anyway).\n>>\n>> I wonder if anyone would mind if we removed the non-prove path.\n>>\n>> When I added it in 5099b99d25 (\"test-lib: Adjust output to be valid TAP\n>> format\", 2010-06-24) there were still some commonly shipped OS's that\n>> had a crappy old \"prove\", but now almost a decade later that's not a\n>> practical problem, and it's installed by default with perl, and we\n>> already depend on perl for the tests.\n>\n> It's not only about crappy old `prove`, it is also about requiring Perl\n> (and remember, Perl is not really native in Git for Windows' case;\n\nWe require perl now for testing, NO_PERL is just for the installed\nversion of git. If you change the various test-lib.sh and\ntest-lib-functions.sh that unconditionally uses \"perl\" or \"$PERL_PATH\"\nhundreds/thousands (didn't take an exact count, just watched fail scroll\nby) tests fail.\n\nSo my assumption is that anyone running the tests now has perl anyway,\nand thus a further hard dependency on it won't hurt anything.\n\n> I still have a hunch that we could save on time *dramatically* by\n> simply running through regular `make` rather than through `prove`).\n\nMy hunch is that on the OS's where this would matter (e.g. Windows) the\noverhead is mainly spawning the processes, and it doesn't matter if it's\nmake or perl doing the spawning, but I have nothing to back that up...\n\n> I did start to implement a parallel test runner for use with BusyBox-based\n> MinGit, but dropped the ball on that front before I could satisfy myself\n> that this is robust enough. Once it *is* robust enough, we could even\n> replace the entire `prove` support with a native, test-tool driven test\n> framework.\n\nLet's be clear about what \"prove support\" means.\n\nWhen I added support for \"prove\" I was really adding support for TAP\nwhich is a standardized test output protocol: https://testanything.org/\n\nIt just so happens that \"prove\" is the most ubiquitous implementation,\nbut there's plenty of others: https://testanything.org/consumers.html\n\nSo hard-depending on \"prove\" in no way ties us to that particular tool\nforever. We'd just do away with ferrying information via a side-channel\n(*.counts files) that we also get from its output.\n\n>> I don't feel strongly about it, but it would allow us to prune some\n>> login in the test library / Makefile.\n>>\n>> Maybe something for a show of hands at the contributor summit?\n>\n> Sure, let's put it up for discussion.\n\n*Nod*\n"},{"id":"368148","messageId":"nycvar.QRO.7.76.6.1901301935010.41@tvgsbejvaqbjf.bet","threadId":"50349","inReplyTo":"87y3722sz7.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-30T18:42:42Z","receivedAt":"2019-01-30T18:43:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Wed, 30 Jan 2019, Ævar Arnfjörð Bjarmason wrote:\n\n> On Wed, Jan 30 2019, Johannes Schindelin wrote:\n> \n> > On Tue, 29 Jan 2019, Ævar Arnfjörð Bjarmason wrote:\n> >\n> >> On Tue, Jan 29 2019, Jeff King wrote:\n> >>\n> >> > On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via\n> >> > GitGitGadget wrote:\n> >> >\n> >> >> From: Derrick Stolee <dstolee@microsoft.com>\n> >> >>\n> >> >> When running the test suite for code coverage using\n> >> >> 'make coverage-test', a single test failure stops the\n> >> >> test suite from completing. This leads to significant\n> >> >> undercounting of covered blocks.\n> >> >>\n> >> >> Add two new targets to the Makefile:\n> >> >>\n> >> >> * 'prove' runs the test suite using 'prove'.\n> >> >>\n> >> >> * 'coverage-prove' compiles the source using the\n> >> >>   coverage flags, then runs the test suite using\n> >> >>   'prove'.\n> >> >>\n> >> >> These targets are modeled after the 'test' and\n> >> >> 'coverage-test' targets.\n> >> >\n> >> > I think these are reasonable to have (and I personally much prefer\n> >> > \"prove\" to the raw \"make test\" output anyway).\n> >>\n> >> I wonder if anyone would mind if we removed the non-prove path.\n> >>\n> >> When I added it in 5099b99d25 (\"test-lib: Adjust output to be valid TAP\n> >> format\", 2010-06-24) there were still some commonly shipped OS's that\n> >> had a crappy old \"prove\", but now almost a decade later that's not a\n> >> practical problem, and it's installed by default with perl, and we\n> >> already depend on perl for the tests.\n> >\n> > It's not only about crappy old `prove`, it is also about requiring Perl\n> > (and remember, Perl is not really native in Git for Windows' case;\n> \n> We require perl now for testing, NO_PERL is just for the installed\n> version of git.\n\nWhich is confusing, if you want to put it nicely.\n\n> If you change the various test-lib.sh and test-lib-functions.sh that\n> unconditionally uses \"perl\" or \"$PERL_PATH\" hundreds/thousands (didn't\n> take an exact count, just watched fail scroll by) tests fail.\n\nI know. Oh boy, I know.\n\nBut we do not have to keep that status quo, nor do we have to make it\nworse.\n\nIt would not surprise me in the least if we could accelerate our entire\ntest suite by reducing our heavy reliance on scripting (including Perl) to\nthe point that it really takes too little time *not* to run. (Right now,\nif you are on Windows, you better think twice before you start the test\nsuite, it will easily take over 3h (!!!) to run in a regular developer\nsetup. Even on a regular Mac, I would think twice before starting the run\nthat blocks my machine for easily 20 minutes straight. Needless to say\nthat few developers, if any, use it to validate their patches, in\nparticular on Windows. Meaning: for all real purposes, the test suite is\nnearly useless on Windows.)\n\nSo let's not bake *even more* Perl usage into our test suite. Thanks.\n\n> So my assumption is that anyone running the tests now has perl anyway,\n> and thus a further hard dependency on it won't hurt anything.\n\nBy that token, the effort to turn many a script into a built-in for better\nperformance and substantially better error checking would be totally\nnonsensical. \"Because anyone running Git used those scripts anyway, so\nmaking them a hard dependency won't hurt anything\"?\n\nI do not believe even a fraction of a second that that effort is\nnonsensical. Just like I do not believe even a fraction of a second that\nit makes sense for our test suite to rely on scripting so much. Or for us\nto make that reliance even bigger, for that matter.\n\n> > I still have a hunch that we could save on time *dramatically* by\n> > simply running through regular `make` rather than through `prove`).\n> \n> My hunch is that on the OS's where this would matter (e.g. Windows) the\n> overhead is mainly spawning the processes, and it doesn't matter if it's\n> make or perl doing the spawning, but I have nothing to back that up...\n\nI have at least the experience of several thousands runs of the test suite\non Windows, together with a couple dozen hours spent recently *just* on\nmaking the CI of GitGitGadget at least bearable.\n\nSo I do not quite understand why you offered a contrary opinion when you\nhave nothing to back it up.\n\nI mean, I would really like to have an informed discussion with you, to\nbenefit from your skills and from your experience to make the entire\ndesign of our test suite better (there is so much room for improvement, we\nshould really be able to put together our knowledge to enhance it). It\nneeds to be based on facts, of course.\n\nCiao,\nDscho"},{"id":"368160","messageId":"87womm2b7r.fsf@evledraar.gmail.com","threadId":"50349","inReplyTo":"nycvar.QRO.7.76.6.1901301935010.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-01-30T19:32:08Z","receivedAt":"2019-01-30T19:32:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 30 2019, Johannes Schindelin wrote:\n\n> Hi Ævar,\n>\n> On Wed, 30 Jan 2019, Ævar Arnfjörð Bjarmason wrote:\n>\n>> On Wed, Jan 30 2019, Johannes Schindelin wrote:\n>>\n>> > On Tue, 29 Jan 2019, Ævar Arnfjörð Bjarmason wrote:\n>> >\n>> >> On Tue, Jan 29 2019, Jeff King wrote:\n>> >>\n>> >> > On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via\n>> >> > GitGitGadget wrote:\n>> >> >\n>> >> >> From: Derrick Stolee <dstolee@microsoft.com>\n>> >> >>\n>> >> >> When running the test suite for code coverage using\n>> >> >> 'make coverage-test', a single test failure stops the\n>> >> >> test suite from completing. This leads to significant\n>> >> >> undercounting of covered blocks.\n>> >> >>\n>> >> >> Add two new targets to the Makefile:\n>> >> >>\n>> >> >> * 'prove' runs the test suite using 'prove'.\n>> >> >>\n>> >> >> * 'coverage-prove' compiles the source using the\n>> >> >>   coverage flags, then runs the test suite using\n>> >> >>   'prove'.\n>> >> >>\n>> >> >> These targets are modeled after the 'test' and\n>> >> >> 'coverage-test' targets.\n>> >> >\n>> >> > I think these are reasonable to have (and I personally much prefer\n>> >> > \"prove\" to the raw \"make test\" output anyway).\n>> >>\n>> >> I wonder if anyone would mind if we removed the non-prove path.\n>> >>\n>> >> When I added it in 5099b99d25 (\"test-lib: Adjust output to be valid TAP\n>> >> format\", 2010-06-24) there were still some commonly shipped OS's that\n>> >> had a crappy old \"prove\", but now almost a decade later that's not a\n>> >> practical problem, and it's installed by default with perl, and we\n>> >> already depend on perl for the tests.\n>> >\n>> > It's not only about crappy old `prove`, it is also about requiring Perl\n>> > (and remember, Perl is not really native in Git for Windows' case;\n>>\n>> We require perl now for testing, NO_PERL is just for the installed\n>> version of git.\n>\n> Which is confusing, if you want to put it nicely.\n>\n>> If you change the various test-lib.sh and test-lib-functions.sh that\n>> unconditionally uses \"perl\" or \"$PERL_PATH\" hundreds/thousands (didn't\n>> take an exact count, just watched fail scroll by) tests fail.\n>\n> I know. Oh boy, I know.\n>\n> But we do not have to keep that status quo, nor do we have to make it\n> worse.\n>\n> It would not surprise me in the least if we could accelerate our entire\n> test suite by reducing our heavy reliance on scripting (including Perl) to\n> the point that it really takes too little time *not* to run. (Right now,\n> if you are on Windows, you better think twice before you start the test\n> suite, it will easily take over 3h (!!!) to run in a regular developer\n> setup. Even on a regular Mac, I would think twice before starting the run\n> that blocks my machine for easily 20 minutes straight. Needless to say\n> that few developers, if any, use it to validate their patches, in\n> particular on Windows. Meaning: for all real purposes, the test suite is\n> nearly useless on Windows.)\n>\n> So let's not bake *even more* Perl usage into our test suite. Thanks.\n>\n>> So my assumption is that anyone running the tests now has perl anyway,\n>> and thus a further hard dependency on it won't hurt anything.\n>\n> By that token, the effort to turn many a script into a built-in for better\n> performance and substantially better error checking would be totally\n> nonsensical. \"Because anyone running Git used those scripts anyway, so\n> making them a hard dependency won't hurt anything\"?\n>\n> I do not believe even a fraction of a second that that effort is\n> nonsensical. Just like I do not believe even a fraction of a second that\n> it makes sense for our test suite to rely on scripting so much. Or for us\n> to make that reliance even bigger, for that matter.\n>\n>> > I still have a hunch that we could save on time *dramatically* by\n>> > simply running through regular `make` rather than through `prove`).\n>>\n>> My hunch is that on the OS's where this would matter (e.g. Windows) the\n>> overhead is mainly spawning the processes, and it doesn't matter if it's\n>> make or perl doing the spawning, but I have nothing to back that up...\n>\n> I have at least the experience of several thousands runs of the test suite\n> on Windows, together with a couple dozen hours spent recently *just* on\n> making the CI of GitGitGadget at least bearable.\n>\n> So I do not quite understand why you offered a contrary opinion when you\n> have nothing to back it up.\n>\n> I mean, I would really like to have an informed discussion with you, to\n> benefit from your skills and from your experience to make the entire\n> design of our test suite better (there is so much room for improvement, we\n> should really be able to put together our knowledge to enhance it). It\n> needs to be based on facts, of course.\n\nLet's get some numbers then. On master, go to the \"t\" directory and run\nthis:\n\n    for f in t[0-9]*.sh; do (echo '#!/bin/sh' && echo \"echo ok 1 $f\" && echo sleep 1 && echo echo 1..1) >$f; done\n\nThat effectively turns all our tests into a \"hello world\" with a sleep\nof 1 second.\n\nThen run both:\n\n    time prove -j12 t00[0-9]*.sh\n\nAnd:\n\n    time make -j12 t00[0-9]*.sh\n\nFor some value of -j12 and t00[0-9]*.sh. In my testing \"make\" is a bit\nfaster, but not by any amount that would matter when this is run for\nreal.\n"},{"id":"368220","messageId":"nycvar.QRO.7.76.6.1901310819180.41@tvgsbejvaqbjf.bet","threadId":"50349","inReplyTo":"87womm2b7r.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-31T07:23:48Z","receivedAt":"2019-01-31T07:24:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Wed, 30 Jan 2019, Ævar Arnfjörð Bjarmason wrote:\n\n> Let's get some numbers then. On master, go to the \"t\" directory and run\n> this:\n> \n>     for f in t[0-9]*.sh; do (echo '#!/bin/sh' && echo \"echo ok 1 $f\" && echo sleep 1 && echo echo 1..1) >$f; done\n>\n> That effectively turns all our tests into a \"hello world\" with a sleep\n> of 1 second.\n> \n> Then run both:\n> \n>     time prove -j12 t00[0-9]*.sh\n> \n> And:\n> \n>     time make -j12 t00[0-9]*.sh\n> \n> For some value of -j12 and t00[0-9]*.sh. In my testing \"make\" is a bit\n> faster, but not by any amount that would matter when this is run for\n> real.\n\nHmm. You're right, I basically see a range of 5.17-5.27 seconds, all well\nin the noise.\n\nSo apart from the size for Perl (which accounts for about a third of the\nsubset of the Git for Windows SDK we have to download into each and every\nCI run, multiple times) it does not hurt that much at the moment, you're\nright.\n\nStill, I would like to get away from our reliance on Perl in the test\nsuite. It does not make sense to require Perl even with NO_PERL.\n\nCiao,\nDscho"}]}