Message ID | 20240112102122.1422-1-ach.lumap@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [Outreachy] Port helper/test-advise.c to unit-tests/t-advise.c | expand |
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? 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? Thanks.
On 12-ene-2024 14:44:37, Junio C Hamano wrote: > 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> 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? 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. >> 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.
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.
On 15-ene-2024 20:24:47, Rubén Justo wrote: > - 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. I hope, no one has wasted time with the code above. It is not testing correctly the conditions being probed in t-advice.c. Take it as a way to see if this is how we want to avoid multiple instances of similar literals that might be tempting to refactor.
diff --git a/Makefile b/Makefile index 15990ff312..27916e4341 100644 --- a/Makefile +++ b/Makefile @@ -783,7 +783,6 @@ X = PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS)) -TEST_BUILTINS_OBJS += test-advise.o TEST_BUILTINS_OBJS += test-bitmap.o TEST_BUILTINS_OBJS += test-bloom.o TEST_BUILTINS_OBJS += test-bundle-uri.o @@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/% UNIT_TEST_PROGRAMS += t-basic UNIT_TEST_PROGRAMS += t-mem-pool UNIT_TEST_PROGRAMS += t-strbuf +UNIT_TEST_PROGRAMS += t-advise UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c deleted file mode 100644 index 8a3fd0009a..0000000000 --- a/t/helper/test-advise.c +++ /dev/null @@ -1,22 +0,0 @@ -#include "test-tool.h" -#include "advice.h" -#include "config.h" -#include "setup.h" - -int cmd__advise_if_enabled(int argc, const char **argv) -{ - if (argc != 2) - die("usage: %s <advice>", argv[0]); - - setup_git_directory(); - git_config(git_default_config, NULL); - - /* - * Any advice type can be used for testing, but NESTED_TAG was - * selected here and in t0018 where this command is being - * executed. - */ - advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[1]); - - return 0; -} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 37ba996539..e7f7326ce6 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -10,7 +10,6 @@ static const char * const test_tool_usage[] = { }; static struct test_cmd cmds[] = { - { "advise", cmd__advise_if_enabled }, { "bitmap", cmd__bitmap }, { "bloom", cmd__bloom }, { "bundle-uri", cmd__bundle_uri }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 8a1a7c63da..68751dda66 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -3,7 +3,6 @@ #include "git-compat-util.h" -int cmd__advise_if_enabled(int argc, const char **argv); int cmd__bitmap(int argc, const char **argv); int cmd__bloom(int argc, const char **argv); int cmd__bundle_uri(int argc, const char **argv); diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh deleted file mode 100755 index c13057a4ca..0000000000 --- a/t/t0018-advice.sh +++ /dev/null @@ -1,33 +0,0 @@ -#!/bin/sh - -test_description='Test advise_if_enabled functionality' - -TEST_PASSES_SANITIZE_LEAK=true -. ./test-lib.sh - -test_expect_success 'advice should be printed when config variable is unset' ' - cat >expect <<-\EOF && - hint: This is a piece of advice - hint: Disable this message with "git config advice.nestedTag false" - EOF - test-tool advise "This is a piece of advice" 2>actual && - test_cmp expect actual -' - -test_expect_success 'advice should be printed when config variable is set to true' ' - cat >expect <<-\EOF && - hint: This is a piece of advice - hint: Disable this message with "git config advice.nestedTag false" - EOF - test_config advice.nestedTag true && - test-tool advise "This is a piece of advice" 2>actual && - test_cmp expect actual -' - -test_expect_success 'advice should not be printed when config variable is set to false' ' - test_config advice.nestedTag false && - test-tool advise "This is a piece of advice" 2>actual && - test_must_be_empty actual -' - -test_done diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c new file mode 100644 index 0000000000..15df29c955 --- /dev/null +++ b/t/unit-tests/t-advise.c @@ -0,0 +1,53 @@ +#include "test-lib.h" +#include "advice.h" +#include "config.h" +#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 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; + struct strbuf actual = STRBUF_INIT; + + if (conf_val) + git_default_advice_config("advice.nestedTag", conf_val); + + file = freopen(out_file, "w", stderr); + if (!check(!!file)) { + test_msg("Error opening file %s", out_file); + return; + } + + advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv); + fclose(file); + + if (!check(strbuf_read_file(&actual, out_file, 0) >= 0)) { + test_msg("Error reading file %s", out_file); + strbuf_release(&actual); + return; + } + + check_str(actual.buf, expect); + strbuf_release(&actual); + + if (!check(remove(out_file) == 0)) + test_msg("Error deleting %s", out_file); +} + +int cmd_main(int argc, const char **argv) { + setenv("TERM", "dumb", 1); + + 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), + "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"); + + return test_done(); +}
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_*()). Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Achu Luma <ach.lumap@gmail.com> --- Makefile | 2 +- t/helper/test-advise.c | 22 ----------------- t/helper/test-tool.c | 1 - t/helper/test-tool.h | 1 - t/t0018-advice.sh | 33 ------------------------- t/unit-tests/t-advise.c | 53 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 54 insertions(+), 58 deletions(-) delete mode 100644 t/helper/test-advise.c delete mode 100755 t/t0018-advice.sh create mode 100644 t/unit-tests/t-advise.c -- 2.42.0.windows.2