{"thread":{"id":"40385","subject":"[PATCH/RFC] Add test_repo_expect_success for running tests in a new repository","startedAt":"2015-09-20T01:25:02Z","lastAt":"2015-09-21T18:23:00Z","messageCount":3,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"270336","messageId":"1442712302-7912-1-git-send-email-pclouds@gmail.com","threadId":"40385","inReplyTo":null,"subject":"[PATCH/RFC] Add test_repo_expect_success for running tests in a new repository","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-09-20T01:25:02Z","receivedAt":"2015-09-20T01:25:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This could be convenient when tests are independent from the rest in the\nsame file. Normally we would do this\n\ntest_expect_success '...' '\n\tgit init foo &&\n\t(\n\t\tcd foo &&\n\t\t<script>\n\t)\n'\n\nNow we can write a shorter version\n\ntest_repo_expect_success '...' '\n\t<script>\n'\n\nThe other function, test_subdir_expect_success, expands the script to\n\"( cd <repo> && <script> )\", which can be useful for grouping a series of\ntests that operate on the same repository in a subdir, e.g.\n\ntest_expect_success 'create repo abc' 'test_create_repo abc'\ntest_subdir_expect_success abc '...' <script>\ntest_subdir_expect_success abc '...' <another-script>\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Lately I start to add more and more tests in this style. So this\n looks like a good change to me.\n\n t/README                | 15 +++++++++++++++\n t/test-lib-functions.sh | 20 ++++++++++++++++++++\n 2 files changed, 35 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex 35438bc..ee761af 100644\n--- a/t/README\n+++ b/t/README\n@@ -738,6 +738,21 @@ library for your script to use.\n    the symbolic link in the file system and a part that does; then only\n    the latter part need be protected by a SYMLINKS prerequisite (see below).\n \n+ - test_subdir_expect_success <subdir> [<prereq>] <message> <script>\n+   test_subdir_expect_success <subdir> <prereq> <message> <script> <prologue>\n+\n+   Expands to\n+\n+        test_expect_success [<prereq>] <message> \"( cd <subdir> && <script> )\"\n+   or\n+\n+        test_expect_success <prereq> <message> \"<prologue> && ( cd <subdir> && <script> )\"\n+\n+ - test_repo_expect_success [<prereq>] <message> <script>\n+\n+   Create a new repository and perform <script> inside this repository\n+   in a subshell.\n+\n Prerequisites\n -------------\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex e8d3c0f..45d7423 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -394,6 +394,26 @@ test_expect_success () {\n \ttest_finish_\n }\n \n+test_subdir_expect_success () {\n+\tlocal subdir=\"$1\"\n+\tshift\n+\tcase \"$#\" in\n+\t\t2) test_expect_success \"$1\" \"( cd $subdir && $2 )\";;\n+\t\t3) test_expect_success \"$1\" \"$2\" \"( cd $subdir && $3 )\";;\n+\t\t4) test_expect_success \"$1\" \"$2\" \"$4 && ( cd $subdir && $3 )\";;\n+\t\t*) error \"bug in the test script: not 3-5 parameters to test-subdir-expect-success\";;\n+\tesac\n+}\n+\n+test_repo_expect_success () {\n+\tlocal repo=repo-$(($test_count+1))\n+\tcase \"$#\" in\n+\t\t2) test_subdir_expect_success \"$repo\" '' \"$1\" \"$2\" \"test_create_repo $repo\";;\n+\t\t3) test_subdir_expect_success \"$repo\" \"$1\" \"$2\" \"$3\" \"test_create_repo $repo\";;\n+\t\t*) error \"bug in the test script: not 2 or 3 parameters to test-repo-expect-success\";;\n+\tesac\n+}\n+\n # test_external runs external test scripts that provide continuous\n # test output about their progress, and succeeds/fails on\n # zero/non-zero exit code.  It outputs the test output on stdout even\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"270410","messageId":"xmqq4minbrks.fsf@gitster.mtv.corp.google.com","threadId":"40385","inReplyTo":"1442712302-7912-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH/RFC] Add test_repo_expect_success for running tests in a new repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-21T17:38:43Z","receivedAt":"2015-09-21T17:38:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> This could be convenient when tests are independent from the rest in the\n> same file. Normally we would do this\n>\n> test_expect_success '...' '\n> \tgit init foo &&\n> \t(\n> \t\tcd foo &&\n> \t\t<script>\n> \t)\n> '\n>\n> Now we can write a shorter version\n>\n> test_repo_expect_success '...' '\n> \t<script>\n> '\n>\n> The other function, test_subdir_expect_success, expands the script to\n> \"( cd <repo> && <script> )\", which can be useful for grouping a series of\n> tests that operate on the same repository in a subdir, e.g.\n>\n> test_expect_success 'create repo abc' 'test_create_repo abc'\n> test_subdir_expect_success abc '...' <script>\n> test_subdir_expect_success abc '...' <another-script>\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  Lately I start to add more and more tests in this style. So this\n>  looks like a good change to me.\n\nImplementations of these helper functions are not all that\ninteresting for reviewers (except for finding unacceptable bits,\nthat is), I would think.\n\nMore interesting are how much cleaner the existing code would become.\nI know we have tons of them that do \"create a new, do this and that\nin the repository\".\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index e8d3c0f..45d7423 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -394,6 +394,26 @@ test_expect_success () {\n>  \ttest_finish_\n>  }\n\nNeed a good doc here for a function that takes variable number of\nparameters in a funny way.\n\n> +test_subdir_expect_success () {\n> +\tlocal subdir=\"$1\"\n\nThis bash-ism will never be applied to my tree.\n\n> +\tshift\n> +\tcase \"$#\" in\n> +\t\t2) test_expect_success \"$1\" \"( cd $subdir && $2 )\";;\n\nMental note: Two-arg form is for title and script.\n\nBy the way, unquoted subdir makes it too sloppy to live.\n\n> +\t\t3) test_expect_success \"$1\" \"$2\" \"( cd $subdir && $3 )\";;\n\nMental note: Three-arg form is for title, prereq and script.\n\n> +\t\t4) test_expect_success \"$1\" \"$2\" \"$4 && ( cd $subdir && $3 )\";;\n\nWhat the heck is this one?  That is why I said \"implementations are\nnot interesting, the way they help existing ones more readable is\".\nIt is totally unclear where the $4 comes from and has to be given in\na funny order to the helper (and I am sure it will make perfect\nsense when it is explained).\n\n> +\t\t*) error \"bug in the test script: not 3-5 parameters to test-subdir-expect-success\";;\n\nToo deep an indent level here.\n\n> +\tesac\n> +}\n> +\n> +test_repo_expect_success () {\n> +\tlocal repo=repo-$(($test_count+1))\n> +\tcase \"$#\" in\n> +\t\t2) test_subdir_expect_success \"$repo\" '' \"$1\" \"$2\" \"test_create_repo $repo\";;\n> +\t\t3) test_subdir_expect_success \"$repo\" \"$1\" \"$2\" \"$3\" \"test_create_repo $repo\";;\n> +\t\t*) error \"bug in the test script: not 2 or 3 parameters to test-repo-expect-success\";;\n> +\tesac\n\nI do not like a few things in here (besides the same kind of\nunacceptable stuff as the other one).\n\nOften we need a new repository for testing a handful steps, and we\nwould want to be able to do this sequence:\n\n    - create a new repo\n    - do one thing in that repo\n    - do another thing in that repo\n    - do yet another thing in that repo\n\nThat would force tests to say \"test_repo_expect_success\" to do the\nfirst thing (creating and running the first command) and then\nsubsequently do \"test_subdir_expect_success $there\" to run the\nremainder.  \n\nI think we would only want to have test_expect_success_there (I am\navoiding \"subdir\" because the word has connotations that you do not\nwant in your use case.  When we say \"subdir\" we typically do not\nmean a separate repository or submodule) without the \"auto creation\nof a repository with unknown name\" bit.\n\n\ttest_expect_success 'set up a new repository for testing' '\n\t\ttest_create_repo myrepo\n\t'\n\n        test_expect_success_there mytest 'do one thing there' '\n\t\t>empty &&\n                git add empty\n                git commit -m \"add empty\"\n\t'\n\n        test_expect_success_there mytest 'do another thing there' '\n\t\tgit ls-files >actual &&\n                echo empty >expect &&\n                test_cmp expect actual\n\t'\n"},{"id":"270416","messageId":"20150921182300.GA32514@sigill.intra.peff.net","threadId":"40385","inReplyTo":"1442712302-7912-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH/RFC] Add test_repo_expect_success for running tests in a new repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-21T18:23:00Z","receivedAt":"2015-09-21T18:23:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 20, 2015 at 08:25:02AM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> This could be convenient when tests are independent from the rest in the\n> same file. Normally we would do this\n> \n> test_expect_success '...' '\n> \tgit init foo &&\n> \t(\n> \t\tcd foo &&\n> \t\t<script>\n> \t)\n> '\n> \n> Now we can write a shorter version\n> \n> test_repo_expect_success '...' '\n> \t<script>\n> '\n\nI dunno. Like Junio, I would prefer to see some proposed conversions to\nbe able to evaluate it.\n\nBut I do not find the first one all that bad. Sure, it is more lines,\nbut it is also a lot more _flexible_. You can \"init --bare\". You can\ncreate a repo, do some setup in it, then do some more stuff outside, and\nthen do some more setup inside it.\n\nI also find it weird to happen at the \"test_expect_success\" layer,\nrather than inside, as it seems somewhat orthogonal. E.g., how do I\nexpect failure?\n\nIt seems like it would be more flexible as a helper inside the test\nsnippet. But then I cannot think of a way to write it that is much\nshorter than \"(cd x && ...)\".\n\nFor single git commands, I do find myself increasingly writing the first\none as:\n\n  test_expect_success '...' '\n\tgit init foo &&\n\tgit -C foo something\n  '\n\nwhich is pretty short and clear, I think. It also keeps ancillary files\nat the top-level. So if you do:\n\n  git clone . foo &&\n  git rev-parse refs/heads/master >expect &&\n  git -C foo rev-parse refs/remotes/origin/master >actual &&\n  test_cmp expect actual\n\nall of the \"expect\" and \"actual\" appear at the same top-level, and you\ndo not have to worry about mentioning \"../expect\", etc.\n\nMy biggest problem with that technique is that you cannot run test_*\nfunctions this way, so:\n\n  git -C foo test_commit bar\n\ndoes not work, and you have to write:\n\n  (cd foo && test_commit bar)\n\nthough IMHO that is not so bad.\n\nLike I said, though, I'd reserve judgement until I saw some proposed\nconversions.\n\nIf we have a whole series of tests that are all supposed to be inside\n\"foo\", I wonder if it would be that distasteful to simply \"cd foo\", run\nthe tests, and then cd back. Of course we'd want to notice if the \"cd\"\nfailed for whatever reason. Perhaps something like:\n\n  test_pushdir foo\n  test_expect_success ...\n  test_popdir\n\nwhere \"pushdir\" would set an \"error\" flag if it fails, and any tests\ninside it would auto-fail without running (which is much safer than\ntrying to run them in the wrong directory!).\n\nThis could also be extended to any other type of state mutation besides\n\"cd\" if we wanted, though I do not have anything in mind (I guess we\ncould set env variables, but we usually just do that inside a\ntest_expect and assume it worked in subsequent tests).\n\n-Peff\n"}]}