Re: [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
On 15-ene-2024 09:27:25, Junio C Hamano wrote:
Show 43 quoted lines
> 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.
Rubén Justo <rjusto@gmail.com>