{"thread":{"id":"60340","subject":"[PATCH] t-progress.c : unit tests for progress.c","startedAt":"2023-10-11T08:27:32Z","lastAt":"2023-10-11T17:30:01Z","messageCount":2,"participants":["Siddharth Singh","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"483038","messageId":"20231011082716.901048-1-siddhartth@google.com","threadId":"60340","inReplyTo":null,"subject":"[PATCH] t-progress.c : unit tests for progress.c","fromName":"Siddharth Singh","fromEmail":"siddhartth@google.com","sentAt":"2023-10-11T08:27:08Z","receivedAt":"2023-10-11T08:27:32Z","isPatch":true,"sender":{"key":"siddhartth@google.com","avatar":"https://avatars.githubusercontent.com/u/68241942?v=4"},"body":"These tests are directly inspired from the tests inside\nt/t0500-progress-display.sh\n\nThe existing shell tests for the Git progress library only test the output of the library, not the correctness of the progress struct fields. Unit tests can fill this gap and improve confidence that the library works as expected. For example, unit tests can verify that the progress struct fields are updated correctly when the library is used.\n\nChange-Id: I190522f29fdab9291af71b7788eeee2c0f26282d\nSigned-off-by: Siddharth Singh <siddhartth@google.com>\n---\n\nDear Git Community,\nAs you may be aware, my colleague Josh is proposing a unit testing framework[1] on the mailing list. I attempted to use the latest version of that series to convert t/helper/test-progress.c to unit tests. However, while writing the tests, I realized that the way progress.c is implemented makes it very difficult to test it in units.\n\nFirstly, most unit tests are typically written as a method that takes the expected output and the actual output of the unit being tested, and compares them for equality. However, because it's intended to print user-facing output on an interactive terminal, progress.c prints everything out to stderr, which makes it difficult to unit test.\n\nAs written, a unit test for throughput progress printing would have to capture stdout from the library and compare it to the expected output, which is difficult to implement and unusual for unit tests anyway.\n\nThere are a few ways to work around this issue in my opinion. One way is to modify the library that does not print to output stream and returns the data to the caller:\n\nstatic void display(struct progress *progress, uint64_t n, char *done){\n…\n}\n\nbecomes\n\nstruct strbuf display(struct progress *progress,uint64_t n,char *done){\n…\n}\n\nAnother way is to capture the output from the library into a file instead of stdout and then read it from the file and compare it to the expected output, However, this is a difficult task, and I recommend against it.\n\nWe may need to make some changes to the way these libraries are implemented in order to make them unit testable in the future.\n\nTherefore, i want to ask if it's worth investing time in developing a solution?\n\n\nI look forward to hearing your thoughts on this.\n[1] https://lore.kernel.org/git/cover.1692297001.git.steadmon@google.com/\n\n\n\n Makefile                  |   1 +\n t/unit-tests/.gitignore   |   1 +\n t/unit-tests/t-progress.c | 229 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 231 insertions(+)\n create mode 100644 t/unit-tests/t-progress.c\n\ndiff --git a/Makefile b/Makefile\nindex 4016da6e39..eabbfe32cf 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1335,6 +1335,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n \n UNIT_TEST_PROGRAMS += t-basic\n UNIT_TEST_PROGRAMS += t-strbuf\n+UNIT_TEST_PROGRAMS += t-progress\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_DIR)/%$X,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\ndiff --git a/t/unit-tests/.gitignore b/t/unit-tests/.gitignore\nindex e292d58348..cf5eb4803e 100644\n--- a/t/unit-tests/.gitignore\n+++ b/t/unit-tests/.gitignore\n@@ -1,2 +1,3 @@\n /t-basic\n /t-strbuf\n+/t-progress\ndiff --git a/t/unit-tests/t-progress.c b/t/unit-tests/t-progress.c\nnew file mode 100644\nindex 0000000000..437d6acf16\n--- /dev/null\n+++ b/t/unit-tests/t-progress.c\n@@ -0,0 +1,229 @@\n+#include \"test-lib.h\"\n+#include \"progress.c\"\n+\n+\n+static void t_simple_progress()\n+{\n+\tint total = 4;\n+\tstruct progress *progress = NULL;\n+\tint i;\n+\tprogress = start_progress(\"Working hard\", total);\n+\tfor (i = 1; i <= total; i++) {\n+\t\tdisplay_progress(progress, i);\n+\t\tcheck_uint(i, ==, progress->last_value);\n+\t\tcheck_str(progress->title, \"Working hard\");\n+\t\tcheck_int(progress->last_percent, ==, i * 100 / total);\n+\t}\n+\treturn;\n+}\n+\n+static void t_simple_progress_percent_text()\n+{\n+\tint total = 4;\n+\tstruct progress *progress = NULL;\n+\tint i;\n+\tchar *expected[] = {\n+\t\t\"  0% (0/4)\",\n+\t\t\" 25% (1/4)\",\n+\t\t\" 50% (2/4)\",\n+\t\t\" 75% (3/4)\",\n+\t\t\"100% (4/4)\"\n+\t\t};\n+\tchar *instructions[] = {\n+\t\t\"progress\",\n+\t\t\"progress\",\n+\t\t\"progress\",\n+\t\t\"progress\",\n+\t\t\"progress\"\n+\t};\n+\tint value[] = {\n+\t\t0,\n+\t\t1,\n+\t\t2,\n+\t\t3,\n+\t\t4\n+\t};\n+\tprogress = start_progress(\"Working hard\", total);\n+\tfor (i = 0; i < 5; i++) {\n+\t\tif(strcmp(instructions[i], \"progress\")==0){\n+\t\t\tdisplay_progress(progress, value[i]);\n+\t\t\tcheck_str(progress->title, \"Working hard\");\n+\t\t\tcheck_str(progress->counters_sb.buf, expected[i]);\n+\t\t\tcheck_uint(i * (100 / total), ==, progress->last_percent);\n+\t\t}\n+\t}\n+\treturn;\n+}\n+\n+static void t_progress_display_breaks_long_lines_1()\n+{\n+\tint total = 100000;\n+\tstruct progress *progress = NULL;\n+\tint i;\n+\tchar *expected[4] = {\n+\t\t\"  0% (100/100000)\",\n+\t\t\"  1% (1000/100000)\",\n+\t\t\" 10% (10000/100000)\",\n+\t\t\"100% (100000/100000)\"\n+\t};\n+\tchar *instructions[] = {\n+\t\t\"progress\",\n+\t\t\"progress\",\n+\t\t\"progress\",\n+\t\t\"progress\"\n+\t};\n+\tint value[] = {\n+\t\t100,\n+\t\t1000,\n+\t\t10000,\n+\t\t100000\n+\t};\n+\tprogress = start_progress(\n+\t\t\"Working hard.......2.........3.........4.........5.........6\",\n+\t\ttotal);\n+\tfor (i = 0; i < 4; i++) {\n+\t\tif(strcmp(instructions[i], \"progress\")==0){\n+\t\t\tdisplay_progress(progress, value[i]);\n+\t\t}\n+\t\tcheck_str(progress->title, \"Working hard.......2.........3.........4.........5.........6\");\n+\t\tcheck_str(progress->counters_sb.buf, expected[i]);\n+\t}\n+\treturn;\n+}\n+\n+static void t_progress_display_breaks_long_lines_2()\n+{\n+\tint total = 100000;\n+\tstruct progress *progress = NULL;\n+\tint i;\n+\tchar *expected[] = {\n+\t\t\"\",\n+\t\t\"  0% (1/100000)\",\n+\t\t\"\",\n+\t\t\"  0% (2/100000)\",\n+\t\t\" 10% (10000/100000)\",\n+\t\t\"100% (100000/100000)\"\n+\t};\n+\tchar *instructions[] = {\n+\t\t\"update\",\n+\t\t\"progress\",\n+\t\t\"update\",\n+\t\t\"progress\",\n+\t\t\"progress\",\n+\t\t\"progress\"\n+\t};\n+\tint value[] = {\n+\t\t-1,\n+\t\t1,\n+\t\t-1,\n+\t\t2,\n+\t\t10000,\n+\t\t100000\n+\t};\n+\tprogress = start_progress(\n+\t\t\"Working hard.......2.........3.........4.........5.........6\",\n+\t\ttotal);\n+\tfor (i = 0; i < 5; i++) {\n+\t\tif(strcmp(instructions[i], \"progress\")==0){\n+\t\t\tdisplay_progress(progress, value[i]);\n+\t\t\tcheck_str(progress->title, \"Working hard.......2.........3.........4.........5.........6\");\n+\t\t\tcheck_str(progress->counters_sb.buf, expected[i]);\n+\t\t}else if(strcmp(instructions[i], \"update\")==0){\n+\t\t\tprogress_test_force_update();\n+\t\t}\n+\t}\n+\treturn;\n+}\n+\n+static void t_progress_display_breaks_long_lines_3()\n+{\n+\tint total = 100000;\n+\tstruct progress *progress = NULL;\n+\tint i;\n+\tchar *expected[4] = {\n+\t\t\" 25% (25000/100000)\",\n+\t\t\" 50% (50000/100000)\",\n+\t\t\" 75% (75000/100000)\",\n+\t\t\"100% (100000/100000)\"\n+\t};\n+\tchar *instructions[] = {\n+\t\t\"progress\",\n+\t\t\"progress\",\n+\t\t\"progress\",\n+\t\t\"progress\"\n+\t};\n+\tint value[] = {\n+\t\t25000,\n+\t\t50000,\n+\t\t75000,\n+\t\t100000\n+\t};\n+\tprogress = start_progress(\n+\t\t\"Working hard.......2.........3.........4.........5.........6\",\n+\t\ttotal);\n+\tfor (i = 0; i < 4; i++) {\n+\t\tif(strcmp(instructions[i], \"progress\")==0){\n+\t\t\tdisplay_progress(progress, value[i]);\n+\t\t\tcheck_str(progress->title, \"Working hard.......2.........3.........4.........5.........6\");\n+\t\t\tcheck_str(progress->counters_sb.buf, expected[i]);\n+\t\t}else if(strcmp(instructions[i], \"update\")==0){\n+\t\t\tprogress_test_force_update();\n+\t\t}\n+\t}\n+\treturn;\n+}\n+\n+\n+static void t_progress_shortens_crazy_caller()\n+{\n+\tint total = 1000;\n+\tstruct progress *progress = NULL;\n+\tint i;\n+\tchar *expected[4] = {\n+\t\t\" 10% (100/1000)\",\n+\t\t\" 20% (200/1000)\",\n+\t\t\"  0% (1/1000)\",\n+\t\t\"100% (1000/1000)\"\n+\t};\n+\tchar *instructions[] = {\n+\t\t\"progress\",\n+\t\t\"progress\",\n+\t\t\"progress\",\n+\t\t\"progress\"\n+\t};\n+\tint value[] = {\n+\t\t100,\n+\t\t200,\n+\t\t1,\n+\t\t1000\n+\t};\n+\tprogress = start_progress(\n+\t\t\"Working hard.......2.........3.........4.........5.........6\",\n+\t\ttotal);\n+\tfor (i = 0; i < 4; i++) {\n+\t\tif(strcmp(instructions[i], \"progress\")==0){\n+\t\t\tdisplay_progress(progress, value[i]);\n+\t\t\tcheck_str(progress->title, \"Working hard.......2.........3.........4.........5.........6\");\n+\t\t\tcheck_str(progress->counters_sb.buf, expected[i]);\n+\t\t}else if(strcmp(instructions[i], \"update\")==0){\n+\t\t\tprogress_test_force_update();\n+\t\t}\n+\t}\n+\treturn;\n+}\n+\n+int cmd_main(int argc, const char **argv)\n+{\n+\tTEST(t_simple_progress(), \"Simple progress upto 3 units\");\n+\tTEST(t_simple_progress_percent_text(),\n+\t     \"Simple progress with percent output\");\n+\tTEST(t_progress_display_breaks_long_lines_1(),\n+\t     \"progress display breaks long lines #1\");\n+\tTEST(t_progress_display_breaks_long_lines_2(),\n+\t     \"progress display breaks long lines #2\");\n+\tTEST(t_progress_display_breaks_long_lines_3(),\n+\t     \"progress display breaks long lines #3\");\n+\tTEST(t_progress_shortens_crazy_caller(),\n+\t     \"progress shortens - crazy caller\");\n+\treturn test_done();\n+}\n-- \n2.40.GIT\n\n"},{"id":"483063","messageId":"xmqq34yhm5in.fsf@gitster.g","threadId":"60340","inReplyTo":"20231011082716.901048-1-siddhartth@google.com","subject":"Re: [PATCH] t-progress.c : unit tests for progress.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-11T17:29:52Z","receivedAt":"2023-10-11T17:30:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Singh <siddhartth@google.com> writes:\n\n> These tests are directly inspired from the tests inside\n> t/t0500-progress-display.sh\n>\n> The existing shell tests for the Git progress library only test the output of the library, not the correctness of the progress struct fields. Unit tests can fill this gap and improve confidence that the library works as expected. For example, unit tests can verify that the progress struct fields are updated correctly when the library is used.\n>\n> Change-Id: I190522f29fdab9291af71b7788eeee2c0f26282d\n> Signed-off-by: Siddharth Singh <siddhartth@google.com>\n> ---\n\nThe contents is of course important, but please also pay attention\nto the formatting to make what you write readable.  Writing good\nthings does not help if it is not read.  Wrap lines to appropriate\nlengths.\n\nDocumentation/SubmittingPatches is your friend.\n\n>\n> Dear Git Community,\n\n> As you may be aware, my colleague Josh is proposing a unit testing\n> framework[1] on the mailing list. I attempted to use the latest\n> version of that series to convert t/helper/test-progress.c to unit\n> tests. However, while writing the tests, I realized that the way\n> progress.c is implemented makes it very difficult to test it in\n> units.\n>\n> Firstly, most unit tests are typically written as a method that\n> takes the expected output and the actual output of the unit being\n> tested, and compares them for equality. However, because it's\n> intended to print user-facing output on an interactive terminal,\n> progress.c prints everything out to stderr, which makes it\n> difficult to unit test.\n\nIt sounds like you found where the test framework is lacking.  If\nmaking sure what we spew out to the standard error stream is worth\ncovering in the unit tests, the test framework needs to support it,\nright?\n\n\n> There are a few ways to work around this issue in my opinion. One\n> way is to modify the library that does not print to output stream\n> and returns the data to the caller:\n>\n> static void display(struct progress *progress, uint64_t n, char *done){\n> …\n> }\n>\n> becomes\n>\n> struct strbuf display(struct progress *progress,uint64_t n,char *done){\n> …\n> }\n\nThe approach adds a feature for outside callers to access this bit\nof internal implementation detail of the progress code.  If no real\ncallers want to exercise that feature and it is only useful for\ntesting, it smells like the tail wagging the dog, though.\n\nIt certainly is not worth butchering the real code for the sake of\nworking around the lack of current unit test framework to\ncontaminate the global namespace with way too generically named\nfunction \"display()\".\n\nAssuming that you *must* work around the lack of stderr support, it\nprobably would make much more sense to add a new member that points\nat an output stream to \"struct progress\".  Make it a thin wrapper\naround \"FILE *\" that supports whatever display() and the helper\nfunctions it calls needs, like write(2) or fprintf(3).  And you can\nmock that \"output stream\" in your unit tests to append to an in-core\nbuffer if you wanted to.  The production code does not have to care\nabout your mock that way.\n"}]}