[GSoC,v5,0/7] t: port reftable/stack_test.c to the unit testing framework

Message

Chandra Pratap Sept. 6, 2024, 11:29 a.m. UTC

The reftable library comes with self tests, which are exercised
as part of the usual end-to-end tests and are designed to
observe the end-user visible effects of Git commands. What it
exercises, however, is a better match for the unit-testing
framework, merged at 8bf6fbd0 (Merge branch 'js/doc-unit-tests',
2023-12-09), which is designed to observe how low level
implementation details, at the level of sequences of individual
function calls, behave.

Hence, port reftable/stack_test.c to the unit testing framework and
improve upon the ported test. The first patch in the series moves
the test to the unit testing framework, and the rest of the patches
improve upon the ported test.

Mentored-by: Patrick Steinhardt <[e-mail]>
Mentored-by: Christian Couder <[e-mail]>
Signed-off-by: Chandra Pratap <[e-mail]>

---
Changes in v5:
- Edit the commit messages in patches 3 and 4 to reflect the changes
  and the motivation behind those changes better.
- Add newlines after variable declarations in patch 6.
- Introduce patch 7 which removes leftover cruft from the previous
  reftable testing scheme.

CI/PR: https://github.com/gitgitgadget/git/pull/1762

Chandra Pratap(7):
t: move reftable/stack_test.c to the unit testing framework
t: harmonize t-reftable-stack.c with coding guidelines
t-reftable-stack: use Git's tempfile API instead of mkstemp()
t-reftable-stack: use reftable_ref_record_equal() to compare ref records
t-reftable-stack: add test for non-default compaction factor
t-reftable-stack: add test for stack iterators
t: clean up leftover reftable test cruft

Makefile                                           |   4 +-
reftable/reftable-tests.h                          |  14 -
reftable/test_framework.c                          |  27 -
reftable/test_framework.h                          |  61 --
t/helper/test-reftable.c                           |   8 -
t/helper/test-tool.c                               |   3 +-
t/helper/test-tool.h                               |   1 -
t/t0032-reftable-unittest.sh                       |  16 -
.../unit-tests/t-reftable-stack.c                  | 665 ++++++++++++---------
9 files changed, 390 insertions(+), 409 deletions(-)

Range-diff against v4:
1:  eae681f53d ! 1:  ca4b00feef t-reftable-stack: use Git's tempfile API instead of mkstemp()
    @@ Commit message
         like mkstemp(), make the ported stack test strictly use Git's
         tempfile API as well.

    +    A bigger benefit is the fact that we know to clean up the tempfile
    +    in case the test fails because it gets registered and pruned via a
    +    signal handler.
    +
         Mentored-by: Patrick Steinhardt <[e-mail]>
         Mentored-by: Christian Couder <[e-mail]>
         Signed-off-by: Chandra Pratap <[e-mail]>
2:  19b2f41003 ! 2:  3e723667dd t-reftable-stack: use reftable_ref_record_equal() to compare ref records
    @@ Commit message
         reftable_ref_record_equal(), and sometimes by explicity comparing
         contents of the ref records.

    -    Replace the latter instances of ref-record comparison with the
    -    former to maintain uniformity throughout the test file.
    +    The latter method is undesired because there can exist unequal ref
    +    records with the some of the contents being equal. Replace the latter
    +    instances of ref-record comparison with the former. This has the
    +    added benefit of preserving uniformity throughout the test file.

         Mentored-by: Patrick Steinhardt <[e-mail]>
         Mentored-by: Christian Couder <[e-mail]>
3:  ffdfba6b98 = 3:  7526550c92 t-reftable-stack: add test for non-default compaction factor
4:  ca447664e7 ! 4:  05e4b7e715 t-reftable-stack: add test for stack iterators
    @@ t/unit-tests/t-reftable-stack.c: static void t_reftable_stack_add(void)
     +			.log = &logs[i],
     +			.update_index = reftable_stack_next_update_index(st),
     +		};
    ++
     +		err = reftable_stack_add(st, write_test_log, &arg);
     +		check(!err);
     +	}
    @@ t/unit-tests/t-reftable-stack.c: static void t_reftable_stack_add(void)
     +	reftable_iterator_seek_ref(&it, refs[0].refname);
     +	for (i = 0; ; i++) {
     +		struct reftable_ref_record ref = { 0 };
    ++
     +		err = reftable_iterator_next_ref(&it, &ref);
     +		if (err > 0)
     +			break;
    @@ t/unit-tests/t-reftable-stack.c: static void t_reftable_stack_add(void)
     +	reftable_iterator_seek_log(&it, logs[0].refname);
     +	for (i = 0; ; i++) {
     +		struct reftable_log_record log = { 0 };
    ++
     +		err = reftable_iterator_next_log(&it, &log);
     +		if (err > 0)
     +			break;
-:  ---------- > 5:  43c1a522fb t: clean up leftover reftable test cruft

Comments

Junio C Hamano Sept. 6, 2024, 4:38 p.m. UTC | #1

Chandra Pratap <[e-mail]> writes:

> Changes in v5:
> - Edit the commit messages in patches 3 and 4 to reflect the changes
>   and the motivation behind those changes better.
> - Add newlines after variable declarations in patch 6.
> - Introduce patch 7 which removes leftover cruft from the previous
>   reftable testing scheme.

Hmph, the end-result looks good to me, but the structure of the
series is a bit curious.  I didn't expect there will be a separate
step for removal.  Shouldn't these "leftover cruft" be removed *in*
the same step that they become cruft (which I am assuming is when
reftable/stack_test.c and all references to it gets removed in an
early part of the series)?

Other than that, looking good.

Thanks.

Junio C Hamano Sept. 6, 2024, 11:57 p.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

> Chandra Pratap <[e-mail]> writes:
>
>> Changes in v5:
>> - Edit the commit messages in patches 3 and 4 to reflect the changes
>>   and the motivation behind those changes better.
>> - Add newlines after variable declarations in patch 6.
>> - Introduce patch 7 which removes leftover cruft from the previous
>>   reftable testing scheme.
>
> Hmph, the end-result looks good to me, but the structure of the
> series is a bit curious.  I didn't expect there will be a separate
> step for removal.  Shouldn't these "leftover cruft" be removed *in*
> the same step that they become cruft (which I am assuming is when
> reftable/stack_test.c and all references to it gets removed in an
> early part of the series)?
>
> Other than that, looking good.
>
> Thanks.

There is another issue.

It is unique to this among the reftable/*_test topics, simply
because this one happens to be the last and needs to clean up a bit
more.

Apparently, GNU "ar" can be invoked without any .o object files and
happily creates an empty archive, but on BSD may not be that may be
the case.  macOS CI jobs seem to be hard failing due to this.

You'd need to squeeze in the following, in addition to [7/7], to
earlier patch(es).

Thanks.

 Makefile | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git c/Makefile w/Makefile
index 64ccb1433f..bdea061971 100644
--- c/Makefile
+++ w/Makefile
@@ -912,7 +912,6 @@ TEST_SHELL_PATH = $(SHELL_PATH)
 LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 REFTABLE_LIB = reftable/libreftable.a
-REFTABLE_TEST_LIB = reftable/libreftable_test.a
 
 GENERATED_H += command-list.h
 GENERATED_H += config-list.h
@@ -2866,9 +2865,6 @@ $(XDIFF_LIB): $(XDIFF_OBJS)
 $(REFTABLE_LIB): $(REFTABLE_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
 
-$(REFTABLE_TEST_LIB): $(REFTABLE_TEST_OBJS)
-	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
-
 export DEFAULT_EDITOR DEFAULT_PAGER
 
 Documentation/GIT-EXCLUDED-PROGRAMS: FORCE
@@ -3248,7 +3244,7 @@ perf: all
 
 t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) $(UNIT_TEST_DIR)/test-lib.o
 
-t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)
+t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
 
 check-sha1:: t/helper/test-tool$X
@@ -3709,7 +3705,7 @@ clean: profile-clean coverage-clean cocciclean
 	$(RM) git.res
 	$(RM) $(OBJECTS)
 	$(RM) headless-git.o
-	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
+	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB)
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
 	$(RM) $(TEST_PROGRAMS)
 	$(RM) $(FUZZ_PROGRAMS)