From: Junio C Hamano <gitster@pobox.com>
To: phillip.wood123@gmail.com
Cc: Josh Steadmon <steadmon@google.com>,
git@vger.kernel.org, linusa@google.com, calvinwan@google.com,
rsbecker@nexbridge.com
Subject: Re: [PATCH v7 2/3] unit tests: add TAP unit test framework
Date: Fri, 22 Sep 2023 13:05:20 -0700 [thread overview]
Message-ID: <xmqqa5te0y9r.fsf@gitster.g> (raw)
In-Reply-To: <xmqq350hw6n7.fsf@gitster.g> (Junio C. Hamano's message of "Thu, 17 Aug 2023 17:12:12 -0700")
It seems this got stuck during Josh's absense and I didn't ping it
further, but I should have noticed that you are the author of this
patch, and pinged you in the meantime.
Any thought on the "polarity" of the return values from the
assertion? I still find it confusing and hard to follow.
Thanks.
> Josh Steadmon <steadmon@google.com> writes:
>
>> +test_expect_success 'TAP output from unit tests' '
>> + cat >expect <<-EOF &&
>> + ok 1 - passing test
>> + ok 2 - passing test and assertion return 0
>> + # check "1 == 2" failed at t/unit-tests/t-basic.c:76
>> + # left: 1
>> + # right: 2
>> + not ok 3 - failing test
>> + ok 4 - failing test and assertion return -1
>> + not ok 5 - passing TEST_TODO() # TODO
>> + ok 6 - passing TEST_TODO() returns 0
>> + # todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
>> + not ok 7 - failing TEST_TODO()
>> + ok 8 - failing TEST_TODO() returns -1
>> + # check "0" failed at t/unit-tests/t-basic.c:30
>> + # skipping test - missing prerequisite
>> + # skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
>> + ok 9 - test_skip() # SKIP
>> + ok 10 - skipped test returns 0
>> + # skipping test - missing prerequisite
>> + ok 11 - test_skip() inside TEST_TODO() # SKIP
>> + ok 12 - test_skip() inside TEST_TODO() returns 0
>> + # check "0" failed at t/unit-tests/t-basic.c:48
>> + not ok 13 - TEST_TODO() after failing check
>> + ok 14 - TEST_TODO() after failing check returns -1
>> + # check "0" failed at t/unit-tests/t-basic.c:56
>> + not ok 15 - failing check after TEST_TODO()
>> + ok 16 - failing check after TEST_TODO() returns -1
>> + # check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
>> + # left: "\011hello\\\\"
>> + # right: "there\"\012"
>> + # check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
>> + # left: "NULL"
>> + # right: NULL
>> + # check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
>> + # left: ${SQ}a${SQ}
>> + # right: ${SQ}\012${SQ}
>> + # check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
>> + # left: ${SQ}\\\\${SQ}
>> + # right: ${SQ}\\${SQ}${SQ}
>> + not ok 17 - messages from failing string and char comparison
>> + # BUG: test has no checks at t/unit-tests/t-basic.c:91
>> + not ok 18 - test with no checks
>> + ok 19 - test with no checks returns -1
>> + 1..19
>> + EOF
>
> Presumably t-basic will serve as a catalog of check_* functions and
> the test binary, together with this test piece, will keep growing as
> we gain features in the unit tests infrastructure. I wonder how
> maintainable the above is, though. When we acquire new test, we
> would need to renumber. What if multiple developers add new
> features to the catalog at the same time?
>
>> diff --git a/t/unit-tests/.gitignore b/t/unit-tests/.gitignore
>> new file mode 100644
>> index 0000000000..e292d58348
>> --- /dev/null
>> +++ b/t/unit-tests/.gitignore
>> @@ -0,0 +1,2 @@
>> +/t-basic
>> +/t-strbuf
>
> Also, can we come up with some naming convention so that we do not
> have to keep adding to this file every time we add a new test
> script?
>
>> diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c
>> new file mode 100644
>> index 0000000000..561611e242
>> --- /dev/null
>> +++ b/t/unit-tests/t-strbuf.c
>> @@ -0,0 +1,75 @@
>> +#include "test-lib.h"
>> +#include "strbuf.h"
>> +
>> +/* wrapper that supplies tests with an initialized strbuf */
>> +static void setup(void (*f)(struct strbuf*, void*), void *data)
>> +{
>> + struct strbuf buf = STRBUF_INIT;
>> +
>> + f(&buf, data);
>> + strbuf_release(&buf);
>> + check_uint(buf.len, ==, 0);
>> + check_uint(buf.alloc, ==, 0);
>> + check(buf.buf == strbuf_slopbuf);
>> + check_char(buf.buf[0], ==, '\0');
>> +}
>
> What I am going to utter from here on are not complaints but purely
> meant as questions.
>
> Would the resulting output and maintainability of the tests change
> (improve, or worsen) if we introduce
>
> static void assert_empty_strbuf(struct strbuf *buf)
> {
> check_uint(buf->len, ==, 0);
> check_uint(buf->alloc, ==, 0);
> check(buf.buf == strbuf_slopbuf);
> check_char(buf.buf[0], ==, '\0');
> }
>
> and call it from the setup() function to ensure that
> strbuf_release(&buf) it calls after running customer test f() brings
> the buffer in a reasonably initialized state? The t_static_init()
> test should be able to say
>
> static void t_static_init(void)
> {
> struct strbuf buf = STRBUF_INIT;
> assert_empty_strbuf(&buf);
> }
>
> if we did so, but is that a good thing or a bad thing (e.g. it may
> make it harder to figure out where the real error came from, because
> of the "line number" thing will not easily capture the caller of the
> caller, perhaps)?
>
>> +static void t_static_init(void)
>> +{
>> + struct strbuf buf = STRBUF_INIT;
>> +
>> + check_uint(buf.len, ==, 0);
>> + check_uint(buf.alloc, ==, 0);
>> + if (check(buf.buf == strbuf_slopbuf))
>> + return; /* avoid de-referencing buf.buf */
>
> strbuf_slopbuf[0] is designed to be readable. Do check() assertions
> return their parameter negated?
>
> In other words, if "we expect buf.buf to point at the slopbuf, but
> if that expectation does not hold, check() returns true and we
> refrain from doing check_char() on the next line because we cannot
> trust what buf.buf points at" is what is going on here, I find it
> very confusing. Perhaps my intuition is failing me, but somehow I
> would have expected that passing check_foo() would return true while
> failing ones would return false.
>
> IOW I would expect
>
> if (check(buf.buf == strbuf_slopbuf))
> return;
>
> to work very similarly to
>
> if (buf.buf == strbuf_slopbuf)
> return;
>
> in expressing the control flow, simply because they are visually
> similar. But of course, if we early-return because buf.buf that
> does not point at strbuf_slopbuf is a sign of trouble, then the
> control flow we want is
>
> if (buf.buf != strbuf_slopbuf)
> return;
>
> or
>
> if (!(buf.buf == strbuf_slopbuf))
> return;
>
> The latter is easier to translate to check_foo(), because what is
> inside the inner parentheses is the condition we expect, and we
> would like check_foo() to complain when the condition does not hold.
>
> For the "check_foo()" thing to work in a similar way, while having
> the side effect of reporting any failed expectations, we would want
> to write
>
> if (!check(buf.buf == strbuf_slopbuf))
> return;
>
> And for that similarity to work, check_foo() must return false when
> its expectation fails, and return true when its expectation holds.
>
> I think that is where my "I find it very confusing" comes from.
>
>> + check_char(buf.buf[0], ==, '\0');
>> +}
>
>> +static void t_dynamic_init(void)
>> +{
>> + struct strbuf buf;
>> +
>> + strbuf_init(&buf, 1024);
>> + check_uint(buf.len, ==, 0);
>> + check_uint(buf.alloc, >=, 1024);
>> + check_char(buf.buf[0], ==, '\0');
>
> Is it sensible to check buf.buf is not slopbuf at this point, or
> does it make the test TOO intimate with the current implementation
> detail?
>
>> + strbuf_release(&buf);
>> +}
>> +
>> +static void t_addch(struct strbuf *buf, void *data)
>> +{
>> + const char *p_ch = data;
>> + const char ch = *p_ch;
>> +
>> + strbuf_addch(buf, ch);
>> + if (check_uint(buf->len, ==, 1) ||
>> + check_uint(buf->alloc, >, 1))
>> + return; /* avoid de-referencing buf->buf */
>
> Again, I find the return values from these check_uint() calls highly
> confusing, if this is saying "if len is 1 and alloc is more than 1,
> then we are in an expected state and can further validate that buf[0]
> is ch and buf[1] is NULL, but otherwise we should punt". The polarity
> looks screwy. Perhaps it is just me?
>
>> + check_char(buf->buf[0], ==, ch);
>> + check_char(buf->buf[1], ==, '\0');
>> +}
>
> In any case, this t_addch() REQUIRES that incoming buf is empty,
> doesn't it? I do not think it is sensible. I would have expected
> that it would be more like
>
> t_addch(struct strbuf *buf, void *data)
> {
> char ch = *(char *)data;
> size_t orig_alloc = buf->alloc;
> size_t orig_len = buf->len;
>
> if (!assert_sane_strbuf(buf))
> return;
> strbuf_addch(buf, ch);
> if (!assert_sane_strbuf(buf))
> return;
> check_uint(buf->len, ==, orig_len + 1);
> check_uint(buf->alloc, >=, orig_alloc);
> check_char(buf->buf[buf->len - 1], ==, ch);
> check_char(buf->buf[buf->len], ==, '\0');
> }
>
> to ensure that we can add a ch to a strbuf with any existing
> contents and get a one-byte longer contents than before, with the
> last byte of the buffer becoming 'ch' and still NUL terminated.
>
> And we protect ourselves with a helper that checks if the given
> strbuf looks *sane*.
>
> static int assert_sane_strbuf(struct strbuf *buf)
> {
> /* can use slopbuf only when the length is 0 */
> if (buf->buf == strbuf_slopbuf)
> return (buf->len == 0);
> /* everybody else must have non-NULL buffer */
> if (buf->buf == NULL)
> return 0;
> /*
> * alloc must be at least 1 byte larger than len
> * for the terminating NUL at the end.
> */
> return ((buf->len + 1 <= buf->alloc) &&
> (buf->buf[buf->len] == '\0'));
> }
>
> You can obviously use your check_foo() for the individual checks
> done in this function to get a more detailed diagnosis, but because
> I have confused myself enough by thinking about their polarity, I
> wrote this in barebones comparison instead.
next prev parent reply other threads:[~2023-09-22 20:05 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20230517-unit-tests-v2-v2-0-8c1b50f75811@google.com>
2023-06-30 22:51 ` [PATCH v4] unit tests: Add a project plan document Josh Steadmon
2023-07-01 0:42 ` Junio C Hamano
2023-07-01 1:03 ` Junio C Hamano
2023-08-07 23:07 ` [PATCH v5] " Josh Steadmon
2023-08-14 13:29 ` Phillip Wood
2023-08-15 22:55 ` Josh Steadmon
2023-08-17 9:05 ` Phillip Wood
2023-08-16 23:50 ` [PATCH v6 0/3] Add unit test framework and project plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Josh Steadmon
2023-08-16 23:50 ` [PATCH v6 1/3] unit tests: Add a project plan document Josh Steadmon
2023-08-16 23:50 ` [PATCH v6 2/3] unit tests: add TAP unit test framework Josh Steadmon
2023-08-17 0:12 ` Junio C Hamano
2023-08-17 0:41 ` Junio C Hamano
2023-08-17 18:34 ` Josh Steadmon
2023-08-16 23:50 ` [PATCH v6 3/3] ci: run unit tests in CI Josh Steadmon
2023-08-17 18:37 ` [PATCH v7 0/3] Add unit test framework and project plan Josh Steadmon
2023-08-17 18:37 ` [PATCH v7 1/3] unit tests: Add a project plan document Josh Steadmon
2023-08-17 18:37 ` [PATCH v7 2/3] unit tests: add TAP unit test framework Josh Steadmon
2023-08-18 0:12 ` Junio C Hamano
2023-09-22 20:05 ` Junio C Hamano [this message]
2023-09-24 13:57 ` phillip.wood123
2023-09-25 18:57 ` Junio C Hamano
2023-10-06 22:58 ` Josh Steadmon
2023-10-09 17:37 ` Josh Steadmon
2023-08-17 18:37 ` [PATCH v7 3/3] ci: run unit tests in CI Josh Steadmon
2023-08-17 20:38 ` [PATCH v7 0/3] Add unit test framework and project plan Junio C Hamano
2023-08-24 20:11 ` Josh Steadmon
2023-09-13 18:14 ` Junio C Hamano
2023-10-09 22:21 ` [PATCH v8 " Josh Steadmon
2023-10-09 22:21 ` [PATCH v8 1/3] unit tests: Add a project plan document Josh Steadmon
2023-10-10 8:57 ` Oswald Buddenhagen
2023-10-11 21:14 ` Josh Steadmon
2023-10-11 23:05 ` Oswald Buddenhagen
2023-11-01 17:31 ` Josh Steadmon
2023-10-27 20:12 ` Christian Couder
2023-11-01 17:47 ` Josh Steadmon
2023-11-01 23:49 ` Junio C Hamano
2023-10-09 22:21 ` [PATCH v8 2/3] unit tests: add TAP unit test framework Josh Steadmon
2023-10-11 21:42 ` Junio C Hamano
2023-10-16 13:43 ` [PATCH v8 2.5/3] fixup! " Phillip Wood
2023-10-16 16:41 ` Junio C Hamano
2023-11-01 17:54 ` Josh Steadmon
2023-11-01 23:48 ` Junio C Hamano
2023-11-01 17:54 ` Josh Steadmon
2023-11-01 23:49 ` Junio C Hamano
2023-10-27 20:15 ` [PATCH v8 2/3] " Christian Couder
2023-11-01 22:54 ` Josh Steadmon
2023-10-09 22:21 ` [PATCH v8 3/3] ci: run unit tests in CI Josh Steadmon
2023-10-09 23:50 ` [PATCH v8 0/3] Add unit test framework and project plan Junio C Hamano
2023-10-19 15:21 ` [PATCH 0/3] CMake unit test fixups Phillip Wood
2023-10-19 15:21 ` [PATCH 1/3] fixup! cmake: also build unit tests Phillip Wood
2023-10-19 15:21 ` [PATCH 2/3] fixup! artifacts-tar: when including `.dll` files, don't forget the unit-tests Phillip Wood
2023-10-19 15:21 ` [PATCH 3/3] fixup! cmake: handle also unit tests Phillip Wood
2023-10-19 19:19 ` [PATCH 0/3] CMake unit test fixups Junio C Hamano
2023-10-16 10:07 ` [PATCH v8 0/3] Add unit test framework and project plan phillip.wood123
2023-11-01 23:09 ` Josh Steadmon
2023-10-27 20:26 ` Christian Couder
2023-11-01 23:31 ` [PATCH v9 " Josh Steadmon
2023-11-01 23:31 ` [PATCH v9 1/3] unit tests: Add a project plan document Josh Steadmon
2023-11-01 23:31 ` [PATCH v9 2/3] unit tests: add TAP unit test framework Josh Steadmon
2023-11-03 21:54 ` Christian Couder
2023-11-09 17:51 ` Josh Steadmon
2023-11-01 23:31 ` [PATCH v9 3/3] ci: run unit tests in CI Josh Steadmon
2023-11-09 18:50 ` [PATCH v10 0/3] Add unit test framework and project plan Josh Steadmon
2023-11-09 18:50 ` [PATCH v10 1/3] unit tests: Add a project plan document Josh Steadmon
2023-11-09 23:15 ` Junio C Hamano
2023-11-09 18:50 ` [PATCH v10 2/3] unit tests: add TAP unit test framework Josh Steadmon
2023-11-09 18:50 ` [PATCH v10 3/3] ci: run unit tests in CI Josh Steadmon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqa5te0y9r.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=calvinwan@google.com \
--cc=git@vger.kernel.org \
--cc=linusa@google.com \
--cc=phillip.wood123@gmail.com \
--cc=rsbecker@nexbridge.com \
--cc=steadmon@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).