On 12-ene-2024 14:44:37, Junio C Hamano wrote: > Achu Luma <ach.lumap@xxxxxxxxx> 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@xxxxxxxxx/ > > 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.
