apply: add unit tests for parse_range #1677
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There are issues in commit 9f88d9c: |
|
There are issues in commit 62c536e: |
philip-peterson
force-pushed
the
peterson/unit-tests
branch
from
62c536e
to
99863bb
Compare
|
There are issues in commit 9f88d9c: |
|
There are issues in commit 99863bb: |
philip-peterson
force-pushed
the
peterson/unit-tests
branch
from
99863bb
to
18d3f5c
Compare
|
There are issues in commit 597cffd: |
philip-peterson
force-pushed
the
peterson/unit-tests
branch
2 times, most recently
from
6634c85
to
a865f07
Compare
|
/preview |
|
Preview email sent as pull.1677.git.git.1708313069.gitgitgadget@gmail.com |
|
/preview |
|
Preview email sent as pull.1677.git.git.1708313752.gitgitgadget@gmail.com |
philip-peterson
force-pushed
the
peterson/unit-tests
branch
from
a865f07
to
7dab12a
Compare
|
/submit |
|
Submitted as pull.1677.git.git.1708317938.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget-git[bot]](https://avatars.githubusercontent.com/in/46807?s=60&v=4){kind=small}
| @@ -1339,6 +1339,7 @@ THIRD_PARTY_SOURCES += compat/regex/% | |||
| THIRD_PARTY_SOURCES += sha1collisiondetection/% | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Philip Peterson via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philip Peterson <philip.c.peterson@gmail.com>
>
> 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.
> diff --git a/apply.c b/apply.c
> index 7608e3301ca..199a1150df6 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -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).
> diff --git a/apply.h b/apply.h
> index 7cd38b1443c..bbc5e3caeb5 100644
> --- a/apply.h
> +++ b/apply.h
> @@ -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.
> diff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c
> new file mode 100644
This should go to the next patch.There was a problem hiding this comment.
On the Git mailing list, Philip wrote (reply to this):
Hi,
Thanks for the tips. I am confused about the change request though:
> 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)
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
On Mon, Feb 19, 2024 at 4:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Philip Peterson via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Philip Peterson <philip.c.peterson@gmail.com>
> >
> > 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.
>
> > diff --git a/apply.c b/apply.c
> > index 7608e3301ca..199a1150df6 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -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).
>
> > diff --git a/apply.h b/apply.h
> > index 7cd38b1443c..bbc5e3caeb5 100644
> > --- a/apply.h
> > +++ b/apply.h
> > @@ -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.
>
> > diff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c
> > new file mode 100644
>
> This should go to the next patch.There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Philip <philip.c.peterson@gmail.com> writes:
> On Mon, Feb 19, 2024 at 4:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> "Philip Peterson via GitGitGadget" <gitgitgadget@gmail.com> writes:
It's quite a blast from a long time ago that I no longer remember.
>> 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)
>
> 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.
| @@ -0,0 +1,100 @@ | |||
| #include "test-lib.h" | |||
| #include "apply.h" | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Philip Peterson via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philip Peterson <philip.c.peterson@gmail.com>
>
> 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 <philip.c.peterson@gmail.com>
> ---
> 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.
| @@ -0,0 +1,100 @@ | |||
| #include "test-lib.h" | |||
| #include "apply.h" | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, "Kristoffer Haugsbakk" wrote (reply to this):
On Mon, Feb 19, 2024, at 05:45, Philip Peterson via GitGitGadget wrote:
> From: Philip Peterson <philip.c.peterson@gmail.com>
>
> 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 <philip.c.peterson@gmail.com>
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).
--
Kristoffer Haugsbakk
|
User |
|
User |
philip-peterson
force-pushed
the
peterson/unit-tests
branch
from
7dab12a
to
8f4886c
Compare
|
There are issues in commit c476258: |
|
There are issues in commit 92f845b: |
|
There are issues in commit 8f4886c: |
philip-peterson
force-pushed
the
peterson/unit-tests
branch
from
8f4886c
to
cc744ed
Compare
|
There are issues in commit cc744ed: |
philip-peterson
force-pushed
the
peterson/unit-tests
branch
3 times, most recently
from
d47dd76
to
2126e44
Compare
philip-peterson
force-pushed
the
peterson/unit-tests
branch
from
3c89376
to
c0d7b93
Compare
|
/preview |
|
Preview email sent as pull.1677.v2.git.git.1716706300848.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1677.v2.git.git.1716710073910.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
|
This patch series was integrated into seen via f314b5b. |
|
There was a status update in the "New Topics" section about the branch source: <pull.1677.v2.git.git.1716710073910.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via e103a19. |
|
There was a status update in the "Cooking" section about the branch A unit test for code that parses the hunk offset and length from a patch fragment header as been added. Will merge to 'next'? source: <pull.1677.v2.git.git.1716710073910.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 3c38b01. |
|
This patch series was integrated into seen via aa3f0c5. |
|
There was a status update in the "Cooking" section about the branch A unit test for code that parses the hunk offset and length from a patch fragment header as been added. Will merge to 'next'? source: <pull.1677.v2.git.git.1716710073910.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 7356646. |
|
This patch series was integrated into seen via 1cedfb7. |
|
This patch series was integrated into seen via c1ad8e0. |
|
There was a status update in the "Cooking" section about the branch A unit test for code that parses the hunk offset and length from a patch fragment header as been added. Will merge to 'next'? source: <pull.1677.v2.git.git.1716710073910.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 87958e7. |
|
This patch series was integrated into seen via b8aba73. |
|
This patch series was integrated into seen via c6bc41b. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Philip Peterson via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philip Peterson <philip.c.peterson@gmail.com>
>
> Also rename parse_range to parse_fragment_range for external linkage.
>
> Signed-off-by: Philip Peterson <philip.c.peterson@gmail.com>
> ---
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 ...
> + 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.
|
|
This patch series was integrated into seen via 74c82d7. |
|
There was a status update in the "Cooking" section about the branch A unit test for code that parses the hunk offset and length from a patch fragment header as been added. Will merge to 'next'. source: <pull.1677.v2.git.git.1716710073910.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via a46be7a. |
|
On the Git mailing list, Phillip Wood wrote (reply to this): 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 <philip.c.peterson@gmail.com>
> > Also rename parse_range to parse_fragment_range for external linkage.
> > Signed-off-by: Philip Peterson <philip.c.peterson@gmail.com>
> ---
> 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.
> +
> #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 |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <phillip.wood123@gmail.com> writes:
> 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.
>
> 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.
|
|
This patch series was integrated into seen via 65d5a5f. |
|
This patch series was integrated into seen via af2271c. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add unit tests for apply's
parse_rangefunction. Also renameparse_rangetoparse_fragment_rangeand expose it externally for use by the unit tests. Internal callers may continue to refer to it by the nameparse_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.
cc: "Kristoffer Haugsbakk" code@khaugsbakk.name
cc: Philip philip.c.peterson@gmail.com
cc: Phillip Wood phillip.wood123@gmail.com