[Tarantool-patches] [PATCH luajit 1/3] test: add unit tests for the write buffer

[Sergey Kaplun]

Hi, Sergey!
Thanks for the patch and painstaking work with coverage of wbuf module!
Generally LGTM with minor suggestions below.

On 02.11.26, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb at tarantool.org>
> 
> The write buffer accumulates the profilers' output and passes it
> to the writer on flush. It has no unit tests, so cover writing of
> the data of all kinds, flushing of a full buffer, the explicit
> flush and the error reporting.
> 
> Part of tarantool/tarantool#11620

Minor: I'm not sure that this is the part of this issue. Maybe
| Relates to
is more appropriate.

> ---
>  test/tarantool-c-tests/unit-wbuf.test.c | 388 ++++++++++++++++++++++++
>  1 file changed, 388 insertions(+)
>  create mode 100644 test/tarantool-c-tests/unit-wbuf.test.c
> 
> diff --git a/test/tarantool-c-tests/unit-wbuf.test.c b/test/tarantool-c-tests/unit-wbuf.test.c
> new file mode 100644
> index 00000000..4bee6243
> --- /dev/null
> +++ b/test/tarantool-c-tests/unit-wbuf.test.c
> @@ -0,0 +1,388 @@
> +/*
> +** Testing of the write buffer.
> +**
> +** The write buffer accumulates the profilers' output and passes
> +** it to the writer on flush. The tests below cover writing of the
> +** data of all kinds, flushing on overflow, the explicit flush and
> +** the error reporting.

Minor: It is better to use
| /*
|  *
|  */

Comments here as in the most C/C++ code in the tests.
The inconsistent comment style in the <unit-leb128.test.c> is my fault
and should be fixed.

> +*/
> +
> +#include <errno.h>
> +
> +#include <stddef.h>
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include "lj_wbuf.h"
> +#include "test.h"
> +
> +/* Size of the write buffer used in the tests. */
> +#define BUFFER_SIZE 10
> +
> +/* Size of the payload that does not fit into the write buffer. */
> +#define PAYLOAD_SIZE (2 * BUFFER_SIZE + 5)
> +
> +/* What the writer does on flush. */
> +enum writer_mode {
> +	/* Report all the data as written. */
> +	WMODE_OK,
> +	/* Report a half of the data as written and keep the buffer. */

Nit: Comment length is more than 66 symbols.
Here and in:
* `test_flush_partial_write()`
* `test_addbyte_failed_flush()`
* `test_addu64_failed_flush()`
* `test_addn_failed_flush()`
* `test_addn_failed_flush_in_loop()`

> +	WMODE_PARTIAL,
> +	/* Imitate an unrecoverable write error, e.g. ENOSPC. */
> +	WMODE_FAIL,
> +};
> +
> +/* Writer context for simulating different write outcomes. */
> +struct writer_ctx {
> +	/* What the writer does on flush. */
> +	enum writer_mode mode;
> +	/* Number of performed flushes. */
> +	size_t flushes;
> +	/* Number of bytes reported as written. */
> +	size_t written;
> +	/* Everything reported as written, in order of writing. */
> +	uint8_t out[2 * PAYLOAD_SIZE];

Why do we need 2x size from the PAYLOAD here? Please add the comment.

> +};
> +
> +static size_t test_writer(const void **buf_addr, size_t len, void *opt)
> +{
> +	struct writer_ctx *ctx = opt;
> +	size_t n = len;
> +
> +	ctx->flushes++;

Minor: Please add the comment that writer function is called only by
`lj_wbuf_flush()` before this increment.

> +	if (ctx->mode == WMODE_FAIL) {
> +		errno = ENOSPC;
> +		*buf_addr = NULL;
> +		return 0;
> +	}
> +	if (ctx->mode == WMODE_PARTIAL) {

Minor: s/if/else if/

> +		errno = EIO;
> +		n = len / 2;
> +	}
> +	memcpy(ctx->out + ctx->written, *buf_addr, n);
> +	ctx->written += n;
> +	return n;
> +}
> +

<snipped>

> +
> +static void assert_stream_stopped_with_error(const struct lj_wbuf *buf)

I suggest using macro instead for such helpers to preserve the line of
original assertion check:

So instead of:
| not ok 9 - test_flush_stopped_stream
|   ---
|   location:     test/tarantool-c-tests/unit-wbuf.test.c:80
|   condition_failed:     'lj_wbuf_test_flag(buf, STREAM_STOP)'

We have:
| not ok 9 - test_flush_stopped_stream
|   ---
|   location:     test/tarantool-c-tests/unit-wbuf.test.c:304
|   condition_failed:     'lj_wbuf_test_flag((&buf), STREAM_STOP)'

> +{
> +	assert_true(lj_wbuf_test_flag(buf, STREAM_STOP));
> +	assert_true(lj_wbuf_test_flag(buf, STREAM_ERRIO));
> +	assert_int_equal(lj_wbuf_errno(buf), ENOSPC);

Since this precise assertion, I suggest naming the helper function/macro
something like `assert_wbuf_stopped_enospc()`.


> +	assert_ptr_equal(buf->buf, NULL);
> +	assert_ptr_equal(buf->pos, NULL);
> +}
> +
> +static int test_addbyte(void *state)

<snipped>


> +static int test_addstring(void *state)
> +{

<snipped>

> +}
> +
> +static int test_addn_pumped_through_buffer(void *state)
> +{
> +	uint8_t mem[BUFFER_SIZE];
> +	uint8_t payload[PAYLOAD_SIZE];
> +	struct lj_wbuf buf;
> +	struct writer_ctx ctx;
> +
> +	UNUSED(state);
> +
> +	memset(payload, 'a', sizeof(payload));
> +	writer_ctx_init(&ctx, WMODE_OK);
> +	lj_wbuf_init(&buf, test_writer, &ctx, mem, sizeof(mem));
> +
> +	/*
> +	 * The payload does not fit into the buffer, so it is
> +	 * pumped through it. Two full buffers are flushed, the
> +	 * rest stays in the buffer.
> +	 */
> +	lj_wbuf_addn(&buf, payload, sizeof(payload));
> +	assert_sizet_equal(ctx.flushes, 2);

This `2` may be not very obvious.
We may declare constant in this function before the comment above:

| const int writes = PAYLOAD_SIZE / BUFFER_SIZE;

> +	assert_sizet_equal(ctx.written, 2 * BUFFER_SIZE);
> +	assert_true(memcmp(ctx.out, payload, 2 * BUFFER_SIZE) == 0);
> +	assert_sizet_equal(buf_len(&buf), PAYLOAD_SIZE - 2 * BUFFER_SIZE);

It is better to add the variable:

| const int remains = PAYLOAD_SIZE - writes * BUFFER_SIZE;

> +	assert_true(memcmp(mem, payload + 2 * BUFFER_SIZE,
> +			   PAYLOAD_SIZE - 2 * BUFFER_SIZE) == 0);
> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +static int test_flush_on_overflow(void *state)
> +{
> +	uint8_t mem[BUFFER_SIZE];
> +	uint8_t payload[BUFFER_SIZE];
> +	struct lj_wbuf buf;
> +	struct writer_ctx ctx;
> +
> +	UNUSED(state);
> +
> +	memset(payload, 'a', sizeof(payload));
> +	writer_ctx_init(&ctx, WMODE_OK);
> +	lj_wbuf_init(&buf, test_writer, &ctx, mem, sizeof(mem));
> +
> +	/* A byte written to a full buffer is preceded by a flush. */
> +	lj_wbuf_addn(&buf, payload, sizeof(payload));
> +	assert_sizet_equal(buf_len(&buf), sizeof(mem));
> +	lj_wbuf_addbyte(&buf, 0xaa);
> +	assert_sizet_equal(ctx.flushes, 1);
> +	assert_sizet_equal(ctx.written, sizeof(mem));
> +	assert_sizet_equal(buf_len(&buf), 1);
> +	assert_int_equal(mem[0], 0xaa);

I suggest splitting this 1 test into 3 subtests, since they have the
similar logic:
1) Init buffer.
2) Write something without flushes.
3) Write something with a flush.

It helps to avoid the specific `ctx.out` size and makes checks for
`flushes`, `written` and `buf_len()` similar.

> +
> +	/* The same for an ULEB128 value written to a full buffer. */
> +	lj_wbuf_addn(&buf, payload, sizeof(payload) - 1);
> +	assert_sizet_equal(buf_len(&buf), sizeof(mem));
> +	lj_wbuf_addu64(&buf, 0);
> +	assert_sizet_equal(ctx.flushes, 2);
> +	assert_sizet_equal(ctx.written, 2 * sizeof(mem));
> +	assert_sizet_equal(buf_len(&buf), 1);

Looks like there should be a check for the content as for the 0xaa
byte.

> +
> +	/* And for a data block written to a full buffer. */
> +	lj_wbuf_addn(&buf, payload, sizeof(payload) - 1);
> +	assert_sizet_equal(buf_len(&buf), sizeof(mem));
> +	lj_wbuf_addn(&buf, payload, 1);
> +	assert_sizet_equal(ctx.flushes, 3);
> +	assert_sizet_equal(ctx.written, 3 * sizeof(mem));
> +	assert_sizet_equal(buf_len(&buf), 1);

Looks like there should be a check for the content as for the 0xaa
byte.

> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +static int test_flush(void *state)

I suggest renaming this as `test_flush_and_write()` or something similar.
Otherwise, the difference between this test and the test above is
unclear.

> +{
> +	uint8_t mem[BUFFER_SIZE];
> +	struct lj_wbuf buf;
> +	struct writer_ctx ctx;
> +
> +	UNUSED(state);
> +
> +	writer_ctx_init(&ctx, WMODE_OK);
> +	lj_wbuf_init(&buf, test_writer, &ctx, mem, sizeof(mem));
> +
> +	lj_wbuf_addn(&buf, "xyz", 3);
> +	lj_wbuf_flush(&buf);
> +	assert_sizet_equal(ctx.flushes, 1);
> +	assert_sizet_equal(ctx.written, 3);
> +	assert_true(memcmp(ctx.out, "xyz", 3) == 0);
> +	/* The buffer is empty after a successful flush. */
> +	assert_sizet_equal(buf_len(&buf), 0);
> +	assert_false(lj_wbuf_test_flag(&buf, STREAM_ERRIO | STREAM_STOP));
> +
> +	/* The buffer is usable after the flush. */
> +	lj_wbuf_addbyte(&buf, 0x7f);
> +	assert_sizet_equal(buf_len(&buf), 1);
> +	assert_int_equal(mem[0], 0x7f);
> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +static int test_flush_partial_write(void *state)
> +{
> +	uint8_t mem[BUFFER_SIZE];
> +	struct lj_wbuf buf;
> +	struct writer_ctx ctx;
> +
> +	UNUSED(state);
> +
> +	writer_ctx_init(&ctx, WMODE_PARTIAL);
> +	lj_wbuf_init(&buf, test_writer, &ctx, mem, sizeof(mem));
> +
> +	lj_wbuf_addn(&buf, "xyz", 3);
> +	lj_wbuf_flush(&buf);
> +	/* A partial write is reported as an error. */
> +	assert_true(lj_wbuf_test_flag(&buf, STREAM_ERRIO));
> +	assert_int_equal(lj_wbuf_errno(&buf), EIO);
> +	/* The writer keeps the buffer, so the stream is not stopped. */
> +	assert_false(lj_wbuf_test_flag(&buf, STREAM_STOP));
> +	assert_ptr_equal(buf.buf, mem);
> +	/* The unwritten data is dropped. */
> +	assert_sizet_equal(buf_len(&buf), 0);

I'm not sure that we should fixate this policy. Generally there is no
guarantee what has happened with data. Except that it is not written
successfully.
Feel free to ignore.

> +
> +	return TEST_EXIT_SUCCESS;
> +}

<snipped>

> +/*
> +** A writer may release the buffer and set *data to NULL on an
> +** unrecoverable error (e.g. ENOSPC). The stream is stopped in
> +** this case, so the write functions must not touch the released
> +** buffer.
> +*/

Minor: It is better to use
| /*
|  *
|  */

Comments here as in the most C/C++ code in the tests.
The inconsistent comment style in the <unit-leb128.test.c> is my fault
and should be fixed.

> +int main(void)
> +{
> +	const struct test_unit tgroup[] = {
> +		test_unit_def(test_addbyte),
> +		test_unit_def(test_addu64),
> +		test_unit_def(test_addn),
> +		test_unit_def(test_addstring),
> +		test_unit_def(test_addn_pumped_through_buffer),
> +		test_unit_def(test_flush_on_overflow),
> +		test_unit_def(test_flush),
> +		test_unit_def(test_flush_partial_write),
> +		test_unit_def(test_flush_stopped_stream),
> +		test_unit_def(test_write_to_stopped_stream),
> +		test_unit_def(test_terminate),
> +	};
> +	const int test_result = test_run_group(tgroup, NULL);

Minor: Why do we need this additional variable?

> +
> +	return test_result;
> +}
> -- 
> 2.56.0
> 

-- 
Best regards,
Sergey Kaplun