| Message ID | 20240723143032.4261-1-chandrapratap3519@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | t: port reftable/pq_test.c to the unit testing framework | expand |
Chandra Pratap <chandrapratap3519@gmail.com> 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@pks.im> > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com> > > --- > 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. > - 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. > Chandra Pratap(7): > reftable: remove unncessary curly braces in reftable/pq.c > reftable: change the type of array indices to 'size_t' in reftable/pq.c > t: move reftable/pq_test.c to the unit testing framework > t-reftable-pq: make merged_iter_pqueue_check() static > t-reftable-pq: make merged_iter_pqueue_check() callable by reference > t-reftable-pq: add test for index based comparison > t-reftable-pq: add tests for merged_iter_pqueue_top() > > Makefile | 2 +- > reftable/pq.c | 29 +++----- > reftable/pq.h | 1 - > reftable/pq_test.c | 74 --------------------- > reftable/reftable-tests.h | 1 - > t/helper/test-reftable.c | 1 - > t/unit-tests/t-reftable-pq.c | 155 +++++++++++++++++++++++++++++++++++++++++++ > 7 files changed, 166 insertions(+), 97 deletions(-) > > Range-diff against v4: > <rebase commits> > 1: d3c5605ea2 = 382: acd9d26aaf reftable: remove unncessary curly braces in reftable/pq.c > 2: 3c333e7770 = 383: 2e0986207b reftable: change the type of array indices to 'size_t' in reftable/pq.c > 3: bf547f705a ! 384: df06b6d604 t: move reftable/pq_test.c to the unit testing framework > @@ Commit message > t: move reftable/pq_test.c to the unit testing framework > > reftable/pq_test.c exercises a priority queue defined by > - reftable/pq.{c, h}. Migrate reftable/pq_test.c to the unit > - testing framework. Migration involves refactoring the tests > - to use the unit testing framework instead of reftable's test > - framework. > + reftable/pq.{c, h}. Migrate reftable/pq_test.c to the unit testing > + framework. Migration involves refactoring the tests to use the unit > + testing framework instead of reftable's test framework, and > + renaming the tests to align with unit-tests' standards. > > Mentored-by: Patrick Steinhardt <ps@pks.im> > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com> > > ## Makefile ## > -@@ Makefile: THIRD_PARTY_SOURCES += sha1dc/% > - UNIT_TEST_PROGRAMS += t-ctype > - UNIT_TEST_PROGRAMS += t-mem-pool > +@@ Makefile: UNIT_TEST_PROGRAMS += t-oidmap > + UNIT_TEST_PROGRAMS += t-oidtree > UNIT_TEST_PROGRAMS += t-prio-queue > + UNIT_TEST_PROGRAMS += t-reftable-basics > +UNIT_TEST_PROGRAMS += t-reftable-pq > + UNIT_TEST_PROGRAMS += t-reftable-record > UNIT_TEST_PROGRAMS += t-strbuf > UNIT_TEST_PROGRAMS += t-strcmp-offset > - UNIT_TEST_PROGRAMS += t-trailer > -@@ Makefile: REFTABLE_TEST_OBJS += reftable/basics_test.o > +@@ Makefile: REFTABLE_OBJS += reftable/writer.o > REFTABLE_TEST_OBJS += reftable/block_test.o > REFTABLE_TEST_OBJS += reftable/dump.o > REFTABLE_TEST_OBJS += reftable/merged_test.o > -REFTABLE_TEST_OBJS += reftable/pq_test.o > - REFTABLE_TEST_OBJS += reftable/record_test.o > REFTABLE_TEST_OBJS += reftable/readwrite_test.o > REFTABLE_TEST_OBJS += reftable/stack_test.o > + REFTABLE_TEST_OBJS += reftable/test_framework.o > + > + ## reftable/reftable-tests.h ## > +@@ reftable/reftable-tests.h: license that can be found in the LICENSE file or at > + int basics_test_main(int argc, const char **argv); > + int block_test_main(int argc, const char **argv); > + int merged_test_main(int argc, const char **argv); > +-int pq_test_main(int argc, const char **argv); > + int record_test_main(int argc, const char **argv); > + int readwrite_test_main(int argc, const char **argv); > + int stack_test_main(int argc, const char **argv); > > ## t/helper/test-reftable.c ## > @@ t/helper/test-reftable.c: int cmd__reftable(int argc, const char **argv) > - record_test_main(argc, argv); > + /* test from simple to complex. */ > block_test_main(argc, argv); > tree_test_main(argc, argv); > - pq_test_main(argc, argv); > @@ t/unit-tests/t-reftable-pq.c: license that can be found in the LICENSE file or a > } > } > > - static void test_pq(void) > +-static void test_pq(void) > ++static void t_pq(void) > { > - struct merged_iter_pqueue pq = { NULL }; > + struct merged_iter_pqueue pq = { 0 }; > @@ t/unit-tests/t-reftable-pq.c: static void test_pq(void) > { > - RUN_TEST(test_pq); > - return 0; > -+ TEST(test_pq(), "pq works"); > ++ TEST(t_pq(), "pq works"); > + > + return test_done(); > } > 4: 7dd3a2b27f = 385: 40745ab18e t-reftable-pq: make merged_iter_pqueue_check() static > 5: c803e7adfc ! 386: ee8432ac4a t-reftable-pq: make merged_iter_pqueue_check() callable by reference > @@ t/unit-tests/t-reftable-pq.c: license that can be found in the LICENSE file or a > } > } > > -@@ t/unit-tests/t-reftable-pq.c: static void test_pq(void) > +@@ t/unit-tests/t-reftable-pq.c: static void t_pq(void) > }; > > merged_iter_pqueue_add(&pq, &e); > 6: 0b03f3567d ! 387: 94a77f5a60 t-reftable-pq: add test for index based comparison > @@ t/unit-tests/t-reftable-pq.c: static void merged_iter_pqueue_check(const struct > } > } > > --static void test_pq(void) > -+static void test_pq_record(void) > +-static void t_pq(void) > ++static void t_pq_record(void) > { > struct merged_iter_pqueue pq = { 0 }; > struct reftable_record recs[54]; > -@@ t/unit-tests/t-reftable-pq.c: static void test_pq(void) > +@@ t/unit-tests/t-reftable-pq.c: static void t_pq(void) > merged_iter_pqueue_release(&pq); > } > > -+static void test_pq_index(void) > ++static void t_pq_index(void) > +{ > + struct merged_iter_pqueue pq = { 0 }; > + struct reftable_record recs[14]; > @@ t/unit-tests/t-reftable-pq.c: static void test_pq(void) > + > int cmd_main(int argc, const char *argv[]) > { > -- TEST(test_pq(), "pq works"); > -+ TEST(test_pq_record(), "pq works with record-based comparison"); > -+ TEST(test_pq_index(), "pq works with index-based comparison"); > +- TEST(t_pq(), "pq works"); > ++ TEST(t_pq_record(), "pq works with record-based comparison"); > ++ TEST(t_pq_index(), "pq works with index-based comparison"); > > return test_done(); > } > 7: 0cdfa6221e ! 388: 9a76f87bd1 t-reftable-pq: add tests for merged_iter_pqueue_top() > @@ t/unit-tests/t-reftable-pq.c: static void merged_iter_pqueue_check(const struct > + return !reftable_record_cmp(a->rec, b->rec) && (a->index == b->index); > +} > + > - static void test_pq_record(void) > + static void t_pq_record(void) > { > struct merged_iter_pqueue pq = { 0 }; > -@@ t/unit-tests/t-reftable-pq.c: static void test_pq_record(void) > +@@ t/unit-tests/t-reftable-pq.c: static void t_pq_record(void) > } while (i != 1); > > while (!merged_iter_pqueue_is_empty(pq)) { > @@ t/unit-tests/t-reftable-pq.c: static void test_pq_record(void) > check(reftable_record_type(e.rec) == BLOCK_TYPE_REF); > if (last) > check_int(strcmp(last, e.rec->u.ref.refname), <, 0); > -@@ t/unit-tests/t-reftable-pq.c: static void test_pq_index(void) > +@@ t/unit-tests/t-reftable-pq.c: static void t_pq_index(void) > } > > for (i = N - 1; !merged_iter_pqueue_is_empty(pq); i--) { > @@ t/unit-tests/t-reftable-pq.c: static void test_pq_index(void) > check(reftable_record_type(e.rec) == BLOCK_TYPE_REF); > check_int(e.index, ==, i); > if (last) > -@@ t/unit-tests/t-reftable-pq.c: static void test_pq_index(void) > +@@ t/unit-tests/t-reftable-pq.c: static void t_pq_index(void) > merged_iter_pqueue_release(&pq); > } > > -+static void test_merged_iter_pqueue_top(void) > ++static void t_merged_iter_pqueue_top(void) > +{ > + struct merged_iter_pqueue pq = { 0 }; > + struct reftable_record recs[14]; > @@ t/unit-tests/t-reftable-pq.c: static void test_pq_index(void) > + > int cmd_main(int argc, const char *argv[]) > { > - TEST(test_pq_record(), "pq works with record-based comparison"); > - TEST(test_pq_index(), "pq works with index-based comparison"); > -+ TEST(test_merged_iter_pqueue_top(), "merged_iter_pqueue_top works"); > + TEST(t_pq_record(), "pq works with record-based comparison"); > + TEST(t_pq_index(), "pq works with index-based comparison"); > ++ TEST(t_merged_iter_pqueue_top(), "merged_iter_pqueue_top works"); > > return test_done(); > }
On Tue, 23 Jul 2024 at 22:39, Junio C Hamano <gitster@pobox.com> wrote: > > Chandra Pratap <chandrapratap3519@gmail.com> 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@pks.im> > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com> > > > > --- > > 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---
On Wed, Jul 24, 2024 at 7:12 AM Chandra Pratap <chandrapratap3519@gmail.com> wrote: > > On Tue, 23 Jul 2024 at 22:39, Junio C Hamano <gitster@pobox.com> wrote: > > > - 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. I agree it might have been better to start by replacing the pq implementation in reftable/ with our own first, as there would be no need for this patch series, but Chandra's GSoC is about replacing the unit test framework in reftable/ with our own which is still valuable. And I think that at this point it is just simpler to not get distracted by replacing the pq implementation now. It's also not like changing the unit test framework would make replacing the pq implementation harder. > 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. Yeah, if it had been discussed and agreed on earlier, I think replacing the pq implementation would have made sense. Now I think it's a bit late at this stage in Chandra's GSoC to go in this direction though. I think it's better if he can focus on finishing to replace the unit test framework.
On Wed, 24 Jul 2024 at 12:48, Christian Couder <christian.couder@gmail.com> wrote: > > On Wed, Jul 24, 2024 at 7:12 AM Chandra Pratap > <chandrapratap3519@gmail.com> wrote: > > > > On Tue, 23 Jul 2024 at 22:39, Junio C Hamano <gitster@pobox.com> wrote: > > > > > - 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. > > I agree it might have been better to start by replacing the pq > implementation in reftable/ with our own first, as there would be no > need for this patch series, but Chandra's GSoC is about replacing the > unit test framework in reftable/ with our own which is still valuable. > And I think that at this point it is just simpler to not get > distracted by replacing the pq implementation now. It's also not like > changing the unit test framework would make replacing the pq > implementation harder. > > > 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. > > Yeah, if it had been discussed and agreed on earlier, I think > replacing the pq implementation would have made sense. Now I think > it's a bit late at this stage in Chandra's GSoC to go in this > direction though. I think it's better if he can focus on finishing to > replace the unit test framework. I'm actually more or less done with porting most of the reftable tests to the unit test framework and it shouldn't be long before patches for rest of the tests see the mailing list, so I can definitely make time for other endeavors like these. The only thing that I think is holding us back from making a change like what Junio suggests is 'how far are we willing to butcher our copy of reftable/?' From what Patrick said earlier, I think the answer to that is 'it is fine to modify reftable/ tests because they don't need to be uniform across all implementations of reftable, but modifying other parts to be more dependent on Git's internals is a no-go.'
On Wed, Jul 24, 2024 at 09:17:55AM +0200, Christian Couder wrote: > On Wed, Jul 24, 2024 at 7:12 AM Chandra Pratap > <chandrapratap3519@gmail.com> wrote: > > On Tue, 23 Jul 2024 at 22:39, Junio C Hamano <gitster@pobox.com> wrote: > > 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. > > Yeah, if it had been discussed and agreed on earlier, I think > replacing the pq implementation would have made sense. Now I think > it's a bit late at this stage in Chandra's GSoC to go in this > direction though. I think it's better if he can focus on finishing to > replace the unit test framework. Replacing the priority queue with the one we already have will create additional work in the future when we want to get the reftable library back into a state where it can be used as a standalone library again. I know that it's currently a mess anyway, but I've heard from multiple folks already who are interested in using the reftable library in their own C projects (most importantly libgit2). If we want Git to be the reftable upstream for such projects, then we should play nice and not make their lifes harder. I plan to work on portability work as soon as somebody properly commits to integrating reftables into their project. This will probably come in the form of: - Removing all Git-specific includes in the "reftable/" directory. - Declaring a list of "shim" functions and types that users of the reftable library need to use. - Implementing those shim functions for Git. Of course, those shims will closely follow the interfaces that we have in Git. E.g. there will shims for "strbuf", the tempfile interface, and everything else that we currently use in the reftable library. So ultimately, I expect that the shim implementations will simply look like the following: ``` typedef struct strbuf reftable_buf; static inline void reftable_buf_add(struct strbuf reftable_buf *buf, const void *data, size_t len) { strbuf_add(buf, data, len); } ``` While we could also shim out the priority queue, I don't really think that it is worth it. Patrick
Patrick Steinhardt <ps@pks.im> writes: > Of course, those shims will closely follow the interfaces that we have > in Git. E.g. there will shims for "strbuf", the tempfile interface, and > everything else that we currently use in the reftable library. So > ultimately, I expect that the shim implementations will simply look like > the following: > > ``` > typedef struct strbuf reftable_buf; > > static inline void reftable_buf_add(struct strbuf reftable_buf *buf, > const void *data, size_t len) > { > strbuf_add(buf, data, len); > } > ``` > > While we could also shim out the priority queue, I don't really think > that it is worth it. OK, I am fine if somebody wants to spend cycles to move the reftable_pq tests to the unit-test framework. I just wasn't sure what the future plans were, and one obvious direction to replace reftable_pq would invalidate such work, and I wanted to make sure everybody involved in this effort is aware of that. Thanks.