Re: [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 15, 2024, 17:27 UTC
Message-ID: <xmqqy1cq4ide.fsf@gitster.g>
In-Reply-To: <93468f5c-5f62-4f22-85ce-b60621852430@gmail.com>

Rubén Justo <rjusto@gmail.com> writes:

Show 22 quoted lines

>> To test the effect of setting one configuration variable, and ensure
>> it results in a slightly different advice message output to the
>> standard error stream, "test-tool advice" needs only a single line
>> of patch, but if we started with this version, how much work does it
>> take to run the equivalent test in the other patch if it were to be
>> rebased on top of this change?  It won't be just the matter of
>> adding a new TEST(check_advise_if_enabled()) call to cmd_main(),
>> will it?
>
> Maybe something like this will do the trick:
>
> diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c
> index 15df29c955..ac7d2620ef 100644
> --- a/t/unit-tests/t-advise.c
> +++ b/t/unit-tests/t-advise.c
> @@ -6,6 +6,7 @@
>
>  static const char expect_advice_msg[] = "hint: This is a piece of advice\n"
>                                         "hint: Disable this message with \"git config advice.nestedTag false\"\n";
> +static const char expect_advice_msg_without_disable_hint[] = "hint: This is a piece of advice\n";
>  static const char advice_msg[] = "This is a piece of advice";
>  static const char out_file[] = "./output.txt";

Yup, but ...

Show 9 quoted lines

> @@ -44,7 +45,7 @@ int cmd_main(int argc, const char **argv) {
>
>         TEST(check_advise_if_enabled(advice_msg, NULL, expect_advice_msg),
>                 "advice should be printed when config variable is unset");
> -       TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg),
> +       TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg_without_disable_hint),
>                 "advice should be printed when config variable is set to true");
>         TEST(check_advise_if_enabled(advice_msg, "false", ""),
>                 "advice should not be printed when config variable is set to false");

... I cannot shake this feeling that the next person who comes to this code and stares at advice.c would be very tempted to "refactor" the messages, so that there is only one instance of the same string in advice.c that is passed to TEST() above. After all, you can change only one place to update the message and avoid triggering test failures that way, right? But that line of thinking obviously reduces the value of having tests.

What if messages from plumbing that should not be modified are being tested with unit tests? These messages are part of contract with users, and it is very much worth our time to write and maintain the tests to ensure they will not be modified by accident. Obviously such a refactoring of test messages to reuse the production strings would destroy the value of having such a test in the first place.

So, I dunno.

Show 9 quoted lines

>> I doubt the issue is not about "picking the right moment" to
>> transition from the test-tool to unit testing framework in this
>> particular case.  Is "check-advice-if-enabled" a bit too high level
>> a feature to be a good match to "unit" testing, I have to wonder?
>
> I don't have a formed opinion on the change, but I don't see it making
> things worse.  I share your doubts, though.
>
> Thanks.