On Tue, 23 Jul 2024 at 22:39, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Chandra Pratap <chandrapratap3519@xxxxxxxxx> writes: > > > 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/pq_test.c to the unit testing framework and > > improve upon the ported test. The first two patches in the series > > are preparatory cleanup, the third patch moves the test to the unit > > testing framework, and the rest of the patches improve upon the > > ported test. > > > > Mentored-by: Patrick Steinhardt <ps@xxxxxx> > > Mentored-by: Christian Couder <chriscool@xxxxxxxxxxxxx> > > Signed-off-by: Chandra Pratap <chandrapratap3519@xxxxxxxxx> > > > > --- > > Changes in v5: > > - Rebase the branch on top of the latest master branch > > If you need to perform this rebase, please say _why_ you are > rebasing. > > "A new rc was tagged so I rebased" is *not* a good reason. > > "I wanted to use that new feature that was merged to 'master' > recently, which was not available when I wrote the previous > iteration of this series, hence I rebased" is a very good reason. > > "Since I wrote the previous iteration, other unit test topics have > graduated, so there are trivial conflicts in Makefile when merging > this topic" is usually not a good reason, especially when the same > conflicts with these other unit test topics are already resolved > when your previous iteration is merged to 'seen'. > > If there isn't a reason worth mentioning why you are rebasing, then > please do not rebase. It is distracting. Oh, okay. > > - Rename tests according to unit-tests' conventions > > - remove 'pq_test_main()' from reftable/reftable-test.h > > > > CI/PR for v5: https://github.com/gitgitgadget/git/pull/1745 > > By the way, I still haven't got any answer to a question I asked > long ago on this series, wrt possibly unifying this pq and another > pq we already use elsewhere in our codebase. If we are butchering > what we borrowed from elsewhere and store in reftable/. directory > and taking responsibility of maintaining it ourselves, we probably > should consider larger refactoring and cleaning up, and part of it > we may end up discarding this pq implementation, making the unit > testing on it a wasted effort. > > Thanks. > I did talk about this with Patrick and Christian on a private slack channel a few weeks ago and here is how that conversation went: Me: Hey, I wanted to talk about the message from Junio the other day. It is true that through this project, we are modifying the reftable directory to a point that it is no longer easily usable by someone from outside. If that is the direction we want to take, wouldn't it make more sense to get rid of reftable/pq.{c, h} altogether and use Git's prio-queue instead? Christian: Yeah, I think the direction the Git project wants to take is to integrate the reftable code more and more with the Git code. On the other hand, there are libification projects which are trying to split parts of the Git code into libraries usable by other projects. But I don't think each of these libraries should have their own test framework, their own prio-queue implementation, their own string implementation, etc. So, even if I am not sure about the end result, I think it would be ok to modify the reftable code so that it uses the Git's prio queue and maybe other Git data structures. But I'd like Patrick to confirm, and the list to agree to this. So I'd rather wait until Patrick is back from his vacation before doing things like replacing reftable/pq.{c, h} with Git's prio-queue. Patrick: Just chiming in real quick: while the reftable library is currently in a position where it cannot be used standalone by other projects, I'd very much like to move into the direction of making it completely standalone again so that we can adapt e.g. libgit2 to use it. So I rather want to move the other direction over time and re-establish a proper boundary between reftable library and the remainder of Git. I don't really think that this needs to impact the reftable tests though. Git is the upstream of the reftable library, and I don't see much of a point why every other project should carry the same tests, too. ---snip---
