| Message ID | 20240621115708.3626-7-chandrapratap3519@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/11] t: move reftable/record_test.c to the unit testing framework | expand |
Chandra Pratap <chandrapratap3519@gmail.com> writes: > reftable_record_is_deletion() is a function defined in > reftable/record.{c, h} that determines whether a record is of > type deletion or not. In the current testing setup, this function > is left untested for all the four record types (ref, log, obj, index). > > Add tests for this function in the case of ref records. > > 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-record.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c > index 1f9c830631..cbc2ce93b2 100644 > --- a/t/unit-tests/t-reftable-record.c > +++ b/t/unit-tests/t-reftable-record.c > @@ -108,6 +108,7 @@ static void test_reftable_ref_record_roundtrip(void) > for (int i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) { > struct reftable_record in = { > .type = BLOCK_TYPE_REF, > + .u.ref.value_type = i, > }; > struct reftable_record out = { .type = BLOCK_TYPE_REF }; > struct strbuf key = STRBUF_INIT; > @@ -121,15 +122,19 @@ static void test_reftable_ref_record_roundtrip(void) > in.u.ref.value_type = i; > switch (i) { > case REFTABLE_REF_DELETION: > + check(reftable_record_is_deletion(&in)); > break; > case REFTABLE_REF_VAL1: > + check(!reftable_record_is_deletion(&in)); > set_hash(in.u.ref.value.val1, 1); > break; > case REFTABLE_REF_VAL2: > + check(!reftable_record_is_deletion(&in)); > set_hash(in.u.ref.value.val2.value, 1); > set_hash(in.u.ref.value.val2.target_value, 2); > break; > case REFTABLE_REF_SYMREF: > + check(!reftable_record_is_deletion(&in)); > in.u.ref.value.symref = xstrdup("target"); > break; > } I think it might be easier to follow if we just move this outside the switch, perhaps something like: diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c index d480cc438a..5cb910f6be 100644 --- a/t/unit-tests/t-reftable-record.c +++ b/t/unit-tests/t-reftable-record.c @@ -139,19 +139,15 @@ static void test_reftable_ref_record_roundtrip(void) in.u.ref.value_type = i; switch (i) { case REFTABLE_REF_DELETION: - check(reftable_record_is_deletion(&in)); break; case REFTABLE_REF_VAL1: - check(!reftable_record_is_deletion(&in)); set_hash(in.u.ref.value.val1, 1); break; case REFTABLE_REF_VAL2: - check(!reftable_record_is_deletion(&in)); set_hash(in.u.ref.value.val2.value, 1); set_hash(in.u.ref.value.val2.target_value, 2); break; case REFTABLE_REF_SYMREF: - check(!reftable_record_is_deletion(&in)); in.u.ref.value.symref = xstrdup("target"); break; } @@ -159,6 +155,7 @@ static void test_reftable_ref_record_roundtrip(void) test_copy(&in); + check_int(reftable_record_is_deletion(&in), ==, i == REFTABLE_REF_DELETION); check_int(reftable_record_val_type(&in), ==, i); reftable_record_key(&in, &key); > -- > 2.45.2.404.g9eaef5822c
On Tue, Jun 25, 2024 at 5:15 AM Karthik Nayak <karthik.188@gmail.com> wrote: > Chandra Pratap <chandrapratap3519@gmail.com> writes: > > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c > > @@ -121,15 +122,19 @@ static void test_reftable_ref_record_roundtrip(void) > > switch (i) { > > case REFTABLE_REF_DELETION: > > + check(reftable_record_is_deletion(&in)); > > break; > > case REFTABLE_REF_VAL1: > > + check(!reftable_record_is_deletion(&in)); > > set_hash(in.u.ref.value.val1, 1); > > break; > > I think it might be easier to follow if we just move this outside the > switch, perhaps something like: > > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c > @@ -139,19 +139,15 @@ static void test_reftable_ref_record_roundtrip(void) > switch (i) { > case REFTABLE_REF_DELETION: > - check(reftable_record_is_deletion(&in)); > break; > case REFTABLE_REF_VAL1: > - check(!reftable_record_is_deletion(&in)); > set_hash(in.u.ref.value.val1, 1); > break; > @@ -159,6 +155,7 @@ static void test_reftable_ref_record_roundtrip(void) > + check_int(reftable_record_is_deletion(&in), ==, i == REFTABLE_REF_DELETION); It's subjective, of course, but for what it's worth, I find Chandra's code easier to reason about than this proposed rewrite for at least two reasons: (1) The intention of the original code is obvious at a glance, whereas the proposed rewrite requires careful reading and thinking to understand what is being tested. (2) In the original, because the check is being done within each `case` arm, it is easy to see how it relates to the case in question, whereas in the proposed rewrite, the test is far enough removed from from the `switch` that it is more difficult to relate to each possible case.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Tue, Jun 25, 2024 at 5:15 AM Karthik Nayak <karthik.188@gmail.com> wrote: >> Chandra Pratap <chandrapratap3519@gmail.com> writes: >> > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c >> > @@ -121,15 +122,19 @@ static void test_reftable_ref_record_roundtrip(void) >> > switch (i) { >> > case REFTABLE_REF_DELETION: >> > + check(reftable_record_is_deletion(&in)); >> > break; >> > case REFTABLE_REF_VAL1: >> > + check(!reftable_record_is_deletion(&in)); >> > set_hash(in.u.ref.value.val1, 1); >> > break; >> >> I think it might be easier to follow if we just move this outside the >> switch, perhaps something like: >> >> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c >> @@ -139,19 +139,15 @@ static void test_reftable_ref_record_roundtrip(void) >> switch (i) { >> case REFTABLE_REF_DELETION: >> - check(reftable_record_is_deletion(&in)); >> break; >> case REFTABLE_REF_VAL1: >> - check(!reftable_record_is_deletion(&in)); >> set_hash(in.u.ref.value.val1, 1); >> break; >> @@ -159,6 +155,7 @@ static void test_reftable_ref_record_roundtrip(void) >> + check_int(reftable_record_is_deletion(&in), ==, i == REFTABLE_REF_DELETION); > > It's subjective, of course, but for what it's worth, I find Chandra's > code easier to reason about than this proposed rewrite for at least > two reasons: > > (1) The intention of the original code is obvious at a glance, whereas > the proposed rewrite requires careful reading and thinking to > understand what is being tested. > > (2) In the original, because the check is being done within each > `case` arm, it is easy to see how it relates to the case in question, > whereas in the proposed rewrite, the test is far enough removed from > from the `switch` that it is more difficult to relate to each possible > case. I agree with your statements, my argument was coming more from a point that the switch statement was used to check and instantiate data into the structure based on its type. As such, it would make sense to isolate this away from the checks made on the same structure.
diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c index 1f9c830631..cbc2ce93b2 100644 --- a/t/unit-tests/t-reftable-record.c +++ b/t/unit-tests/t-reftable-record.c @@ -108,6 +108,7 @@ static void test_reftable_ref_record_roundtrip(void) for (int i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) { struct reftable_record in = { .type = BLOCK_TYPE_REF, + .u.ref.value_type = i, }; struct reftable_record out = { .type = BLOCK_TYPE_REF }; struct strbuf key = STRBUF_INIT; @@ -121,15 +122,19 @@ static void test_reftable_ref_record_roundtrip(void) in.u.ref.value_type = i; switch (i) { case REFTABLE_REF_DELETION: + check(reftable_record_is_deletion(&in)); break; case REFTABLE_REF_VAL1: + check(!reftable_record_is_deletion(&in)); set_hash(in.u.ref.value.val1, 1); break; case REFTABLE_REF_VAL2: + check(!reftable_record_is_deletion(&in)); set_hash(in.u.ref.value.val2.value, 1); set_hash(in.u.ref.value.val2.target_value, 2); break; case REFTABLE_REF_SYMREF: + check(!reftable_record_is_deletion(&in)); in.u.ref.value.symref = xstrdup("target"); break; }
reftable_record_is_deletion() is a function defined in reftable/record.{c, h} that determines whether a record is of type deletion or not. In the current testing setup, this function is left untested for all the four record types (ref, log, obj, index). Add tests for this function in the case of ref records. 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-record.c | 5 +++++ 1 file changed, 5 insertions(+)