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

Message

Chandra Pratap Aug. 26, 2024, 5:29 p.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 v2:
- Use 'size_t' as array index instead of 'int' in a test that is
  modified in patch 2.
- Fix a coding style violation in the newly introduced test in
  patch 6.

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

Chandra Pratap(6):
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

Makefile                                                 |   2 +-
reftable/reftable-tests.h                                |   1 -
t/helper/test-reftable.c                                 |   1 -
reftable/stack_test.c => t/unit-tests/t-reftable-stack.c | 611 +++++++++++++++++++--------------
4 files changed, 360 insertions(+), 255 deletions(-)

Range-diff against v2:
1:  e172ceefd7 ! 1:  838ccc63a7 t: harmonize t-reftable-stack.c with coding guidelines
    @@ t/unit-tests/t-reftable-stack.c: static void t_reftable_stack_compaction_concurr
      	/* break abstraction boundary to simulate unclean shutdown. */
     -	int i = 0;
     -	for (; i < st->readers_len; i++) {
    -+	for (int i = 0; i < st->readers_len; i++)
    ++	for (size_t i = 0; i < st->readers_len; i++)
      		reftable_reader_free(st->readers[i]);
     -	}
      	st->readers_len = 0;
2:  73b94aaa6a = 2:  2a151c299c t-reftable-stack: use Git's tempfile API instead of mkstemp()
3:  b6f0363656 = 3:  fb5073da2c t-reftable-stack: use reftable_ref_record_equal() to compare ref records
4:  35c09e6054 = 4:  028fa6f70b t-reftable-stack: add test for non-default compaction factor
5:  3163c2af7d ! 5:  fa0d358e65 t-reftable-stack: add test for stack iterators
    @@ t/unit-tests/t-reftable-stack.c: static void t_reftable_stack_add(void)
     +	}
     +
     +	for (i = 0; i < N; i++) {
    -+		err = reftable_stack_add(st, &write_test_ref, &refs[i]);
    ++		err = reftable_stack_add(st, write_test_ref, &refs[i]);
     +		check(!err);
     +	}
     +
    @@ 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);
    ++		err = reftable_stack_add(st, write_test_log, &arg);
     +		check(!err);
     +	}
     +

Comments

Junio C Hamano Aug. 26, 2024, 5:48 p.m. UTC | #1

Chandra Pratap <[e-mail]> writes:

> Changes in v2:
> - Use 'size_t' as array index instead of 'int' in a test that is
>   modified in patch 2.
> - Fix a coding style violation in the newly introduced test in
>   patch 6.

If I recall correctly, the first iteration conflicted too badly with
other topics in flight, and kept out of 'seen' so far.  Have you
tested this series, not just standalone, but see how well it works
together with other topics by creating trial merges of it into
'next' and 'seen'?

It some other topics that conflict with what you do in this topic
have advanced (or better yet, graduated to 'master'), it may not be
a bad idea to consider rebasing the topic to more recent 'master',
possibly merging other topics into it first.

Thanks.

Chandra Pratap Aug. 26, 2024, 6:34 p.m. UTC | #2

On Mon, 26 Aug 2024 at 23:18, Junio C Hamano <[e-mail]> wrote:
>
> Chandra Pratap <[e-mail]> writes:
>
> > Changes in v2:
> > - Use 'size_t' as array index instead of 'int' in a test that is
> >   modified in patch 2.
> > - Fix a coding style violation in the newly introduced test in
> >   patch 6.
>
> If I recall correctly, the first iteration conflicted too badly with
> other topics in flight, and kept out of 'seen' so far.

Yeah, I remember Patrick was working on some other patch
series involving 'reftable/stack_test.c' which caused conflicts
with this series. I _did_ make sure to rebase this series on top
of that one.

> Have you tested this series, not just standalone, but see how well
> it works together with other topics by creating trial merges of it into
> 'next' and 'seen'?

I did notice some small Makefile conflicts with the latest 'master'
on the GitHub CI, but other than that, no, I haven't checked the
patch against 'next' or 'seen'.

> It some other topics that conflict with what you do in this topic
> have advanced (or better yet, graduated to 'master'), it may not be
> a bad idea to consider rebasing the topic to more recent 'master',
> possibly merging other topics into it first.

I generally make sure to rebase my patches on top of the latest
master before sending them to the mailing list. I'll add 'next' and
'seen' to my checklist as well.

Junio C Hamano Aug. 26, 2024, 7:07 p.m. UTC | #3

Chandra Pratap <[e-mail]> writes:

> I generally make sure to rebase my patches on top of the latest
> master before sending them to the mailing list.

This is generally considered a bad practice.  You shouldn't rebase
unless there is a compelling reason (e.g., API you rely on
disappeared and you need to reimplement your change differently, you
rewrote some code based on an old version, but the code you touched
have been enhanced so you'd need to rewrite the new part).

On the other hand, you should realize that every topic first is
queued to 'seen' and only after it proves that it plays well with
other topics in flight, it is considered to advance to 'next'.  So
if you conflict with other topics that makes you conflict when
merged to either 'next' or 'seen', you'd be better off creating a
more suitable base than 'master' and then build on top of it.

An effective way to do so is to mimick how topics like

  https://lore.kernel.org/git/[e-mail]/

are built.  This one says:

    This patch series continues to build on top of 25673b1c47 (The third
    batch, 2024-08-07) with Junio's ps/reftable-stack-compaction at
    f234df07f6 (reftable/stack: handle locked tables during auto-compaction,
    2024-08-08) merged into it.

in its cover letter, allowing anybody to reconstruct the base with

    $ git checkout -b ps/reftable-drop-generic 25673b1c47
    $ git merge f234df07f6

before running "git am" to apply the series on top.  Because it
_depends_ on ps/reftable-stack-compaction, the resulting topic
cannot be merged to anywhere without dragging the base topic with
it, so such a dependency needs to be already fairly stable (e.g.,
have been thoroughly reviewed and merged to 'next'), but then the
topic will have less chance to conflict when merged to 'next' and
'seen'.

Thanks.

Junio C Hamano Aug. 26, 2024, 7:26 p.m. UTC | #4

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

> On the other hand, you should realize that every topic first is
> queued to 'seen' and only after it proves that it plays well with
> other topics in flight, it is considered to advance to 'next'.  So
> if you conflict with other topics that makes you conflict when
> merged to either 'next' or 'seen', you'd be better off creating a
> more suitable base than 'master' and then build on top of it.

$ git log --first-parent --oneline \
  cp/unit-test-reftable-stack..next -- reftable/stack_test.c

tells me that Patrick's "concurrent compaction" and "drop generic"
are the big two topics that touch reftable/stack_test.c file that
you are removing.  As the former already depends on the latter,
it may make sense to 

    $ git checkout -b cp/unit-test-reftable-stack master
    $ git merge ps/reftable-concurrent-compaction

to prepare the base, and then rebuild these patches on top of it.
There is another topic jk/mark-unused-parameters that touches the
same file, but the conflict it causes is trivial.

In any case, the cover letter is a good place to describe how you
prepared such a custom base (as opposed to "the patches in this
topic apply cleanly to any recent tip of 'master', as the topic
touches a relatively dormant calm area of the code base", in which
case you do not say anything).

Thanks.