| Message ID | 20240814121122.4642-9-chandrapratap3519@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: port reftable/block_test.c to the unit testing framework | expand |
On Wed, Aug 14, 2024 at 05:33:16PM +0530, Chandra Pratap wrote: > @@ -101,9 +101,95 @@ static void t_block_read_write(void) > reftable_record_release(&recs[i]); > } > > +static void t_log_block_read_write(void) > +{ > + const int header_off = 21; > + struct reftable_record recs[30]; > + const size_t N = ARRAY_SIZE(recs); > + const size_t block_size = 2048; > + struct reftable_block block = { 0 }; > + struct block_writer bw = { > + .last_key = STRBUF_INIT, > + }; > + struct reftable_record rec = { > + .type = BLOCK_TYPE_LOG, > + }; > + size_t i = 0; > + int n; > + struct block_reader br = { 0 }; > + struct block_iter it = BLOCK_ITER_INIT; > + struct strbuf want = STRBUF_INIT; > + > + REFTABLE_CALLOC_ARRAY(block.data, block_size); > + block.len = block_size; > + block.source = malloc_block_source(); > + block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size, > + header_off, hash_size(GIT_SHA1_FORMAT_ID)); > + > + for (i = 0; i < N; i++) { > + rec.u.log.refname = xstrfmt("branch%02"PRIuMAX , (uintmax_t)i); > + rec.u.log.update_index = i; > + rec.u.log.value_type = REFTABLE_LOG_UPDATE; > + > + recs[i] = rec; > + n = block_writer_add(&bw, &rec); > + rec.u.log.refname = NULL; > + rec.u.log.value_type = REFTABLE_LOG_DELETION; > + check_int(n, ==, 0); > + } > + > + n = block_writer_finish(&bw); > + check_int(n, >, 0); Do we maybe want to rename `n` to `ret`? That's way more customary in our codebase. > + block_writer_release(&bw); > + > + block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ); > + > + block_iter_seek_start(&it, &br); > + > + for (i = 0; ; i++) { > + int r = block_iter_next(&it, &rec); > + check_int(r, >=, 0); > + if (r > 0) > + break; We can also reuse `n` (or `ret`) here, right? > + check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ)); > + } One thing that this loop doesn't verify is whether we actually got the expected number of log records. It could be that the first iteration already returns `r > 0`, which is not our expectation. So we should likely add a check for `i == N` after the loop. Patrick
On Thu, 15 Aug 2024 at 15:11, Patrick Steinhardt <ps@pks.im> wrote: > > On Wed, Aug 14, 2024 at 05:33:16PM +0530, Chandra Pratap wrote: > > @@ -101,9 +101,95 @@ static void t_block_read_write(void) > > reftable_record_release(&recs[i]); > > } > > > > +static void t_log_block_read_write(void) > > +{ > > + const int header_off = 21; > > + struct reftable_record recs[30]; > > + const size_t N = ARRAY_SIZE(recs); > > + const size_t block_size = 2048; > > + struct reftable_block block = { 0 }; > > + struct block_writer bw = { > > + .last_key = STRBUF_INIT, > > + }; > > + struct reftable_record rec = { > > + .type = BLOCK_TYPE_LOG, > > + }; > > + size_t i = 0; > > + int n; > > + struct block_reader br = { 0 }; > > + struct block_iter it = BLOCK_ITER_INIT; > > + struct strbuf want = STRBUF_INIT; > > + > > + REFTABLE_CALLOC_ARRAY(block.data, block_size); > > + block.len = block_size; > > + block.source = malloc_block_source(); > > + block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size, > > + header_off, hash_size(GIT_SHA1_FORMAT_ID)); > > + > > + for (i = 0; i < N; i++) { > > + rec.u.log.refname = xstrfmt("branch%02"PRIuMAX , (uintmax_t)i); > > + rec.u.log.update_index = i; > > + rec.u.log.value_type = REFTABLE_LOG_UPDATE; > > + > > + recs[i] = rec; > > + n = block_writer_add(&bw, &rec); > > + rec.u.log.refname = NULL; > > + rec.u.log.value_type = REFTABLE_LOG_DELETION; > > + check_int(n, ==, 0); > > + } > > + > > + n = block_writer_finish(&bw); > > + check_int(n, >, 0); > > Do we maybe want to rename `n` to `ret`? That's way more customary in > our codebase. Sure thing, but then I would want to change the existing test (which gets renamed as t_ref_block_read_write) and I'm unsure of which patch would be the most suitable for that change. Would it be fine to include that change as a part of this patch? > > + block_writer_release(&bw); > > + > > + block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ); > > + > > + block_iter_seek_start(&it, &br); > > + > > + for (i = 0; ; i++) { > > + int r = block_iter_next(&it, &rec); > > + check_int(r, >=, 0); > > + if (r > 0) > > + break; > > We can also reuse `n` (or `ret`) here, right? > > > + check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ)); > > + } > > One thing that this loop doesn't verify is whether we actually got the > expected number of log records. It could be that the first iteration > already returns `r > 0`, which is not our expectation. So we should > likely add a check for `i == N` after the loop. What about something like if (r > 0) { check_int(i, ==, N); break; } That should achieve the same results if I'm not wrong.
On Fri, Aug 16, 2024 at 12:06:47AM +0530, Chandra Pratap wrote: > On Thu, 15 Aug 2024 at 15:11, Patrick Steinhardt <ps@pks.im> wrote: > > > > On Wed, Aug 14, 2024 at 05:33:16PM +0530, Chandra Pratap wrote: > > > @@ -101,9 +101,95 @@ static void t_block_read_write(void) > > > reftable_record_release(&recs[i]); > > > } > > > > > > +static void t_log_block_read_write(void) > > > +{ > > > + const int header_off = 21; > > > + struct reftable_record recs[30]; > > > + const size_t N = ARRAY_SIZE(recs); > > > + const size_t block_size = 2048; > > > + struct reftable_block block = { 0 }; > > > + struct block_writer bw = { > > > + .last_key = STRBUF_INIT, > > > + }; > > > + struct reftable_record rec = { > > > + .type = BLOCK_TYPE_LOG, > > > + }; > > > + size_t i = 0; > > > + int n; > > > + struct block_reader br = { 0 }; > > > + struct block_iter it = BLOCK_ITER_INIT; > > > + struct strbuf want = STRBUF_INIT; > > > + > > > + REFTABLE_CALLOC_ARRAY(block.data, block_size); > > > + block.len = block_size; > > > + block.source = malloc_block_source(); > > > + block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size, > > > + header_off, hash_size(GIT_SHA1_FORMAT_ID)); > > > + > > > + for (i = 0; i < N; i++) { > > > + rec.u.log.refname = xstrfmt("branch%02"PRIuMAX , (uintmax_t)i); > > > + rec.u.log.update_index = i; > > > + rec.u.log.value_type = REFTABLE_LOG_UPDATE; > > > + > > > + recs[i] = rec; > > > + n = block_writer_add(&bw, &rec); > > > + rec.u.log.refname = NULL; > > > + rec.u.log.value_type = REFTABLE_LOG_DELETION; > > > + check_int(n, ==, 0); > > > + } > > > + > > > + n = block_writer_finish(&bw); > > > + check_int(n, >, 0); > > > > Do we maybe want to rename `n` to `ret`? That's way more customary in > > our codebase. > > Sure thing, but then I would want to change the existing test (which gets > renamed as t_ref_block_read_write) and I'm unsure of which patch would > be the most suitable for that change. Would it be fine to include that > change as a part of this patch? The way I'd do it is to first do the minimum required changes to port the old test to the new testing framework, without any of the cleanups to align code style. Then I'd put another commit on top that does all the changes like removing braces, converting types and also adapting names. > > > + block_writer_release(&bw); > > > + > > > + block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ); > > > + > > > + block_iter_seek_start(&it, &br); > > > + > > > + for (i = 0; ; i++) { > > > + int r = block_iter_next(&it, &rec); > > > + check_int(r, >=, 0); > > > + if (r > 0) > > > + break; > > > > We can also reuse `n` (or `ret`) here, right? > > > > > + check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ)); > > > + } > > > > One thing that this loop doesn't verify is whether we actually got the > > expected number of log records. It could be that the first iteration > > already returns `r > 0`, which is not our expectation. So we should > > likely add a check for `i == N` after the loop. > > What about something like > if (r > 0) { > check_int(i, ==, N); > break; > } > That should achieve the same results if I'm not wrong. Yup, looks good to me. Patrick
diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c index fa289e10f2..01ef10e7a6 100644 --- a/t/unit-tests/t-reftable-block.c +++ b/t/unit-tests/t-reftable-block.c @@ -12,7 +12,7 @@ license that can be found in the LICENSE file or at #include "reftable/constants.h" #include "reftable/reftable-error.h" -static void t_block_read_write(void) +static void t_ref_block_read_write(void) { const int header_off = 21; /* random */ struct reftable_record recs[30]; @@ -101,9 +101,95 @@ static void t_block_read_write(void) reftable_record_release(&recs[i]); } +static void t_log_block_read_write(void) +{ + const int header_off = 21; + struct reftable_record recs[30]; + const size_t N = ARRAY_SIZE(recs); + const size_t block_size = 2048; + struct reftable_block block = { 0 }; + struct block_writer bw = { + .last_key = STRBUF_INIT, + }; + struct reftable_record rec = { + .type = BLOCK_TYPE_LOG, + }; + size_t i = 0; + int n; + struct block_reader br = { 0 }; + struct block_iter it = BLOCK_ITER_INIT; + struct strbuf want = STRBUF_INIT; + + REFTABLE_CALLOC_ARRAY(block.data, block_size); + block.len = block_size; + block.source = malloc_block_source(); + block_writer_init(&bw, BLOCK_TYPE_LOG, block.data, block_size, + header_off, hash_size(GIT_SHA1_FORMAT_ID)); + + for (i = 0; i < N; i++) { + rec.u.log.refname = xstrfmt("branch%02"PRIuMAX , (uintmax_t)i); + rec.u.log.update_index = i; + rec.u.log.value_type = REFTABLE_LOG_UPDATE; + + recs[i] = rec; + n = block_writer_add(&bw, &rec); + rec.u.log.refname = NULL; + rec.u.log.value_type = REFTABLE_LOG_DELETION; + check_int(n, ==, 0); + } + + n = block_writer_finish(&bw); + check_int(n, >, 0); + + block_writer_release(&bw); + + block_reader_init(&br, &block, header_off, block_size, GIT_SHA1_RAWSZ); + + block_iter_seek_start(&it, &br); + + for (i = 0; ; i++) { + int r = block_iter_next(&it, &rec); + check_int(r, >=, 0); + if (r > 0) + break; + check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ)); + } + + for (i = 0; i < N; i++) { + block_iter_reset(&it); + strbuf_reset(&want); + strbuf_addstr(&want, recs[i].u.log.refname); + + n = block_iter_seek_key(&it, &br, &want); + check_int(n, ==, 0); + + n = block_iter_next(&it, &rec); + check_int(n, ==, 0); + + check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ)); + + want.len--; + n = block_iter_seek_key(&it, &br, &want); + check_int(n, ==, 0); + + n = block_iter_next(&it, &rec); + check_int(n, ==, 0); + check(reftable_record_equal(&recs[10 * (i / 10)], &rec, GIT_SHA1_RAWSZ)); + } + + block_reader_release(&br); + block_iter_close(&it); + reftable_record_release(&rec); + reftable_block_done(&br.block); + strbuf_release(&want); + for (i = 0; i < N; i++) + reftable_record_release(&recs[i]); +} + int cmd_main(int argc, const char *argv[]) { - TEST(t_block_read_write(), "read-write operations on blocks work"); + TEST(t_log_block_read_write(), "read-write operations on log blocks work"); + TEST(t_ref_block_read_write(), "read-write operations on ref blocks work"); return test_done(); }
In the current testing setup, block operations are only exercised for ref blocks. Add another test that exercises these operations for log blocks as well. Mentored-by: Patrick Steinhardt <ps@pks.im> Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com> --- t/unit-tests/t-reftable-block.c | 90 ++++++++++++++++++++++++++++++++- 1 file changed, 88 insertions(+), 2 deletions(-)