Re: [PATCH 06/11] t-reftable-record: add ref tests for reftable_record_is_deletion()
From: Eric Sunshine <hidden>
Date: 2024-06-25 17:31:54
On Tue, Jun 25, 2024 at 5:15 AM Karthik Nayak [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Chandra Pratap [off-list ref] writes:quoted
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.