{"thread":{"id":"31087","subject":"Re: [RFC/PATCH v2] t3300-*.sh: Fix a TAP parse error","startedAt":"2012-07-24T18:48:20Z","lastAt":"2012-07-25T19:46:53Z","messageCount":3,"participants":["Jonathan Nieder","Ramsay Jones"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"195646","messageId":"500EEDF4.7050007@ramsay1.demon.co.uk","threadId":"31087","inReplyTo":null,"subject":"[RFC/PATCH v2] t3300-*.sh: Fix a TAP parse error","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-07-24T18:48:20Z","receivedAt":"2012-07-24T18:48:20Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nAt present, running the t3300-*.sh test on cygwin looks like:\n\n    $ cd t\n    $ ./t3300-funny-names.sh\n    ok 1 - setup\n    # passed all 1 test(s)\n    1..1 # SKIP Your filesystem does not allow tabs in filenames\n    $\n\nUnfortunately, this is not valid TAP output, which prove notes\nas follows:\n\n    $ prove --exec sh t3300-funny-names.sh\n    t3300-funny-names.sh .. All 1 subtests passed\n\n    Test Summary Report\n    -------------------\n    t3300-funny-names.sh (Wstat: 0 Tests: 1 Failed: 0)\n      Parse errors: No plan found in TAP output\n    Files=1, Tests=1,  2 wallclock secs ( 0.05 usr  0.00 sys +  \\\n        0.90 cusr  0.49 csys =  1.43 CPU)\n    Result: FAIL\n    $\n\nThis is due to the 'trailing_plan' having a 'skip_directive'\nattached to it. This is not allowed by the TAP grammar, which\nonly allows a 'leading_plan' to be followed by an optional\n'skip_directive'. (see perldoc TAP::Parser::Grammar).\n\nA trailing_plan is one that appears in the TAP output after one or\nmore test status lines (that start 'not '? 'ok ' ...), whereas a\nleading_plan must appear before all test status lines (if any).\n\nIn practice, this means that the test script cannot contain a use\nof the 'skip all' facility:\n\n    skip_all='Some reason to skip *all* tests in this file'\n    test_done\n\nafter having already executed one or more tests with (for example)\n'test_expect_success'. Unfortunately, this is exactly what this\ntest script is doing. The first 'setup' test is actually used to\ndetermine if the test prerequisite is satisfied by the filesystem\n(ie does it allow tabs in filenames?).\n\nIn order to fix the parse errors, place the code to determine the\ntest prerequisite at the top level of the script, prior to the\nfirst test, rather than as a parameter to test_expect_success.\nThis allows us to correctly use 'skip_all', thus:\n\n    $ ./t3300-funny-names.sh\n    # passed all 0 test(s)\n    1..0 # SKIP Your filesystem does not allow tabs in filenames\n    $\n\n    $ prove --exec sh t3300-funny-names.sh\n    t3300-funny-names.sh .. skipped: Your filesystem does not \\\n        allow tabs in filenames\n    Files=1, Tests=0,  2 wallclock secs ( 0.02 usr  0.03 sys +  \\\n        0.84 cusr  0.41 csys =  1.29 CPU)\n    Result: NOTESTS\n    $\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nHi Jonathan,\n\nThis version of the patch only moves code to determine the test\nprerequisite to the outer level, while leaving the 'setup' aspects\nof the first test in place.\n\nI did think about merging the following test (\"setup: populate index\nand tree\") into the first test, but I wanted to keep it simple for now.\n\nWhat do you think?\n\nATB,\nRamsay Jones\n\n t/t3300-funny-names.sh | 24 +++++++++++-------------\n 1 file changed, 11 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t3300-funny-names.sh b/t/t3300-funny-names.sh\nindex 1f35e55..7480d6e 100755\n--- a/t/t3300-funny-names.sh\n+++ b/t/t3300-funny-names.sh\n@@ -11,6 +11,16 @@ tree, index, and tree objects.\n \n . ./test-lib.sh\n \n+HT='\t'\n+\n+echo 2>/dev/null > \"Name with an${HT}HT\"\n+if ! test -f \"Name with an${HT}HT\"\n+then\n+\t# since FAT/NTFS does not allow tabs in filenames, skip this test\n+\tskip_all='Your filesystem does not allow tabs in filenames'\n+\ttest_done\n+fi\n+\n p0='no-funny'\n p1='tabs\t,\" (dq) and spaces'\n p2='just space'\n@@ -23,21 +33,9 @@ test_expect_success 'setup' '\n \tEOF\n \n \t{ cat \"$p0\" >\"$p1\" || :; } &&\n-\t{ echo \"Foo Bar Baz\" >\"$p2\" || :; } &&\n-\n-\tif test -f \"$p1\" && cmp \"$p0\" \"$p1\"\n-\tthen\n-\t\ttest_set_prereq TABS_IN_FILENAMES\n-\tfi\n+\t{ echo \"Foo Bar Baz\" >\"$p2\" || :; }\n '\n \n-if ! test_have_prereq TABS_IN_FILENAMES\n-then\n-\t# since FAT/NTFS does not allow tabs in filenames, skip this test\n-\tskip_all='Your filesystem does not allow tabs in filenames'\n-\ttest_done\n-fi\n-\n test_expect_success 'setup: populate index and tree' '\n \tgit update-index --add \"$p0\" \"$p2\" &&\n \tt0=$(git write-tree)\n-- \n1.7.11.2\n"},{"id":"195645","messageId":"20120724191707.GA5210@burratino","threadId":"31087","inReplyTo":"500EEDF4.7050007@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH v2] t3300-*.sh: Fix a TAP parse error","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-24T19:17:07Z","receivedAt":"2012-07-24T19:17:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRamsay Jones wrote:\n\n> Hi Jonathan,\n>\n> This version of the patch only moves code to determine the test\n> prerequisite to the outer level, while leaving the 'setup' aspects\n> of the first test in place.\n\nI guess I don't see the point.  The current convention of \"don't do\nanything complicated outside test assertions\" is easy to explain.\nWhat new convention are you suggesting to replace it?\n\nThanks,\nJonathan\n"},{"id":"195780","messageId":"50104D2D.70109@ramsay1.demon.co.uk","threadId":"31087","inReplyTo":"20120724191707.GA5210@burratino","subject":"Re: [RFC/PATCH v2] t3300-*.sh: Fix a TAP parse error","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-07-25T19:46:53Z","receivedAt":"2012-07-25T19:46:53Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jonathan Nieder wrote:\n>> This version of the patch only moves code to determine the test\n>> prerequisite to the outer level, while leaving the 'setup' aspects\n>> of the first test in place.\n> \n> I guess I don't see the point.\n\nYou don't see the point of fixing the TAP Parse error? :-D\n\n>                                The current convention of \"don't do\n> anything complicated outside test assertions\" is easy to explain.\n> What new convention are you suggesting to replace it?\n\nHmm, well I guess I'm not going to suggest anything!\n\nHowever, what is \"anything complicated\"?\n\nAt the end of test-lib.sh we find:\n\n    # test whether the filesystem supports symbolic links\n    ln -s x y 2>/dev/null && test -h y 2>/dev/null && test_set_prereq SYMLINKS\n    rm -f y\n\n    # When the tests are run as root, permission tests will report that\n    # things are writable when they shouldn't be.\n    test -w / || test_set_prereq SANITY\n\nIs this too complicated? If not, why not? If yes, should it be added to\na test assertion?\n\nWould it be acceptable for me to add some code, here at the end of\ntest-lib.sh, to set the TABS_IN_FILENAME test prerequisite and use it\nin tests t3300-funny-names.sh, t3902-quoted.sh, t3600-rm.sh,\nt4016-diff-quote.sh and t4135-apply-weird-filenames.sh?\n\nHow about some of the test \"library\" files:\n\ndiff-lib.sh              lib-git-p4.sh            lib-read-tree.sh\ngitweb-lib.sh            lib-git-svn.sh           lib-rebase.sh\nlib-bash.sh              lib-gpg.sh*              lib-t6000.sh\nlib-credential.sh*       lib-httpd.sh             lib-terminal.sh\nlib-cvs.sh               lib-pager.sh             test-lib-functions.sh\nlib-diff-alternative.sh  lib-patch-mode.sh        test-lib.sh\nlib-gettext.sh           lib-prereq-FILEMODE.sh\nlib-git-daemon.sh        lib-read-tree-m-3way.sh\n\nSeveral of these files contain executable code (rather than just a\nlibrary of functions). For example, look at lib-cvs.sh, lib-httpd.sh,\nand lib-prereq-FILEMODE.sh. Is this code too complicated?\n\nWould it be acceptable for me to create an lib-prereq-TABSINFILE.sh\nfile so that I could source it only in the test files that require\nthe TABS_IN_FILENAME prerequisite?\n\nATB,\nRamsay Jones\n"}]}