threads / patch / 24980

patchAdd test-string-list.c

Subject: [PATCH] Add test-string-list.c

## tl;dr

6 messages between Sep 5, 2010 and Sep 7, 2010. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Thiago Farina· Sep 5, 2010, 02:34 UTC · lore

Add a simple test that demonstrates how to create and manipulate a list of strings using the string-list.h API.

To see the test, call it by: ./bin-wrappers/test-string-list.c

Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
 Makefile           |    1 +
 test-string-list.c |   29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)
 create mode 100644 test-string-list.c
Show changes to 2 files +30 −0

Makefile, test-string-list.c

diff --git a/Makefile b/Makefile
index 40fbcae..287bc2c 100644
--- a/Makefile
+++ b/Makefile
@@ -422,6 +422,7 @@ TEST_PROGRAMS_NEED_X += test-string-pool
 TEST_PROGRAMS_NEED_X += test-svn-fe
 TEST_PROGRAMS_NEED_X += test-treap
 TEST_PROGRAMS_NEED_X += test-index-version
+TEST_PROGRAMS_NEED_X += test-string-list
 
 TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
 
diff --git a/test-string-list.c b/test-string-list.c
new file mode 100644
index 0000000..11b8a7f
--- /dev/null
+++ b/test-string-list.c
@@ -0,0 +1,29 @@
+#include <stdio.h>
+#include <string.h>
+#include "string-list.h"
+
+int main(int argc, const char **argv)
+{
+	struct string_list list;
+	int i;
+
+	memset(&list, 0, sizeof(struct string_list));
+
+	string_list_append(&list, "foo");
+	string_list_append(&list, "bar");
+
+	for (i = 0; i < list.nr; i++)
+		printf("%s\n", list.items[i].string);
+
+	print_string_list(&list, "");
+
+        int has_foo = string_list_has_string(&list, "foo");
+	if (has_foo != 1)
+		error("List doesn't have foo.");
+
+	string_list_clear(&list, 0);
+	if (list.nr > 0)
+		error("List is not clear.");
+
+	return 0;
+}

-- 
1.7.2.3.313.gcd15
Jonathan Nieder· Sep 5, 2010, 05:02 UTC · re: Thiago Farina · lore

Re: [PATCH] Add test-string-list.c

Hi Thiago,
Thiago Farina wrote:
> Add a simple test that demonstrates how to create and manipulate
> a list of strings using the string-list.h API.
Quick thoughts:
> --- /dev/null
> +++ b/test-string-list.c
> @@ -0,0 +1,29 @@
> +#include <stdio.h>
Git programs tend to start with 
 #include "cache.h"
or
 #include "git-compat-util.h"
to get all the portability niceties.
> +	print_string_list(&list, "");
> +
> +        int has_foo = string_list_has_string(&list, "foo");

Whitespace, declaration after statement... (see Documentation/CodingGuidelines).

> +	if (has_foo != 1)
> +		error("List doesn't have foo.");

This does not exit with nonzero status when it fails. You probably wanted

	if (bad things)
		return error("problems!");
> +	string_list_clear(&list, 0);
> +	if (list.nr > 0)
> +		error("List is not clear.");

To make sure this example remains valid, wouldn't you want to include a caller in the t/ directory so it can be automatically run? (See t/README.)

Thoughts separate from the code:
 * it is probably worth mentioning Documentation/technical/api-string-list.txt
   for people who do not know about it.
 * for this to be useful as a test I think one has to sort of believe
   that it can break.  That is, a test of something this basic (which
   is already demonstrated and exercised by code throughout git, after
   all) would tend to be especially devious.
 * api-string-list.txt does not mention the STRING_LIST_INIT macros
   you introduced.  Maybe that would be worth improving.

Regards, Jonathan

Junio C Hamano· Sep 6, 2010, 06:12 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Add test-string-list.c

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 9 quoted lines
> Git programs tend to start with 
>
>  #include "cache.h"
>
> or
>
>  #include "git-compat-util.h"
>
> to get all the portability niceties.

Quote Documentation/CodingGuidelines here, too. And use of these headers is not merely "niceties"--some exotic platforms tend to want standard system headers in particular inclusion order and what git-compat-util.h does is the result of us painfully finding these rules out over years.

Show 12 quoted lines
> Thoughts separate from the code:
>
>  * it is probably worth mentioning Documentation/technical/api-string-list.txt
>    for people who do not know about it.
>
>  * for this to be useful as a test I think one has to sort of believe
>    that it can break.  That is, a test of something this basic (which
>    is already demonstrated and exercised by code throughout git, after
>    all) would tend to be especially devious.
>
>  * api-string-list.txt does not mention the STRING_LIST_INIT macros
>    you introduced.  Maybe that would be worth improving.
 * this may be built but nothing exercises it.  Is it worth adding
   tNNNN-run-string-list-test.sh for some value of NNNN?

I tend to agree with your second point and have a moderately negative feeling against the patch. An addition to API documentation like you suggested would be more useful for people, and what Thiago did perhaps might be useful in its sample code section, but then we already have plenty of working samples in live code and they are single "git grep" away, so...

Thiago Farina· Sep 6, 2010, 23:48 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Add test-string-list.c

On Sun, Sep 5, 2010 at 2:02 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:>
Show 11 quoted lines
>
> Git programs tend to start with
>
>  #include "cache.h"
>
> or
>
>  #include "git-compat-util.h"
>
> to get all the portability niceties.
>
Including it now.
Show 7 quoted lines
>> +     print_string_list(&list, "");
>> +
>> +        int has_foo = string_list_has_string(&list, "foo");
>
> Whitespace, declaration after statement... (see
> Documentation/CodingGuidelines).
>
Fixed.
Show 9 quoted lines
>> +     if (has_foo != 1)
>> +             error("List doesn't have foo.");
>
> This does not exit with nonzero status when it fails.  You probably
> wanted
>
>        if (bad things)
>                return error("problems!");
>
Fixed.
Show 8 quoted lines
>> +     string_list_clear(&list, 0);
>> +     if (list.nr > 0)
>> +             error("List is not clear.");
>
> To make sure this example remains valid, wouldn't you want to include
> a caller in the t/ directory so it can be automatically run?  (See
> t/README.)
>

I read it, but I'm not sure how to do this. Maybe you could point me to an example?

Show 7 quoted lines
> Thoughts separate from the code:
>
>  * for this to be useful as a test I think one has to sort of believe
>   that it can break.  That is, a test of something this basic (which
>   is already demonstrated and exercised by code throughout git, after
>   all) would tend to be especially devious.
>

It is basic, so anyone can read it, and say "Oh, I can do this.". Looking through the code maybe not so easy.

It can be expanded later by anyone to test many other things though. So, why not? (Is it so bad to not have it at all?).

Jonathan Nieder· Sep 7, 2010, 00:07 UTC · re: Thiago Farina · lore

Re: [PATCH] Add test-string-list.c

Hi Thiago,
Thiago Farina wrote:
> On Sun, Sep 5, 2010 at 2:02 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:>
Show 6 quoted lines
>> To make sure this example remains valid, wouldn't you want to include
>> a caller in the t/ directory so it can be automatically run?  (See
>> t/README.)
>
> I read it, but I'm not sure how to do this. Maybe you could point me
> to an example?
t0070-fundamental.sh might be a good place to add it.
> It can be expanded later by anyone to test many other things though.

I suppose. Edge cases for arguments, sorting, and _DUP versus _NODUP semantics would be the main thing I would be interested in testing. Maybe another day.

Thanks, Jonathan

Thiago Farina· Sep 7, 2010, 12:04 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Add test-string-list.c

On Mon, Sep 6, 2010 at 9:07 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 14 quoted lines
> Hi Thiago,
>
> Thiago Farina wrote:
>> On Sun, Sep 5, 2010 at 2:02 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:>
>
>>> To make sure this example remains valid, wouldn't you want to include
>>> a caller in the t/ directory so it can be automatically run?  (See
>>> t/README.)
>>
>> I read it, but I'm not sure how to do this. Maybe you could point me
>> to an example?
>
> t0070-fundamental.sh might be a good place to add it.
>
Thanks, added it to there.
>> It can be expanded later by anyone to test many other things though.
>
> I suppose.  Edge cases for arguments, sorting, and _DUP versus _NODUP
> semantics would be the main thing I would be interested in testing.

Reworked into small functions to test string_list functions separated (using assert to check if the return value of the function is what is expected).

Please, take another look.

← back to recent threads