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

["Rubén Justo" <rjusto@gmail.com>]

On 15-ene-2024 09:27:25, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> >> 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 ...
> 
> > @@ -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?

I see.  Maybe you're expecting something like:

diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c
index 15df29c955..15e293fa82 100644
--- a/t/unit-tests/t-advise.c
+++ b/t/unit-tests/t-advise.c
@@ -4,14 +4,15 @@
 #include "setup.h"
 #include "strbuf.h"
 
-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[] = "hint: This is a piece of advice\n";
+static const char expect_hint_msg[] = "hint: Disable this message with \"git config advice.nestedTag false\"\n";
 static const char advice_msg[] = "This is a piece of advice";
 static const char out_file[] = "./output.txt";
 
 
 static void check_advise_if_enabled(const char *argv, const char *conf_val, const char *expect) {
 	FILE *file;
+	const char *hint;
 	struct strbuf actual = STRBUF_INIT;
 
 	if (conf_val)
@@ -32,7 +33,9 @@ static void check_advise_if_enabled(const char *argv, const char *conf_val, cons
 		return;
 	}
 
-	check_str(actual.buf, expect);
+	check_str_len(actual.buf, expect, strlen(expect));
+	if (!conf_val && skip_prefix(actual.buf, expect, &hint))
+		check_str_len(hint, expect_hint_msg, strlen(expect_hint_msg));
 	strbuf_release(&actual);
 
 	if (!check(remove(out_file) == 0))

This implies a new check_str_len() helper, which I'm not including here
but it's a trivial copy of check_str() but using strncmp().

Maybe we can turn the screw a little more.

I'm still not sure of the value in the changes in this series, though.