Re: [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
On 12-ene-2024 14:44:37, Junio C Hamano wrote:
Show 32 quoted lines
> Achu Luma <ach.lumap@gmail.com> writes:
>
> > In the recent codebase update (8bf6fbd00d (Merge branch
> > 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> > merged, providing a standardized approach for testing C code. Prior to
> > this update, some unit tests relied on the test helper mechanism,
> > lacking a dedicated unit testing framework. It's more natural to perform
> > these unit tests using the new unit test framework.
> >
> > This commit migrates the unit tests for advise_if_enabled functionality
> > from the legacy approach using the test-tool command `test-tool advise`
> > in t/helper/test-advise.c to the new unit testing framework
> > (t/unit-tests/test-lib.h).
> >
> > The migration involves refactoring the tests to utilize the testing
> > macros provided by the framework (TEST() and check_*()).
>
> In the light of the presense of work like this one
>
> https://lore.kernel.org/git/c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com/
>
> I am not sure if moving this to unit-test framework is a good thing,
> at least right at this moment.
>
> 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";
@@ -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 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.
Rubén Justo <rjusto@gmail.com>