From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 617646EC44; Thu, 5 Nov 2026 21:40:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 617646EC44 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1793904015; bh=xXvL3Mrr5y0fVpnKJP6rXC8yGxSqQxZ10+qK7SzZ5oc=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=MQ50RONdjf/SS1UYhUNOhKWtX4gz0RvthCRf2Q0/FubaDDD35SOv6zZ3h5Npj1+yX XKoV3/eF71cuboYNQxKh3Yh2n9CRX9I3abZBt6B5ncHe+OlA/y+SF7W+VTTNSibMe8 Ra0W2CcxjDKH/Bw5YnlyKD/tuHjJYtiBZ2CkI4Sg= Received: from send194.i.mail.ru (send194.i.mail.ru [95.163.59.33]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 51E646EC44 for ; Thu, 5 Nov 2026 21:40:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 51E646EC44 Received: by exim-smtp-5cc8b577fd-b7jlc with esmtpa (envelope-from ) id 1xP2Nh-00000000GLn-1vBb; Thu, 05 Nov 2026 21:40:13 +0300 Date: Thu, 5 Nov 2026 21:39:46 +0300 To: Sergey Bronnikov Message-ID: References: <4dd8dd2aecca35784e687f7920e6880ad0211f3c.1793624427.git.estetus@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4dd8dd2aecca35784e687f7920e6880ad0211f3c.1793624427.git.estetus@gmail.com> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD96F2D5E088614A2373E200990B77D8079EE68BA4D62480B7F182A05F5380850403148082CD585A8D43DE06ABAFEAF670597D3802B56E97CDA1FA90FBD77FF4C3ACDC27D7208C63596 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AEA4A6B3AFC9B957C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE789066434B85BF7C7EA1F7E6F0F101C67CDEEF6D7F21E0D1D9295C2E9FA3191EE1B59CA4C82EFA6587B4FD461121988C65D03B9DA6973CF4F3A5E41049CA289FC117882F44604297287769387670735201E561CDFBCA1751FC26CFBAC0749D213D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269176DF2183F8FC7C0D75EB778CE7D8A0C68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7CE0F3BA37685B2B9043847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A5785A91C3A396BC745002B1117B3ED696963AD325F7F02940361FAC1196A180DE823CB91A9FED034534781492E4B8EEAD8D8BB953E4894305BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0AD73CAD6646DEDE1918E10F71CB4DF9F96AB70F9BE574AE9C625B6776AC983F447FC0B9F89525902EE6F57B2FD27647F25E66C117BDB76D659F0FA8332ADD2CA558A8F697AF8EC93A5E5E890567111194BAE65FB22AA781AB92D8A31CC29E97C71B8341EE9D5BE9A0A1728C082C7DDC186C28A9732893741772FA63675FD5B57A9C7CEAA0681F5848F4C41F94D744909CECFA6C6B0C050A61A8CAF69B82BA93681CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXOcIETfglQORZ0zpDET4Zrk3igikrdHlWEXnDTjvX5/Q3KofJT2Itw4= X-Mailru-Sender: 689FA8AB762F73937C9FA53A4753B313BECFE5F22917552015E894D6F7004A960057130D653669BCE49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/3] test: add unit tests for the write buffer X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > > 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 is my fault and should be fixed. > +*/ > + > +#include > + > +#include > +#include > +#include > + > +#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; > +} > + > + > +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) > +static int test_addstring(void *state) > +{ > +} > + > +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; > +} > +/* > +** 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 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