Hi René
On 02/07/2024 21:55, René Scharfe wrote:
Am 02.07.24 um 17:13 schrieb phillip.wood123@xxxxxxxxx:
On 29/06/2024 16:43, René Scharfe wrote:
The macro TEST only allows defining a test that consists of a
single expression. Add the new macro, TEST_RUN, which provides a
way to define unit tests that are made up of one or more
statements. A test started with it implicitly ends when the next
test is started or test_done() is called.
TEST_RUN allows defining self-contained tests en bloc, a bit like
test_expect_success does for regular tests. Unlike TEST it does
not require defining wrapper functions for test statements.
There are pros and cons to not requiring one function per test. It
can be a pain to have to write separate functions but it keeps each
test self contained which hopefully makes it harder to have
accidental dependencies between tests. Having separate functions for
each test makes it easy to initialize and free resources for every
test by writing a setup() function that initializes the resources,
calls the test function and then frees the resources.
Right. We should use TEST and TEST_RUN when appropriate.
The changes in patch 6 to use TEST_RUN() mean that each test now has
more boilerplate to initialize and free the strbuf. > This makes them more similar to strbuf usage in the wild. Using
the API idiomatically just makes more sense to me.
I see what you mean. I think it only looks idiomatic if you're already
familiar with the api though as the test bodies call wrappers rather
than using the strbuf api directly. I think that reduces its value as an
example of idomatic usage for someone who is not familiar with the
strbuf api.
Not hiding
initialization and release makes the tests visibly independent.
This is not enforced by TEST_RUN, but made possible by it.
Having each test in its own function also makes main() shorter and
which means can quickly get an overview of all the test cases from
it.
That's true, now you need to grep for TEST_RUN to get such an
overview.
On the other hand I find the start of the description in TEST
invocations somewhat hard to locate, as they are not vertically
aligned due to the preceding variable-length function name. Just
saying..
Yes I really wanted the first argument of TEST to be the description but
that isn't easy to do while supporting printf style format strings.
+int test__run(const char *location, const char *format, ...)
+{
+ va_list ap;
+ char *desc;
+
+ test__run_maybe_end();
+
+ va_start(ap, format);
+ desc = xstrvfmt(format, ap);
This uses an strbuf under the hood. So far we've avoided doing that
as we want to be able to test the strbuf implementation with this
test framework. We don't need to support arbitrary length strings
here so we could use a fixed array and xsnprinf() instead.
Fair point. xsnprinf() might be a bit too strict, as it doesn't
handle short buffers gracefully. Perhaps that's OK; a developer
getting hit by that could simply increase the buffer size.
I think so.
We could also let xstrvfmt() call vsnprintf(3) directly. The code
duplication would be a bit grating, but perhaps there's some good
base function hidden in there somewhere.
Oh, interesting - maybe something like
char* xstrvfmt(const char *fmt, ...)
{
va_list ap, aq;
va_start(ap, fmt);
va_copy(aq, ap);
len = vnsprintf(NULL, 0, fmt, ap);
if (len < 0)
BUG(...)
buf = xmalloc(len + 1);
if (vnsprintf(buf, len + 1, fmt, aq) != len)
BUG(...)
va_end(aq);
va_end(ap);
return buf;
}
Looking ahead the plan seems to be to change most instances of TEST()
to TEST_RUN(). If we are going to go that way perhaps we should steal
TEST() for this macro and rename the existing TEST() macro.
Not my plan, at least -- I'm content with just having the *ability*
to keep all parts of a test together.
That sounds sensible to me
+ if (TEST_RUN("static initialization works")) {
+ struct strbuf buf = STRBUF_INIT;
+ if (!check_uint(buf.len, ==, 0) ||
+ !check_uint(buf.alloc, ==, 0) ||
+ !check_char(buf.buf[0], ==, '\0'))
+ test_skip_all("STRBUF_INIT is broken");
+ }
that's a nice use of test_skip_all()
I'm not very enthusiastic about requiring the test author to wrap
TEST_RUN() in an if() statement rather than just doing that for them.
It makes it explicit but from the test author's point of view it just
feels like pointless boilerplate.
Hmm. We can add more magic, but I suspect that it might confuse
developers and editors.
To me its confusing to have to wrap TEST_RUN() in an if() statement
until one realizes that the test might be skipped. If we document that
the test body should be enclosed in braces I don't think it should
confuse developers or editors and will keep the tests a bit cleaner.
Best Wishes
Phillip
