Skip to contentSkip to conversation

apply: add unit tests for parse_range

PATCH Superseded Philip Peterson via GitGitGadget 5 participants 12 messages Feb 19, 2024 – Jun 7, 2024 Kbuild

lore
Share & tools
lore

Philip Peterson via GitGitGadget

author
#0 lore raw

cover apply: add unit tests for parse_range

Hello all,

This patchset adds unit tests for apply's parse_range function. Also renames parse_range to parse_fragment_range.

It was necessary to make the function be non-internal linkage in order to expose it to the unit testing framework. Because there is another function in the codebase also called parse_range, I changed this one to a more specific name as well: parse_fragment_range. There are also several test cases added (both positive and negative) for the function.

Thank you for your help,

Philip

Philip Peterson (2):
apply: add unit tests for parse_range and rename to
parse_fragment_range
apply: rewrite unit tests with structured cases

 Makefile               |   1 +
 apply.c                |   8 ++--
 apply.h                |   4 ++
 t/unit-tests/t-apply.c | 100 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 109 insertions(+), 4 deletions(-)
 create mode 100644 t/unit-tests/t-apply.c

base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1677%2Fphilip-peterson%2Fpeterson%2Funit-tests-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1677/philip-peterson/peterson/unit-tests-v1
Pull-Request: https://github.com/git/git/pull/1677

Signature · 1 line
gitgitgadget
author
#1 lore raw

PATCH 1/2 apply: add unit tests for parse_range and rename to parse_fragment_range

From: Philip Peterson <[email protected]>

This patchset makes the parse_range function in apply be non-internal linkage in order to expose to the unit testing framework. In so doing, because there is another function called parse_range, I gave this one a more specific name, parse_fragment_range. Other than that, this commit adds several test cases (positive and negative) for the function.

Signed-off-by Philip Peterson <[email protected]>
---
 Makefile               |  1 +
 apply.c                |  8 ++---
 apply.h                |  4 +++
 t/unit-tests/t-apply.c | 67 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+), 4 deletions(-)
 create mode 100644 t/unit-tests/t-apply.c
Makefile +1 -0
index 15990ff3122..369092aedfe 100644
@@ -1339,6 +1339,7 @@ THIRD_PARTY_SOURCES += compat/regex/%
 THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
+UNIT_TEST_PROGRAMS += t-apply
 UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-strbuf
apply.c +4 -4
index 7608e3301ca..199a1150df6 100644
@@ -1430,8 +1430,8 @@ static int parse_num(const char *line, unsigned long *p)
 	return ptr - line;
 }
 
-static int parse_range(const char *line, int len, int offset, const char *expect,
-		       unsigned long *p1, unsigned long *p2)
+int parse_fragment_range(const char *line, int len, int offset, const char *expect,
+			 unsigned long *p1, unsigned long *p2)
 {
 	int digits, ex;
 

@@ -1530,8 +1530,8 @@ static int parse_fragment_header(const char *line, int len, struct fragment *fra
 		return -1;
 
 	/* Figure out the number of lines in a fragment */
-	offset = parse_range(line, len, 4, " +", &fragment->oldpos, &fragment->oldlines);
-	offset = parse_range(line, len, offset, " @@", &fragment->newpos, &fragment->newlines);
+	offset = parse_fragment_range(line, len, 4, " +", &fragment->oldpos, &fragment->oldlines);
+	offset = parse_fragment_range(line, len, offset, " @@", &fragment->newpos, &fragment->newlines);
 
 	return offset;
 }
apply.h +4 -0
index 7cd38b1443c..bbc5e3caeb5 100644
@@ -187,3 +187,7 @@ int apply_all_patches(struct apply_state *state,
 		      int options);
 
 #endif
+
+
+int parse_fragment_range(const char *line, int len, int offset, const char *expect,
+		       unsigned long *p1, unsigned long *p2);
t/unit-tests/t-apply.c new file +67 -0

67 lines not on this page · raw message

Signature · 1 line
gitgitgadget
#2 lore raw

Re: [PATCH 1/2] apply: add unit tests for parse_range and rename to parse_fragment_range

Quoted 7 lines · Philip Peterson via GitGitGadget

From: Philip Peterson <[email protected]>

This patchset makes the parse_range function in apply be non-internal linkage in order to expose to the unit testing framework. In so doing, because there is another function called parse_range, I gave this one a more specific name, parse_fragment_range. Other than that, this commit adds several test cases (positive and negative) for the function.

We do not write "I did this, I did that" in our proposed log message. In addition, guidance on the proposed commit log in a handful of sections in Documentation/SubmittingPatches would be helpful.

It may probably be a good idea to split this into a preliminary patch that makes a symbol extern (and doing nothing else), and the main patch that adds external caller(s) to the function from a new unit test.

It certainly is better than doing nothing and just make it extern, but I am not sure "fragment" is specific enough to make the symbol clearly belong to "apply" API.

Quoted 5 lines
apply.c +0 -0
index 7608e3301ca..199a1150df6 100644

apply.c static int parse_num(const char *line, unsigned long *p) · line 1436

Quoted 9 lines
apply.c +2 -2
@@ -1430,8 +1430,8 @@ static int parse_num(const char *line, unsigned long *p)
 	return ptr - line;
 }
 
-static int parse_range(const char *line, int len, int offset, const char *expect,
-		       unsigned long *p1, unsigned long *p2)
+int parse_fragment_range(const char *line, int len, int offset, const char *expect,
+			 unsigned long *p1, unsigned long *p2)
 {
 	int digits, ex;

Alternatively we could do something like this to make the blast radius of this patch smaller.

-static int parse_range(const char *line, int len, int offset, const char *expect, +#define apply_parse_fragment_range parse_range
+int parse_range(const char *line, int len, int offset, const char *expect,
unsigned long *p1, unsigned long *p2)

If not for unit-test, this function has no reason to be extern with such a long name, so it is better to allow internal callers to refer to it with the name that has been good enough for them for the past 19 years since it was introduced in fab2c257 (git-apply: make the diffstat output happen for "--stat" only., 2005-05-26).

apply.h int apply_all_patches(struct apply_state *state, · line 193

Quoted 12 lines
apply.h +4 -0
index 7cd38b1443c..bbc5e3caeb5 100644
@@ -187,3 +187,7 @@ int apply_all_patches(struct apply_state *state,
 		      int options);
 
 #endif
+
+
+int parse_fragment_range(const char *line, int len, int offset, const char *expect,
+		       unsigned long *p1, unsigned long *p2);

This is wrong. The #endif is about avoiding double inclusion of this header file and any new declaration must go before it.

t/unit-tests/t-apply.c

Quoted 2 lines
t/unit-tests/t-apply.c new file +0 -0
new file mode 100644

This should go to the next patch.

#3 lore raw

Re: [PATCH 1/2] apply: add unit tests for parse_range and rename to parse_fragment_range

Hi,

Thanks for the tips. I am confused about the change request though:

Review

Quoted 7 lines

Alternatively we could do something like this to make the blast radius of this patch smaller.

(quoted diff) +2 -1
-static int parse_range(const char *line, int len, int offset, const char *expect,
+#define apply_parse_fragment_range parse_range
+int parse_range(const char *line, int len, int offset, const char *expect,
                        unsigned long *p1, unsigned long *p2)

From what I understand, this still creates a new extern symbol called parse_range. The hope was to avoid creating a new symbol with a generic name that someone might start to consume, because if they did start consuming that symbol, it would be a burden on them when we eventually have to rename it due to the conflict with the other symbol currently called parse_range in add-patch.c, which we might also want to add unit tests for later. Is it not preferable to just rename it now, before it becomes extern?

Cheers,
Phil

Quoted 74 lines · Junio C Hamano
Quoted 7 lines · Philip Peterson via GitGitGadget

From: Philip Peterson <[email protected]>

This patchset makes the parse_range function in apply be non-internal linkage in order to expose to the unit testing framework. In so doing, because there is another function called parse_range, I gave this one a more specific name, parse_fragment_range. Other than that, this commit adds several test cases (positive and negative) for the function.

We do not write "I did this, I did that" in our proposed log message. In addition, guidance on the proposed commit log in a handful of sections in Documentation/SubmittingPatches would be helpful.

It may probably be a good idea to split this into a preliminary patch that makes a symbol extern (and doing nothing else), and the main patch that adds external caller(s) to the function from a new unit test.

It certainly is better than doing nothing and just make it extern, but I am not sure "fragment" is specific enough to make the symbol clearly belong to "apply" API.

Quoted 5 lines
apply.c +0 -0
index 7608e3301ca..199a1150df6 100644

apply.c static int parse_num(const char *line, unsigned long *p) · line 1436

Quoted 9 lines
apply.c +2 -2
@@ -1430,8 +1430,8 @@ static int parse_num(const char *line, unsigned long *p)
      return ptr - line;
 }
 
-static int parse_range(const char *line, int len, int offset, const char *expect,
-                    unsigned long *p1, unsigned long *p2)
+int parse_fragment_range(const char *line, int len, int offset, const char *expect,
+                      unsigned long *p1, unsigned long *p2)
 {
      int digits, ex;

Alternatively we could do something like this to make the blast radius of this patch smaller.

-static int parse_range(const char *line, int len, int offset, const char *expect, +#define apply_parse_fragment_range parse_range
+int parse_range(const char *line, int len, int offset, const char *expect,
unsigned long *p1, unsigned long *p2)

If not for unit-test, this function has no reason to be extern with such a long name, so it is better to allow internal callers to refer to it with the name that has been good enough for them for the past 19 years since it was introduced in fab2c257 (git-apply: make the diffstat output happen for "--stat" only., 2005-05-26).

apply.h int apply_all_patches(struct apply_state *state, · line 193

Quoted 12 lines
apply.h +4 -0
index 7cd38b1443c..bbc5e3caeb5 100644
@@ -187,3 +187,7 @@ int apply_all_patches(struct apply_state *state,
                    int options);
 
 #endif
+
+
+int parse_fragment_range(const char *line, int len, int offset, const char *expect,
+                    unsigned long *p1, unsigned long *p2);

This is wrong. The #endif is about avoiding double inclusion of this header file and any new declaration must go before it.

t/unit-tests/t-apply.c

Quoted 2 lines
t/unit-tests/t-apply.c new file +0 -0
new file mode 100644

This should go to the next patch.

#4 lore raw

Re: [PATCH 1/2] apply: add unit tests for parse_range and rename to parse_fragment_range

Quoted 3 lines · Philip
Quoted 2 lines · Junio C Hamano

"Philip Peterson via GitGitGadget" <[email protected]> writes:

It's quite a blast from a long time ago that I no longer remember.

Quoted 10 lines

Review

Quoted 7 lines

Alternatively we could do something like this to make the blast radius of this patch smaller.

(quoted diff) +2 -1
-static int parse_range(const char *line, int len, int offset, const char *expect,
+#define apply_parse_fragment_range parse_range
+int parse_range(const char *line, int len, int offset, const char *expect,
                        unsigned long *p1, unsigned long *p2)

From what I understand, this still creates a new extern symbol called parse_range.

Sorry, I misspoke. The direction of #define is the other way around. That is, we may have to give the function a name that is overly long because it needs to be externally linkable only to support for your test, but we would want to locally rename that long name down to the name currently used by the primary callers of that function, so that the patch does not have to touch these existing calling sites. After all, this function is a small implementation detail and not a part of the official apply.c API, and the only reason why we are making it extern is because some new tests want to link with it from the side.

So, in the <apply.h> header file you'll do

	/* 
	 * exposed only for tests; do not call this as it not
	 * a part of the API
	 */
	extern int apply_parse_fragment_range(...);

and then in the original file that used to have "static int parse_range(...)", you'd add

	#define parse_range apply_parse_fragment_range

near the top, after the header files are included.

Thanks.

author
#5 lore raw

PATCH 2/2 apply: rewrite unit tests with structured cases

In reply to Philip Peterson via GitGitGadget

From: Philip Peterson <[email protected]>

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 <[email protected]>
---
 t/unit-tests/t-apply.c | 123 ++++++++++++++++++++++++++---------------
 1 file changed, 78 insertions(+), 45 deletions(-)
t/unit-tests/t-apply.c +78 -45
index ff0abfb2e0b..2b78624b690 100644
@@ -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();
 }
Signature · 1 line
gitgitgadget
#6 lore raw

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

Quoted 10 lines · Philip Peterson via GitGitGadget

From: Philip Peterson <[email protected]>

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 <[email protected]>
---
 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.

#7 lore raw

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

In reply to Philip Peterson via GitGitGadget
Quoted 7 lines · Philip Peterson via GitGitGadget

From: Philip Peterson <[email protected]>

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 <[email protected]>

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).

Signature · 1 line
Kristoffer Haugsbakk
author
#8 lore raw

PATCH v2 1/1 apply: add unit tests for parse_range

In reply to Philip Peterson via GitGitGadget

From: Philip Peterson <[email protected]>

Also rename parse_range to parse_fragment_range for external linkage.

Signed-off-by Philip Peterson <[email protected]>

---
apply: add unit tests for parse_range

Add unit tests for apply's parse_range function. Also rename parse_range to parse_fragment_range and expose it externally for use by the unit tests. Internal callers may continue to refer to it by the name parse_range.

It is necessary to make the function be non-internal linkage in order to expose it to the unit testing framework. Because there is another function in the codebase also called parse_range, change this one to a more specific name as well: parse_fragment_range. Also add several test cases (both positive and negative) for the function.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1677%2Fphilip-peterson%2Fpeterson%2Funit-tests-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1677/philip-peterson/peterson/unit-tests-v2
Pull-Request: https://github.com/git/git/pull/1677

Range-diff vs v1:

 1:  2c60c4406d4 < -:  ----------- apply: add unit tests for parse_range and rename to parse_fragment_range
 2:  7dab12ab7b8 ! 1:  c0d7b93e383 apply: rewrite unit tests with structured cases
     @@ Metadata
      Author: Philip Peterson <[email protected]>
      
       ## Commit message ##
     -    apply: rewrite unit tests with structured cases
     +    apply: add unit tests for parse_range
      
     -    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.
     +    Also rename parse_range to parse_fragment_range for external linkage.
      
          Signed-off-by: Philip Peterson <[email protected]>
      
     - ## t/unit-tests/t-apply.c ##
     + ## Makefile ##
     +@@ Makefile: THIRD_PARTY_SOURCES += compat/regex/%
     + THIRD_PARTY_SOURCES += sha1collisiondetection/%
     + THIRD_PARTY_SOURCES += sha1dc/%
     + 
     ++UNIT_TEST_PROGRAMS += t-apply
     + UNIT_TEST_PROGRAMS += t-ctype
     + UNIT_TEST_PROGRAMS += t-mem-pool
     + UNIT_TEST_PROGRAMS += t-prio-queue
     +
     + ## apply.c ##
      @@
     + #include "wildmatch.h"
     + #include "ws.h"
       
     - #define FAILURE -1
     ++#define parse_range apply_parse_fragment_range
     ++
     + struct gitdiff_data {
     + 	struct strbuf *root;
     + 	int linenr;
     +@@ apply.c: static int parse_num(const char *line, unsigned long *p)
     + 	return ptr - line;
     + }
       
     --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)
     +-static int parse_range(const char *line, int len, int offset, const char *expect,
     +-		       unsigned long *p1, unsigned long *p2)
     ++int apply_parse_fragment_range(const char *line, int len, int offset, const char *expect,
     ++			 unsigned long *p1, unsigned long *p2)
     + {
     + 	int digits, ex;
     + 
     +
     + ## apply.h ##
     +@@ apply.h: int apply_all_patches(struct apply_state *state,
     + 		      int argc, const char **argv,
     + 		      int options);
     + 
     ++/*
     ++ * exposed only for tests; do not call this as it not
     ++ * a part of the API
     ++ */
     ++extern int apply_parse_fragment_range(const char *line, int len, int offset,
     ++				      const char *expect, unsigned long *p1,
     ++				      unsigned long *p2);
     ++
     + #endif
     +
     + ## t/unit-tests/t-apply.c (new) ##
     +@@
     ++#include "test-lib.h"
     ++#include "apply.h"
     ++
     ++#define FAILURE -1
     ++
      +typedef struct test_case {
      +	const char *line;
      +	const char *expect_suffix;
     @@ t/unit-tests/t-apply.c
      +} 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);
     ++{
     ++	unsigned long p1 = 9999;
     ++	unsigned long p2 = 9999;
     ++	int result = apply_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");
     ++}
     ++
     ++int cmd_main(int argc, const char **argv)
     ++{
      +	TEST(setup_static((struct test_case) {
      +		.line = "@@ -4,4 +",
      +		.offset = 4,
     @@ t/unit-tests/t-apply.c
      +		.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,
     @@ t/unit-tests/t-apply.c
      +		.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,
     @@ t/unit-tests/t-apply.c
      +		.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,
     @@ t/unit-tests/t-apply.c
      +		.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,
     @@ t/unit-tests/t-apply.c
      +		.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,
     @@ t/unit-tests/t-apply.c
      +		.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,
     @@ t/unit-tests/t-apply.c
      +		.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,
     @@ t/unit-tests/t-apply.c
      +		.expect_p1 = 9999,
      +		.expect_p2 = 9999
      +	}), "negative offset");
     - 
     - 	return test_done();
     - }
     ++
     ++	return test_done();
     ++}
 Makefile               |   1 +
 apply.c                |   6 ++-
 apply.h                |   8 ++++
 t/unit-tests/t-apply.c | 101 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 114 insertions(+), 2 deletions(-)
 create mode 100644 t/unit-tests/t-apply.c
Makefile +1 -0
index 8f4432ae57c..1615de69e6c 100644
@@ -1334,6 +1334,7 @@ THIRD_PARTY_SOURCES += compat/regex/%
 THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
+UNIT_TEST_PROGRAMS += t-apply
 UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-prio-queue
apply.c +4 -2
index 901b67e6255..8cdf7e7d77f 100644
@@ -36,6 +36,8 @@
 #include "wildmatch.h"
 #include "ws.h"
 
+#define parse_range apply_parse_fragment_range
+
 struct gitdiff_data {
 	struct strbuf *root;
 	int linenr;

@@ -1438,8 +1440,8 @@ static int parse_num(const char *line, unsigned long *p)
 	return ptr - line;
 }
 
-static int parse_range(const char *line, int len, int offset, const char *expect,
-		       unsigned long *p1, unsigned long *p2)
+int apply_parse_fragment_range(const char *line, int len, int offset, const char *expect,
+			 unsigned long *p1, unsigned long *p2)
 {
 	int digits, ex;
apply.h +8 -0
index 7cd38b1443c..283aba77495 100644
@@ -186,4 +186,12 @@ int apply_all_patches(struct apply_state *state,
 		      int argc, const char **argv,
 		      int options);
 
+/*
+ * exposed only for tests; do not call this as it not
+ * a part of the API
+ */
+extern int apply_parse_fragment_range(const char *line, int len, int offset,
+				      const char *expect, unsigned long *p1,
+				      unsigned long *p2);
+
 #endif
t/unit-tests/t-apply.c new file +101 -0

101 lines not on this page · raw message

base-commit: b9cfe4845cb2562584837bc0101c0ab76490a239

Signature · 1 line
gitgitgadget
#9 lore raw
Quoted 6 lines · Philip Peterson via GitGitGadget

From: Philip Peterson <[email protected]>

Also rename parse_range to parse_fragment_range for external linkage.

Signed-off-by Philip Peterson <[email protected]>

---

This version saw no reviews (unfortunately).

I just gave another quick glance, did not spot anything glaringly wrong, and the way the tests are organized as a series of ...

t/unit-tests/t-apply.c line 98

Quoted 8 lines
t/unit-tests/t-apply.c new file +8 -0
+	TEST(setup_static((struct test_case) {
+		.line = "@@ -4,4",
+		.offset = -1,
+		.expect_suffix = " +",
+		.expect_result = FAILURE,
+		.expect_p1 = 9999,
+		.expect_p2 = 9999
+	}), "negative offset");

... rather pleasant to read. I'd admit that it does not count as a proper review, though.

Thanks.

#10 lore raw
In reply to Philip Peterson via GitGitGadget

Hi Philip

Thanks for re-rolling, I've left a few comments below

Quoted 13 lines · Philip Peterson via GitGitGadget

From: Philip Peterson <[email protected]>

Also rename parse_range to parse_fragment_range for external linkage.

Signed-off-by Philip Peterson <[email protected]>

---
apply: add unit tests for parse_range

Add unit tests for apply's parse_range function. Also rename parse_range to parse_fragment_range and expose it externally for use by the unit tests. Internal callers may continue to refer to it by the name parse_range.

Quoted 5 lines

It is necessary to make the function be non-internal linkage in order to expose it to the unit testing framework. Because there is another function in the codebase also called parse_range, change this one to a more specific name as well: parse_fragment_range. Also add several test cases (both positive and negative) for the function.

This description is useful and should be part of the commit message above the "---" line

apply.h int apply_all_patches(struct apply_state *state, · line 195

Quoted 7 lines
apply.h +7 -0
+/*
+ * exposed only for tests; do not call this as it not
+ * a part of the API
+ */
+extern int apply_parse_fragment_range(const char *line, int len, int offset,
+				      const char *expect, unsigned long *p1,
+				      unsigned long *p2);

We don't bother with extern on function declarations as can be seen from all the other declarations in this header file.

Quoted 11 lines
apply.h +1 -0
+
  #endif
t/unit-tests/t-apply.c new file +3 -0
new file mode 100644
index 00000000000..0eb0c22fc0d
@@ -0,0 +1,101 @@
+#include "test-lib.h"
+#include "apply.h"
+

t/unit-tests/t-apply.c line 13

Quoted 10 lines
t/unit-tests/t-apply.c new file +10 -0
+#define FAILURE -1
+
+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;

We don't use typedef's in this code base

t/unit-tests/t-apply.c line 21

Quoted 7 lines
t/unit-tests/t-apply.c new file +7 -0
+static void setup_static(struct test_case t)
+{
+	unsigned long p1 = 9999;
+	unsigned long p2 = 9999;
+	int result = apply_parse_fragment_range(t.line, strlen(t.line), t.offset,
+						t.expect_suffix, &p1, &p2);
+	check_int(result, ==, t.expect_result);

If we expect apply_parse_fragment_range() to return an error then I'm not sure we want to bother checking p1 and p2 as I don't think we make any guarantees about their values in that case. If we promised not to touch them unless the fragment was successfully parsed we would want to check that but we don't make that promise.

Quoted 4 lines
t/unit-tests/t-apply.c new file +4 -0
+	check_int(p1, ==, t.expect_p1);
+	check_int(p2, ==, t.expect_p2);
+}
+

t/unit-tests/t-apply.c line 35

Quoted 10 lines
t/unit-tests/t-apply.c new file +10 -0
+int cmd_main(int argc, const char **argv)
+{
+	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");

This is a nice use of compound literals. It would be good to have a test that checks .expect_result for a longer input string to ensure we're not just stopping at the end of the string.

t/unit-tests/t-apply.c line 36

Quoted 2 lines
t/unit-tests/t-apply.c new file +2 -0
+	TEST(setup_static((struct test_case) {
+		.line = "@@ -4 +8 @@",

I would be nice to vary "-4" a bit, all the tests where the parse is successful expect p1 == 4

Quoted 13 lines
t/unit-tests/t-apply.c new file +13 -0
+		.offset = 4,
+		.expect_suffix = " +",
+		.expect_result = 7,
+		.expect_p1 = 4,
+		.expect_p2 = 1
+	}), "non-comma range");
+
+	TEST(setup_static((struct test_case) {
+		.line = "@@ -X,4 +",
+		.offset = 4,
+		.expect_suffix = " +",
+		.expect_result = FAILURE,
+		.expect_p1 = 9999,

t/unit-tests/t-apply.c line 61

Quoted 10 lines
t/unit-tests/t-apply.c new file +10 -0
+		.expect_p2 = 9999
+	}), "non-digit range (first 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

This is an example of why I don't think we should check p1 and p2 when we're expecting the parse to fail. Also please note we don't use "//" comments in the code base.

There is a good range of failing non-digit inputs. It would be nice to see a test for a hunk header where the number is too large to parse. Ideally we'd also change the parsing code to return an error in that case rather than waiting to error out later on as the apply code does currently. To do that You'd need to explicitly set errno to zero before calling strtoul() and check it afterwards.

Overall this is looking pretty good, thanks for working on it

Best Wishes

Phillip

Quoted 42 lines
t/unit-tests/t-apply.c new file +40 -0
+	}), "non-digit range (second coordinate)");
+
+	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");
+
+	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");
+
+	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");
+
+	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();
+}

base-commit: b9cfe4845cb2562584837bc0101c0ab76490a239

#11 lore raw
Quoted 6 lines · Phillip Wood

Thanks for re-rolling, I've left a few comments below
...
This is an example of why I don't think we should check p1 and p2 when we're expecting the parse to fail. Also please note we don't use "//" comments in the code base.

Quoted 6 lines

There is a good range of failing non-digit inputs. It would be nice to see a test for a hunk header where the number is too large to parse. Ideally we'd also change the parsing code to return an error in that case rather than waiting to error out later on as the apply code does currently. To do that You'd need to explicitly set errno to zero before calling strtoul() and check it afterwards.

Thanks. All points you raised make quite a lot of sense to me.