Re: [PATCH 2/7] reftable/basics: improve `binsearch()` test
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 25, 2024, 10:07 UTC
- Message-ID
- <ZgFM5pUU4TKRbiMt@tanuki>
- In-Reply-To
- <zlj4jlfv645bxqgtjkrm3qdan3b7q4lzhxv3nqp7qagycri323@m5hhhxwuoetv>
On Fri, Mar 22, 2024 at 01:46:56PM -0500, Justin Tobler wrote:
Show 35 quoted lines
> On 24/03/22 01:22PM, Patrick Steinhardt wrote:
> > The `binsearch()` test is somewhat weird in that it doesn't explicitly
> > spell out its expectations. Instead it does so in a rather ad-hoc way
> > with some hard-to-understand computations.
> >
> > Refactor the test to spell out the needle as well as expected index for
> > all testcases. This refactoring highlights that the `binsearch_func()`
> > is written somewhat weirdly to find the first integer smaller than the
> > needle, not smaller or equal to it. Adjust the function accordingly.
> >
> > While at it, rename the callback function to better convey its meaning.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > reftable/basics_test.c | 55 ++++++++++++++++++++++++------------------
> > 1 file changed, 31 insertions(+), 24 deletions(-)
> >
> > diff --git a/reftable/basics_test.c b/reftable/basics_test.c
> > index dc1c87c5df..85c4d1621c 100644
> > --- a/reftable/basics_test.c
> > +++ b/reftable/basics_test.c
> > @@ -12,40 +12,47 @@ license that can be found in the LICENSE file or at
> > #include "test_framework.h"
> > #include "reftable-tests.h"
> >
> > -struct binsearch_args {
> > - int key;
> > - int *arr;
> > +struct integer_needle_lesseq {
> > + int needle;
> > + int *haystack;
> > };
>
> This is probably just personal preference, but I think `key` and `arr`
> in this case are a bit more straightforward.I was trying to make this consistent across all the callsites, and here I personally think that `haystack` and `needle` are well understood by most folks and generic enough.
> I do like that we rename > the args to be more specific. Do we want to also append `_args` to > denote that it is an argument set? Maybe `integer_lesseq_args`?
Oh, yeah, that's an oversight indeed.
Patrick