diff mbox series

[3/4] t1300: add more tests for whitespace and inline comments

Message ID 590731e15a01558d1bbcdfc01df4f78573138742.1710508691.git.dsimic@manjaro.org (mailing list archive)
State Superseded
Headers show
Series Fix a bug in configuration parsing, and improve tests and documentation | expand

Commit Message

Dragan Simic March 15, 2024, 1:22 p.m. UTC 
Add a handful of additional automated tests, to improve the coverage of
configuration file entries whose values contain internal whitespace, leading
and/or trailing whitespace, or which contain an additional inline comment.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 t/t1300-config.sh | 102 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 101 insertions(+), 1 deletion(-)

Comments

Eric Sunshine March 15, 2024, 7:39 p.m. UTC | #1 
On Fri, Mar 15, 2024 at 9:22 AM Dragan Simic <dsimic@manjaro.org> wrote:
> Add a handful of additional automated tests, to improve the coverage of
> configuration file entries whose values contain internal whitespace, leading
> and/or trailing whitespace, or which contain an additional inline comment.
>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>

Just some minor style-related comments...

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> @@ -11,6 +11,96 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +cat > .git/config << EOF
> +[section]
> +       solid = rock
> +       sparse = big             blue
> +       sparseAndTail = big              blue
> +       sparseAndTailQuoted = "big               blue "
> +       sparseAndBiggerTail = big                blue
> +       sparseAndBiggerTailQuoted = "big                 blue           "
> +       sparseAndBiggerTailQuotedPlus =  "big            blue           "
> +       headAndTail =   big blue
> +       headAndTailQuoted = "   big blue "
> +       headAndTailQuotedPlus =  "      big blue "
> +       annotated = big blue    # to be discarded
> +       annotatedQuoted = "big blue"    # to be discarded
> +EOF

These days we try to place all test-related code inside a
test_expect_success() context rather than having it standalone. In
this case, since the file being created is (presumably) shared by
multiple tests in this script, you may want to add a new test which
performs this setup step.

Use \EOF rather than EOF to signal to readers that we don't expect any
variable interpolation to happen within the here-doc body.

Further, use -\EOF inside test_expect_success() to allow us to indent
the body of the heredoc to match the test indentation.

Style guideline says to omit whitespace after redirection operators
(such as `<<` and `>`).

We have a q_to_tab() function which lets us state explicitly for
readers the location of TAB characters in the heredoc body. You'll
often see that used instead of literal TABs.

Taking all the above into account, perhaps:

    test_expect_success 'setup whitespace' '
        q_to_tab >.git/config <<-\EOF
        [section]
        solid = rock
        sparse = bigQblue
        ...
        EOF

Same comments apply to rest of patch.
Dragan Simic March 15, 2024, 8:04 p.m. UTC | #2 
Hello Eric,

On 2024-03-15 20:39, Eric Sunshine wrote:
> On Fri, Mar 15, 2024 at 9:22 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> Add a handful of additional automated tests, to improve the coverage 
>> of
>> configuration file entries whose values contain internal whitespace, 
>> leading
>> and/or trailing whitespace, or which contain an additional inline 
>> comment.
>> 
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> 
> Just some minor style-related comments...
> 
>> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
>> @@ -11,6 +11,96 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>> +cat > .git/config << EOF
>> +[section]
>> +       solid = rock
>> +       sparse = big             blue
>> +       sparseAndTail = big              blue
>> +       sparseAndTailQuoted = "big               blue "
>> +       sparseAndBiggerTail = big                blue
>> +       sparseAndBiggerTailQuoted = "big                 blue          
>>  "
>> +       sparseAndBiggerTailQuotedPlus =  "big            blue          
>>  "
>> +       headAndTail =   big blue
>> +       headAndTailQuoted = "   big blue "
>> +       headAndTailQuotedPlus =  "      big blue "
>> +       annotated = big blue    # to be discarded
>> +       annotatedQuoted = "big blue"    # to be discarded
>> +EOF
> 
> These days we try to place all test-related code inside a
> test_expect_success() context rather than having it standalone. In
> this case, since the file being created is (presumably) shared by
> multiple tests in this script, you may want to add a new test which
> performs this setup step.
> 
> Use \EOF rather than EOF to signal to readers that we don't expect any
> variable interpolation to happen within the here-doc body.
> 
> Further, use -\EOF inside test_expect_success() to allow us to indent
> the body of the heredoc to match the test indentation.
> 
> Style guideline says to omit whitespace after redirection operators
> (such as `<<` and `>`).
> 
> We have a q_to_tab() function which lets us state explicitly for
> readers the location of TAB characters in the heredoc body. You'll
> often see that used instead of literal TABs.
> 
> Taking all the above into account, perhaps:
> 
>     test_expect_success 'setup whitespace' '
>         q_to_tab >.git/config <<-\EOF
>         [section]
>         solid = rock
>         sparse = bigQblue
>         ...
>         EOF
> 
> Same comments apply to rest of patch.

Thank you for your review and all suggestions!  I'll make sure to rework
the tests in v2, in the way you described it above.

I'll come back with any questions I might have while reworking the new
tests.  I hope that's fine.
Eric Sunshine March 15, 2024, 8:29 p.m. UTC | #3 
On Fri, Mar 15, 2024 at 3:39 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> These days we try to place all test-related code inside a
> test_expect_success() context rather than having it standalone. In
> this case, since the file being created is (presumably) shared by
> multiple tests in this script, you may want to add a new test which
> performs this setup step.
>
> Taking all the above into account, perhaps:
>
>     test_expect_success 'setup whitespace' '
>         q_to_tab >.git/config <<-\EOF
>         [section]
>         solid = rock
>         sparse = bigQblue
>         ...
>         EOF
>
> Same comments apply to rest of patch.

To be clear, this case is special because the file being created is
shared by multiple tests, so it deserves being placed in its own
test_expect_success() invocation.

For the remaining cases where you're doing some set-up outside of
test_expect_success(), just move the set-up code into the
corresponding test_expect_success() invocation. For instance, rather
than:

    echo 'big               blue' > expect

    test_expect_success 'internal whitespace' '
        git config --get section.sparse > output &&
        test_cmp expect output
    '

do this:

    test_expect_success 'internal whitespace' '
        echo 'bigQblue' | q_to_tab >expect
        git config --get section.sparse >actual &&
        test_cmp expect actual
    '

(I changed "output" to "actual" above since the names "expect" and
"actual" are common in the tests.)
Dragan Simic March 15, 2024, 9:42 p.m. UTC | #4 
On 2024-03-15 21:29, Eric Sunshine wrote:
> On Fri, Mar 15, 2024 at 3:39 PM Eric Sunshine <sunshine@sunshineco.com> 
> wrote:
>> These days we try to place all test-related code inside a
>> test_expect_success() context rather than having it standalone. In
>> this case, since the file being created is (presumably) shared by
>> multiple tests in this script, you may want to add a new test which
>> performs this setup step.
>> 
>> Taking all the above into account, perhaps:
>> 
>>     test_expect_success 'setup whitespace' '
>>         q_to_tab >.git/config <<-\EOF
>>         [section]
>>         solid = rock
>>         sparse = bigQblue
>>         ...
>>         EOF
>> 
>> Same comments apply to rest of patch.
> 
> To be clear, this case is special because the file being created is
> shared by multiple tests, so it deserves being placed in its own
> test_expect_success() invocation.
> 
> For the remaining cases where you're doing some set-up outside of
> test_expect_success(), just move the set-up code into the
> corresponding test_expect_success() invocation. For instance, rather
> than:
> 
>     echo 'big               blue' > expect
> 
>     test_expect_success 'internal whitespace' '
>         git config --get section.sparse > output &&
>         test_cmp expect output
>     '
> 
> do this:
> 
>     test_expect_success 'internal whitespace' '
>         echo 'bigQblue' | q_to_tab >expect
>         git config --get section.sparse >actual &&
>         test_cmp expect actual
>     '
> 
> (I changed "output" to "actual" above since the names "expect" and
> "actual" are common in the tests.)

This looks nice, thanks again.  It keeps the expected results and
the test execution in a single "block", making it a bit easier to
keep track of different tests and their expected results.
diff mbox series

Patch

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 31c387868708..6eef8a48098c 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -11,6 +11,96 @@  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+cat > .git/config << EOF
+[section]
+	solid = rock
+	sparse = big 		 blue
+	sparseAndTail = big 		 blue 
+	sparseAndTailQuoted = "big 		 blue "
+	sparseAndBiggerTail = big 		 blue 	 	
+	sparseAndBiggerTailQuoted = "big 		 blue 	 	"
+	sparseAndBiggerTailQuotedPlus =  "big 		 blue 	 	"	 
+	headAndTail = 	big blue 
+	headAndTailQuoted = "	big blue "
+	headAndTailQuotedPlus =  "	big blue " 
+	annotated = big blue	# to be discarded
+	annotatedQuoted = "big blue"	# to be discarded
+EOF
+
+echo 'rock' > expect
+
+test_expect_success 'no internal whitespace' '
+	git config --get section.solid > output &&
+	test_cmp expect output
+'
+
+echo 'big 		 blue' > expect
+
+test_expect_success 'internal whitespace' '
+	git config --get section.sparse > output &&
+	test_cmp expect output
+'
+
+test_expect_success 'internal and trailing whitespace' '
+	git config --get section.sparseAndTail > output &&
+	test_cmp expect output
+'
+
+test_expect_success 'internal and more trailing whitespace' '
+	git config --get section.sparseAndBiggerTail > output &&
+	test_cmp expect output
+'
+
+echo 'big 		 blue ' > expect
+
+test_expect_success 'internal and trailing whitespace, all quoted' '
+	git config --get section.sparseAndTailQuoted > output &&
+	test_cmp expect output
+'
+
+echo 'big 		 blue 	 	' > expect
+
+test_expect_success 'internal and more trailing whitespace, all quoted' '
+	git config --get section.sparseAndBiggerTailQuoted > output &&
+	test_cmp expect output
+'
+
+test_expect_success 'internal and more trailing whitespace, not all quoted' '
+	git config --get section.sparseAndBiggerTailQuotedPlus > output &&
+	test_cmp expect output
+'
+
+echo 'big blue' > expect
+
+test_expect_success 'leading and trailing whitespace' '
+	git config --get section.headAndTail > output &&
+	test_cmp expect output
+'
+
+echo '	big blue ' > expect
+
+test_expect_success 'leading and trailing whitespace, all quoted' '
+	git config --get section.headAndTailQuoted > output &&
+	test_cmp expect output
+'
+
+test_expect_success 'leading and trailing whitespace, not all quoted' '
+	git config --get section.headAndTailQuotedPlus > output &&
+	test_cmp expect output
+'
+
+echo 'big blue' > expect
+
+test_expect_success 'inline comment' '
+	git config --get section.annotated > output &&
+	test_cmp expect output
+'
+
+test_expect_success 'inline comment, quoted' '
+	git config --get section.annotatedQuoted > output &&
+	test_cmp expect output
+'
+
 test_expect_success 'clear default config' '
 	rm -f .git/config
 '
@@ -1066,7 +1156,17 @@  test_expect_success '--null --get-regexp' '
 	test_cmp expect result
 '
 
-test_expect_success 'inner whitespace kept verbatim' '
+test_expect_success 'inner whitespace kept verbatim, spaces only' '
+	git config section.val "foo   bar" &&
+	test_cmp_config "foo   bar" section.val
+'
+
+test_expect_success 'inner whitespace kept verbatim, horizontal tabs only' '
+	git config section.val "foo		bar" &&
+	test_cmp_config "foo		bar" section.val
+'
+
+test_expect_success 'inner whitespace kept verbatim, horizontal tabs and spaces' '
 	git config section.val "foo 	  bar" &&
 	test_cmp_config "foo 	  bar" section.val
 '