[2/2] apply: rewrite unit tests with structured cases

Commit Message

Philip Peterson Feb. 19, 2024, 4:45 a.m. UTC

From: Philip Peterson <[e-mail]>

The imperative format was a little hard to read, so I rewrote the test cases
in a declarative style by defining a common structure for each test case and
its assertions.

Signed-off-by: Philip Peterson <[e-mail]>
---
 t/unit-tests/t-apply.c | 123 ++++++++++++++++++++++++++---------------
 1 file changed, 78 insertions(+), 45 deletions(-)

Comments

Junio C Hamano Feb. 19, 2024, 9:49 p.m. UTC | #1

"Philip Peterson via GitGitGadget" <[e-mail]> writes:

> From: Philip Peterson <[e-mail]>
>
> The imperative format was a little hard to read, so I rewrote the test cases
> in a declarative style by defining a common structure for each test case and
> its assertions.
>
> Signed-off-by: Philip Peterson <[e-mail]>
> ---
>  t/unit-tests/t-apply.c | 123 ++++++++++++++++++++++++++---------------
>  1 file changed, 78 insertions(+), 45 deletions(-)

In this project, we do not add a version of a known-to-be-bad file
in patch [1/2], only to be immediately improved in patch [2/2].
Unless, of course, there is a good reason to do so.  And "to
preserve the true history of what happened in the developer's
working tree" is not a good reason.

We give our developers "rebase -i" and other means to rewrite their
Git history, not just because we want them to be able to pretend
that they are a better developer who never make such a mistake or
misdesign in the first place, but because a polished history is
easier to review in the shorter term and to maintain in the longer
term.

Thanks.

Kristoffer Haugsbakk Feb. 19, 2024, 10:04 p.m. UTC | #2

On Mon, Feb 19, 2024, at 05:45, Philip Peterson via GitGitGadget wrote:
> From: Philip Peterson <[e-mail]>
>
> The imperative format was a little hard to read, so I rewrote the test cases
> in a declarative style by defining a common structure for each test case and
> its assertions.
>
> Signed-off-by: Philip Peterson <[e-mail]>

IMO in general you can just assert that X and Y in the commit message.

  “ The imperative format is hard to read. Rewrite the test cases …

If your patch passes review and is merged then that’s the truth as
determined by you and the reviewers.

More subjective-sounding “This was hard to read” and maybe anecdotes
like “this tripped me up when reading” can go outside the commit message
like the cover letter or the free-form space between the commit message
and the patch (after the three-hyphen/three-dash lines).

Patch

diff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c
index ff0abfb2e0b..2b78624b690 100644
--- a/t/unit-tests/t-apply.c
+++ b/t/unit-tests/t-apply.c
@@ -3,65 +3,98 @@ 
 
 #define FAILURE -1
 
-static void setup_static(const char *line, int len, int offset,
-						 const char *expect, int assert_result,
-						 unsigned long assert_p1,
-						 unsigned long assert_p2)
+typedef struct test_case {
+	const char *line;
+	const char *expect_suffix;
+	int offset;
+	unsigned long expect_p1;
+	unsigned long expect_p2;
+	int expect_result;
+} test_case;
+
+static void setup_static(struct test_case t)
 {
 	unsigned long p1 = 9999;
 	unsigned long p2 = 9999;
-	int result = parse_fragment_range(line, len, offset, expect, &p1, &p2);
-	check_int(result, ==, assert_result);
-	check_int(p1, ==, assert_p1);
-	check_int(p2, ==, assert_p2);
+	int result = parse_fragment_range(t.line, strlen(t.line), t.offset, t.expect_suffix, &p1, &p2);
+	check_int(result, ==, t.expect_result);
+	check_int(p1, ==, t.expect_p1);
+	check_int(p2, ==, t.expect_p2);
 }
 
 int cmd_main(int argc, const char **argv)
 {
-	char* text;
-	int expected_result;
-
-	/* Success */
-	text = "@@ -4,4 +";
-	expected_result = 9;
-	TEST(setup_static(text, strlen(text), 4, " +", expected_result, 4, 4),
-		 "well-formed range");
+	TEST(setup_static((struct test_case) {
+		.line = "@@ -4,4 +",
+		.offset = 4,
+		.expect_suffix = " +",
+		.expect_result = 9,
+		.expect_p1 = 4,
+		.expect_p2 = 4
+	}), "well-formed range");
 
-	text = "@@ -4 +8 @@";
-	expected_result = 7;
-	TEST(setup_static(text, strlen(text), 4, " +", expected_result, 4, 1),
-		 "non-comma range");
+	TEST(setup_static((struct test_case) {
+		.line = "@@ -4 +8 @@",
+		.offset = 4,
+		.expect_suffix = " +",
+		.expect_result = 7,
+		.expect_p1 = 4,
+		.expect_p2 = 1
+	}), "non-comma range");
 
-	/* Failure */
-	text = "@@ -X,4 +";
-	expected_result = FAILURE;
-	TEST(setup_static(text, strlen(text), 4, " +", expected_result, 9999, 9999),
-		 "non-digit range (first coordinate)");
+	TEST(setup_static((struct test_case) {
+		.line = "@@ -X,4 +",
+		.offset = 4,
+		.expect_suffix = " +",
+		.expect_result = FAILURE,
+		.expect_p1 = 9999,
+		.expect_p2 = 9999
+	}), "non-digit range (first coordinate)");
 
-	text = "@@ -4,X +";
-	expected_result = FAILURE;
-	TEST(setup_static(text, strlen(text), 4, " +", expected_result, 4, 1), // p2 is 1, a little strange but not catastrophic
-		 "non-digit range (second coordinate)");
+	TEST(setup_static((struct test_case) {
+		.line = "@@ -4,X +",
+		.offset = 4,
+		.expect_suffix = " +",
+		.expect_result = FAILURE,
+		.expect_p1 = 4,
+		.expect_p2 = 1 // A little strange this is 1, but not end of the world
+	}), "non-digit range (second coordinate)");
 
-	text = "@@ -4,4 -";
-	expected_result = FAILURE;
-	TEST(setup_static(text, strlen(text), 4, " +", expected_result, 4, 4),
-		 "non-expected trailing text");
+	TEST(setup_static((struct test_case) {
+		.line = "@@ -4,4 -",
+		.offset = 4,
+		.expect_suffix = " +",
+		.expect_result = FAILURE,
+		.expect_p1 = 4,
+		.expect_p2 = 4
+	}), "non-expected trailing text");
 
-	text = "@@ -4,4";
-	expected_result = FAILURE;
-	TEST(setup_static(text, strlen(text), 4, " +", expected_result, 4, 4),
-		 "not long enough for expected trailing text");
+	TEST(setup_static((struct test_case) {
+		.line = "@@ -4,4",
+		.offset = 4,
+		.expect_suffix = " +",
+		.expect_result = FAILURE,
+		.expect_p1 = 4,
+		.expect_p2 = 4
+	}), "not long enough for expected trailing text");
 
-	text = "@@ -4,4";
-	expected_result = FAILURE;
-	TEST(setup_static(text, strlen(text), 7, " +", expected_result, 9999, 9999),
-		 "not long enough for offset");
+	TEST(setup_static((struct test_case) {
+		.line = "@@ -4,4",
+		.offset = 7,
+		.expect_suffix = " +",
+		.expect_result = FAILURE,
+		.expect_p1 = 9999,
+		.expect_p2 = 9999
+	}), "not long enough for offset");
 
-	text = "@@ -4,4";
-	expected_result = FAILURE;
-	TEST(setup_static(text, strlen(text), -1, " +", expected_result, 9999, 9999),
-		 "negative offset");
+	TEST(setup_static((struct test_case) {
+		.line = "@@ -4,4",
+		.offset = -1,
+		.expect_suffix = " +",
+		.expect_result = FAILURE,
+		.expect_p1 = 9999,
+		.expect_p2 = 9999
+	}), "negative offset");
 
 	return test_done();
 }