{"thread":{"id":"61037","subject":"[PATCH] test-lib-functions: simplify `test_file_not_empty` failure message","startedAt":"2024-03-01T20:51:30Z","lastAt":"2024-03-03T06:42:53Z","messageCount":8,"participants":["Eric Sunshine","Junio C Hamano","Dirk Gouders","Rubén Justo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"489759","messageId":"20240301204922.40304-1-ericsunshine@charter.net","threadId":"61037","inReplyTo":null,"subject":"[PATCH] test-lib-functions: simplify `test_file_not_empty` failure message","fromName":"Eric Sunshine","fromEmail":"ericsunshine@charter.net","sentAt":"2024-03-01T20:49:22Z","receivedAt":"2024-03-01T20:51:30Z","isPatch":true,"sender":{"key":"ericsunshine@charter.net","avatar":null},"body":"From: Eric Sunshine \n\nThe function `test_file_not_empty` asserts that a file exists and is not\nempty. When the assertion fails, it complains:\n\n 'foo' is not a non-empty file.\n\nwhich is difficult to interpret due to the double-negative. To make it\neasier to understand the problem, simplify the message by dropping the\ndouble-negative and stating the problem more directly:\n\n 'foo' is empty but should not be\n\n(The full-stop is also dropped from the message to reflect the style of\nmessages issued by other `test_path_*` functions.)\n\nNote: Technically, the revised message is slightly less accurate since\nthe function asserts both that the file exists and that it is non-empty,\nbut the new message talks only about the emptiness of the file, not\nwhether it exists. A more accurate message might be \"'foo' is empty but\nshould not be (or doesn't exist)\", but that's unnecessarily long-winded\nand adds little information that the test author couldn't discover by\nnoticing the file's absence.\n\nSigned-off-by: Eric Sunshine \n---\n\nThis is a tangential follow-up to the discussion at [1].\n\n[1]: https://lore.kernel.org/git/CAPig+cQ+JNBwydUq0CsTZGs8mHs3L3fJDuSosd+-WdKwWWw=gg@mail.gmail.com/\n\n t/test-lib-functions.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex b5eaf7fdc1..9e97b324c5 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -991,7 +991,7 @@ test_file_not_empty () {\n \ttest \"$#\" = 2 && BUG \"2 param\"\n \tif ! test -s \"$1\"\n \tthen\n-\t\techo \"'$1' is not a non-empty file.\"\n+\t\techo \"'$1' is empty but should not be\"\n \t\tfalse\n \tfi\n }\n-- \n2.44.0\n\n"},{"id":"489760","messageId":"xmqqsf194n7a.fsf@gitster.g","threadId":"61037","inReplyTo":"20240301204922.40304-1-ericsunshine@charter.net","subject":"Re: [PATCH] test-lib-functions: simplify `test_file_not_empty` failure message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-01T22:11:21Z","receivedAt":"2024-03-01T22:11:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Note: Technically, the revised message is slightly less accurate since\n> the function asserts both that the file exists and that it is non-empty,\n> but the new message talks only about the emptiness of the file, not\n> whether it exists.\n>\n> A more accurate message might be \"'foo' is empty but\n> should not be (or doesn't exist)\", but that's unnecessarily long-winded\n> and adds little information that the test author couldn't discover by\n> noticing the file's absence.\n\nBesides, that is way too confusing. \" is empty or it does not\nexist\" I may understand, but with your construct, I wouldn't be able\nto tell how I am supposed to interpret the \"(or doesn't exist)\"\npart.\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index b5eaf7fdc1..9e97b324c5 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -991,7 +991,7 @@ test_file_not_empty () {\n> \ttest \"$#\" = 2 && BUG \"2 param\"\n> \tif ! test -s \"$1\"\n> \tthen\n> -\t\techo \"'$1' is not a non-empty file.\"\n> +\t\techo \"'$1' is empty but should not be\"\n\nThe \"adds little information\" version may be\n\n\t\techo \"'$1' is either missing or empty, but should not be\"\n\nAnd avoiding \"X is Y, but should be ~Y\" construct, perhaps\n\n\t\techo \"'$1' should be a file with non-empty contents\"\n\nwould work better? I dunno.\n"},{"id":"489761","messageId":"CAPig+cSt4Q0e+DioFiy7yjBgn5+gWNqQOYc0eCAgdYrZUp8VBQ@mail.gmail.com","threadId":"61037","inReplyTo":"xmqqsf194n7a.fsf@gitster.g","subject":"Re: [PATCH] test-lib-functions: simplify `test_file_not_empty` failure message","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-01T22:59:42Z","receivedAt":"2024-03-01T22:59:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 1, 2024 at 5:11 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > A more accurate message might be \"'foo' is empty but\n> > should not be (or doesn't exist)\", but that's unnecessarily long-winded\n> > and adds little information that the test author couldn't discover by\n> > noticing the file's absence.\n>\n> The \"adds little information\" version may be\n>\n> echo \"'$1' is either missing or empty, but should not be\"\n>\n> And avoiding \"X is Y, but should be ~Y\" construct, perhaps\n>\n> echo \"'$1' should be a file with non-empty contents\"\n>\n> would work better? I dunno.\n\nI find \"'$1' is either missing or empty, but should not be\" suggestion\nclear and easily understood. I'll reroll with that.\n"},{"id":"489762","messageId":"ghr0gtglhd.fsf@gouders.net","threadId":"61037","inReplyTo":"CAPig+cSt4Q0e+DioFiy7yjBgn5+gWNqQOYc0eCAgdYrZUp8VBQ@mail.gmail.com","subject":"Re: [PATCH] test-lib-functions: simplify `test_file_not_empty` failure message","fromName":"Dirk Gouders","fromEmail":"dirk@gouders.net","sentAt":"2024-03-02T07:07:42Z","receivedAt":"2024-03-02T07:08:02Z","isPatch":true,"sender":{"key":"dirk@gouders.net","avatar":"https://avatars.githubusercontent.com/u/81326422?v=4"},"body":"Eric Sunshine writes:\n\n> On Fri, Mar 1, 2024 at 5:11 PM Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>> > A more accurate message might be \"'foo' is empty but\n>> > should not be (or doesn't exist)\", but that's unnecessarily long-winded\n>> > and adds little information that the test author couldn't discover by\n>> > noticing the file's absence.\n>>\n>> The \"adds little information\" version may be\n>>\n>> echo \"'$1' is either missing or empty, but should not be\"\n>>\n>> And avoiding \"X is Y, but should be ~Y\" construct, perhaps\n>>\n>> echo \"'$1' should be a file with non-empty contents\"\n>>\n>> would work better? I dunno.\n>\n> I find \"'$1' is either missing or empty, but should not be\" suggestion\n> clear and easily understood. I'll reroll with that.\n\nThis is a view from a position with more distance:\n\nI find that not so easily understood -- the \"but should not\nbe\" part is rather unexpected and I feel, it doesn't provide necessary\ninformation, e.g.:\n\ntest_path_is_executable () {\n...\n\t\techo \"$1 is not executable\"\n...\n\nalso doesn't state what is wanted and I doubt that message doesn't\nclearly describe the problem.\n\nWhile I looked at it: there is another `test -s` in test_grep () that\nperhaps could be fixed the same way:\n\n\tif test -s \"$last_arg\"\n\tthen\n\t\tcat >&4 \"$last_arg\"\n\telse\n\t\techo >&4 \"\"\n\tfi\n\nDirk\n"},{"id":"489780","messageId":"cf978790-4885-4103-946d-10f807048441@gmail.com","threadId":"61037","inReplyTo":"20240301204922.40304-1-ericsunshine@charter.net","subject":"Re: [PATCH] test-lib-functions: simplify `test_file_not_empty` failure message","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-02T16:38:57Z","receivedAt":"2024-03-02T16:39:09Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Mar 01, 2024 at 03:49:22PM -0500, Eric Sunshine wrote:\n> From: Eric Sunshine \n> \n> The function `test_file_not_empty` asserts that a file exists and is not\n> empty. When the assertion fails, it complains:\n> \n> 'foo' is not a non-empty file.\n> \n> which is difficult to interpret due to the double-negative. To make it\n> easier to understand the problem, simplify the message by dropping the\n> double-negative and stating the problem more directly:\n> \n> 'foo' is empty but should not be\n> \n> (The full-stop is also dropped from the message to reflect the style of\n> messages issued by other `test_path_*` functions.)\n> \n> Note: Technically, the revised message is slightly less accurate since\n> the function asserts both that the file exists and that it is non-empty,\n> but the new message talks only about the emptiness of the file, not\n> whether it exists. A more accurate message might be \"'foo' is empty but\n> should not be (or doesn't exist)\", but that's unnecessarily long-winded\n> and adds little information that the test author couldn't discover by\n> noticing the file's absence.\n\nTo improve the accuracy of the message, I wonder if it is worth doing\nwhat we do in test_must_be_empty:\n\n\ttest_must_be_empty () {\n\t\ttest \"$#\" -ne 1 && BUG \"1 param\"\n\t\ttest_path_is_file \"$1\" &&\n\t\tif test -s \"$1\"\n\t\tthen\n\t\t\techo \"'$1' is not empty, it contains:\"\n\t\t\tcat \"$1\"\n\t\t\treturn 1\n\t\tfi\n\t}\n\nPerhaps:\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex b5eaf7fdc1..5b5ee0dc1d 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -989,9 +989,10 @@ test_dir_is_empty () {\n # Check if the file exists and has a size greater than zero\n test_file_not_empty () {\n test \"$#\" = 2 && BUG \"2 param\"\n+ test_path_is_file \"$1\" &&\n if ! test -s \"$1\"\n then\n- echo \"'$1' is not a non-empty file.\"\n+\t\techo \"'$1' is empty but should not be\"\n false\n fi\n }\n\n> \n> Signed-off-by: Eric Sunshine \n> ---\n> \n> This is a tangential follow-up to the discussion at [1].\n> \n> [1]: https://lore.kernel.org/git/CAPig+cQ+JNBwydUq0CsTZGs8mHs3L3fJDuSosd+-WdKwWWw=gg@mail.gmail.com/\n> \n> t/test-lib-functions.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index b5eaf7fdc1..9e97b324c5 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -991,7 +991,7 @@ test_file_not_empty () {\n> \ttest \"$#\" = 2 && BUG \"2 param\"\n> \tif ! test -s \"$1\"\n> \tthen\n> -\t\techo \"'$1' is not a non-empty file.\"\n> +\t\techo \"'$1' is empty but should not be\"\n> \t\tfalse\n> \tfi\n> }\n> -- \n> 2.44.0\n> \n"},{"id":"489783","messageId":"xmqqbk7wy1di.fsf@gitster.g","threadId":"61037","inReplyTo":"ghr0gtglhd.fsf@gouders.net","subject":"Re: [PATCH] test-lib-functions: simplify `test_file_not_empty` failure message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-02T17:44:41Z","receivedAt":"2024-03-02T17:44:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dirk Gouders writes:\n\n> Eric Sunshine writes:\n>\n>> On Fri, Mar 1, 2024 at 5:11 PM Junio C Hamano wrote:\n>>> Eric Sunshine writes:\n>>> > A more accurate message might be \"'foo' is empty but\n>>> > should not be (or doesn't exist)\", but that's unnecessarily long-winded\n>>> > and adds little information that the test author couldn't discover by\n>>> > noticing the file's absence.\n>>>\n>>> The \"adds little information\" version may be\n>>>\n>>> echo \"'$1' is either missing or empty, but should not be\"\n>>> ...\n>> I find \"'$1' is either missing or empty, but should not be\" suggestion\n>> clear and easily understood. I'll reroll with that.\n>\n> This is a view from a position with more distance:\n>\n> I find that not so easily understood -- the \"but should not\n> be\" part is rather unexpected and I feel, it doesn't provide necessary\n> information, e.g.:\n>\n> test_path_is_executable () {\n> ...\n> \t\techo \"$1 is not executable\"\n> ...\n>\n> also doesn't state what is wanted and I doubt that message doesn't\n> clearly describe the problem.\n\nI cannot tell if you really meant the double negative involving\n\"doubt\", but assuming you did, you are saying that\n\n With \"X is not Y\", it is clear enough that we expect X to be Y\n (if it were not clear to somebody who read \"X is not Y\" that we\n want X to be Y, then \"X is not Y, but it should be\" may needed,\n but \"X is not Y\" is clear enough).\n\nSo you think \"$1 is either missing or empty\" is better without \"but\nshould not be\" added to the end? Am I reading you correctly?\n\nI think this takes us back pretty much to square one ;-) but that is\nalso fine.\n\nBut the above argument depends on an untold assumption. The message\n\"X is not Y\" must be clearly understood as a complaint, not a mere\nstatement of a fact. I am not sure if that is the case.\n\nInstead of \"X is not Y, but it should be\", the way to clarify these\nmessages may be to say \"error: X is not Y\", perhaps?\n\n> While I looked at it: there is another `test -s` in test_grep () that\n> perhaps could be fixed the same way:\n>\n> \tif test -s \"$last_arg\"\n> \tthen\n> \t\tcat >&4 \"$last_arg\"\n> \telse\n> \t\techo >&4 \"\"\n> \tfi\n\nIf you are worried about \"test -s\" failing because \"$last_arg\" does\nnot exist, then you are worried too much. We upfront guard the\ntest_grep() helper with \"test -f\" of the same file and diagnoses the\nlack of the file as a bug in the test. And we do not assume gremlins\nremoving random files while we are running tests.\n\n"},{"id":"489786","messageId":"xmqq1q8sy09t.fsf@gitster.g","threadId":"61037","inReplyTo":"cf978790-4885-4103-946d-10f807048441@gmail.com","subject":"Re: [PATCH] test-lib-functions: simplify `test_file_not_empty` failure message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-02T18:08:30Z","receivedAt":"2024-03-02T18:08:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> To improve the accuracy of the message, I wonder if it is worth doing\n> ...\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index b5eaf7fdc1..5b5ee0dc1d 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -989,9 +989,10 @@ test_dir_is_empty () {\n> # Check if the file exists and has a size greater than zero\n> test_file_not_empty () {\n> test \"$#\" = 2 && BUG \"2 param\"\n> + test_path_is_file \"$1\" &&\n> if ! test -s \"$1\"\n> then\n> - echo \"'$1' is not a non-empty file.\"\n> +\t\techo \"'$1' is empty but should not be\"\n> false\n> fi\n> }\n\nSimple and effective to remove the need to have to worry about the\n\"missing\" case. The \"but should not be\" part may still be subject\nto discussion, but I do not have a strong opinion there.\n"},{"id":"489801","messageId":"ghmsrfhl43.fsf@gouders.net","threadId":"61037","inReplyTo":"xmqqbk7wy1di.fsf@gitster.g","subject":"Re: [PATCH] test-lib-functions: simplify `test_file_not_empty` failure message","fromName":"Dirk Gouders","fromEmail":"dirk@gouders.net","sentAt":"2024-03-03T06:42:36Z","receivedAt":"2024-03-03T06:42:53Z","isPatch":true,"sender":{"key":"dirk@gouders.net","avatar":"https://avatars.githubusercontent.com/u/81326422?v=4"},"body":"Junio C Hamano writes:\n\n> Dirk Gouders writes:\n>\n>> Eric Sunshine writes:\n>>\n>>> On Fri, Mar 1, 2024 at 5:11 PM Junio C Hamano wrote:\n>>>> Eric Sunshine writes:\n>>>> > A more accurate message might be \"'foo' is empty but\n>>>> > should not be (or doesn't exist)\", but that's unnecessarily long-winded\n>>>> > and adds little information that the test author couldn't discover by\n>>>> > noticing the file's absence.\n>>>>\n>>>> The \"adds little information\" version may be\n>>>>\n>>>> echo \"'$1' is either missing or empty, but should not be\"\n>>>> ...\n>>> I find \"'$1' is either missing or empty, but should not be\" suggestion\n>>> clear and easily understood. I'll reroll with that.\n>>\n>> This is a view from a position with more distance:\n>>\n>> I find that not so easily understood -- the \"but should not\n>> be\" part is rather unexpected and I feel, it doesn't provide necessary\n>> information, e.g.:\n>>\n>> test_path_is_executable () {\n>> ...\n>> \t\techo \"$1 is not executable\"\n>> ...\n>>\n>> also doesn't state what is wanted and I doubt that message doesn't\n>> clearly describe the problem.\n>\n> I cannot tell if you really meant the double negative involving\n> \"doubt\", but assuming you did, you are saying that\n\nI'm sorry about that double negative which was probably wrong wording of\na non-native speaker.\n\n> With \"X is not Y\", it is clear enough that we expect X to be Y\n> (if it were not clear to somebody who read \"X is not Y\" that we\n> want X to be Y, then \"X is not Y, but it should be\" may needed,\n> but \"X is not Y\" is clear enough).\n>\n> So you think \"$1 is either missing or empty\" is better without \"but\n> should not be\" added to the end? Am I reading you correctly?\n>\n> I think this takes us back pretty much to square one ;-) but that is\n> also fine.\n>\n> But the above argument depends on an untold assumption. The message\n> \"X is not Y\" must be clearly understood as a complaint, not a mere\n> statement of a fact. I am not sure if that is the case.\n>\n> Instead of \"X is not Y, but it should be\", the way to clarify these\n> messages may be to say \"error: X is not Y\", perhaps?\n\nThat is exactly what came to my mind when I was later re-thinking\nwhat I had written.\n\n>> While I looked at it: there is another `test -s` in test_grep () that\n>> perhaps could be fixed the same way:\n>>\n>> \tif test -s \"$last_arg\"\n>> \tthen\n>> \t\tcat >&4 \"$last_arg\"\n>> \telse\n>> \t\techo >&4 \"\"\n>> \tfi\n>\n> If you are worried about \"test -s\" failing because \"$last_arg\" does\n> not exist, then you are worried too much. We upfront guard the\n> test_grep() helper with \"test -f\" of the same file and diagnoses the\n> lack of the file as a bug in the test. And we do not assume gremlins\n> removing random files while we are running tests.\n\nYes, thank you for clarification and sorry for the noise.\n\nDirk\n"}]}