From: Philip Peterson <redacted>
Also rename parse_range to parse_fragment_range for external linkage.
Signed-off-by: Philip Peterson <redacted>
---
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 [off-list ref]
## 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 [off-list ref]
- ## 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
diff --git a/Makefile b/Makefile
index 8f4432ae57c..1615de69e6c 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/apply.c b/apply.c
index 901b67e6255..8cdf7e7d77f 100644
--- a/apply.c
+++ b/apply.c
@@ -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;
diff --git a/apply.h b/apply.h
index 7cd38b1443c..283aba77495 100644
--- a/apply.h
+++ b/apply.h
@@ -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
diff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c
new file mode 100644
index 00000000000..0eb0c22fc0d
--- /dev/null
+++ b/t/unit-tests/t-apply.c
@@ -0,0 +1,101 @@
+#include "test-lib.h"
+#include "apply.h"
+
+#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;
+
+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);
+ check_int(p1, ==, t.expect_p1);
+ check_int(p2, ==, t.expect_p2);
+}
+
+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");
+
+ 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");
+
+ 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)");
+
+ 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)");
+
+ 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
--
gitgitgadget
Hi Philip
Thanks for re-rolling, I've left a few comments below
On 26/05/2024 08:54, Philip Peterson via GitGitGadget wrote:
From: Philip Peterson <redacted>
Also rename parse_range to parse_fragment_range for external linkage.
Signed-off-by: Philip Peterson <redacted>
---
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.
This description is useful and should be part of the commit message
above the "---" line
+/*
+ * 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 hunk
+
#endif
diff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c
new file mode 100644
index 00000000000..0eb0c22fc0d
--- /dev/null
+++ b/t/unit-tests/t-apply.c
@@ -0,0 +1,101 @@
+#include "test-lib.h"
+#include "apply.h"
+
+#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
+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.
+ check_int(p1, ==, t.expect_p1);
+ check_int(p2, ==, t.expect_p2);
+}
+
+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.
+ 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
+ .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,
+ .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
+ }), "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