Re: [PATCH v3 09/11] core.fsyncmethod: tests for batch mode

>
>
> On Thu, Mar 24 2022, Neeraj Singh via GitGitGadget wrote:
>
> > From: Neeraj Singh <neerajsi@microsoft.com>
> >
> > Add test cases to exercise batch mode for:
> >  * 'git add'
> >  * 'git stash'
> >  * 'git update-index'
> >  * 'git unpack-objects'
> >
> > These tests ensure that the added data winds up in the object database.
> >
> > In this change we introduce a new test helper lib-unique-files.sh. The
> > goal of this library is to create a tree of files that have different
> > oids from any other files that may have been created in the current test
> > repo. This helps us avoid missing validation of an object being added
> > due to it already being in the repo.
> >
> > Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> > ---
> >  t/lib-unique-files.sh  | 32 ++++++++++++++++++++++++++++++++
> >  t/t3700-add.sh         | 28 ++++++++++++++++++++++++++++
> >  t/t3903-stash.sh       | 20 ++++++++++++++++++++
> >  t/t5300-pack-object.sh | 41 +++++++++++++++++++++++++++--------------
> >  4 files changed, 107 insertions(+), 14 deletions(-)
> >  create mode 100644 t/lib-unique-files.sh
> >
> > diff --git a/t/lib-unique-files.sh b/t/lib-unique-files.sh
> > new file mode 100644
> > index 00000000000..74efca91dd7
> > --- /dev/null
> > +++ b/t/lib-unique-files.sh
> > @@ -0,0 +1,32 @@
> > +# Helper to create files with unique contents
> > +
> > +# Create multiple files with unique contents within this test run. Takes the
> > +# number of directories, the number of files in each directory, and the base
> > +# directory.
> > +#
> > +# test_create_unique_files 2 3 my_dir -- Creates 2 directories with 3 files
> > +#                                     each in my_dir, all with contents
> > +#                                     different from previous invocations
> > +#                                     of this command in this run.
> > +
> > +test_create_unique_files () {
> > +     test "$#" -ne 3 && BUG "3 param"
> > +
> > +     local dirs="$1" &&
> > +     local files="$2" &&
> > +     local basedir="$3" &&
> > +     local counter=0 &&
> > +     test_tick &&
> > +     local basedata=$basedir$test_tick &&
> > +     rm -rf "$basedir" &&
> > +     for i in $(test_seq $dirs)
> > +     do
> > +             local dir=$basedir/dir$i &&
> > +             mkdir -p "$dir" &&
> > +             for j in $(test_seq $files)
> > +             do
> > +                     counter=$((counter + 1)) &&
> > +                     echo "$basedata.$counter">"$dir/file$j.txt"
> > +             done
> > +     done
> > +}
>
> Having written my own perf tests for this series, I still don't get why
> this is needed, at all.
>
> tl;dr: the below: I think this whole workaround is because you missed
> that "test_when_finished" exists, and how it excludes perf timings.
>

> I.e. I get that if we ran this N times we'd want to wipe our repo
> between tests, as for e.g. "git add" you want it to actually add the
> objects.
>
> It's what I do with the "hyperfine" command in
> https://lore.kernel.org/git/RFC-patch-v2-4.7-61f4f3d7ef4-20220323T140753Z-avarab@gmail.com/
> with the "-p" option.
>
> I.e. hyperfine has a way to say "this is setup, but don't measure the
> time", which is 1/2 of what you're working around here and in 10/11.
>
> But as 10/11 shows you're limited to one run with t/perf because you
> want to not include those "setup" numbers, and "test_perf" has no easy
> way to avoid that (but more on that later).
>
> Which b.t.w. I'm really skeptical of as an approach here in any case
> (even if we couldn't exclude it from the numbers).
>
> I.e. yes what "hyperfine" does would be preferrable, but in exchange for
> avoiding that you're comparing samples of 1 runs.
>
> Surely we're better off with N run (even if noisy). Given enough of them
> the difference will shake out, and our estimated +/- will narrow..
>
> But aside from that, why isn't this just:
>
>         for cfg in true false blah
>         done
>                 test_expect_success "setup for $cfg" '
>                         git init repo-$cfg &&
>                         for f in $(test_seq 1 100)
>                         do
>                                 >repo-$cfg/$f
>                         done
>                 '
>
>                 test_perf "perf test for $cfg" '
>                         git -C repo-$cfg
>                 '
>         done
>
> Which surely is going to be more accurate in the context of our limited
> t/perf environment because creating unique files is not sufficient at
> all to ensure that your tests don't interfere with each other.
>
> That's because in the first iteration we'll create N objects in
> .git/objects/aa/* or whatever, which will *still be there* for your
> second test, which will impact performance.
>
> Whereas if you just make N repos you don't need unique files, and you
> won't be introducing that as a conflating variable.
>
> But anyway, reading perf-lib.sh again I haven't tested, but this whole
> workaround seems truly unnecessary. I.e. in test_run_perf_ we do:
>
>         test_run_perf_ () {
>                 test_cleanup=:
>                 test_export_="test_cleanup"
>                 export test_cleanup test_export_
>                 "$GTIME" -f "%E %U %S" -o test_time.$i "$TEST_SHELL_PATH" -c '
>                         [... code we run and time ...]
>                 '
>                 [... later ...]
>                 test_eval_ "$test_cleanup"
>         }
>
> So can't you just avoid this whole glorious workaround for the low low
> cost of approximately one shellscript string assignment? :)
>
> I.e. if you do:
>
>         setup_clean () {
>                 rm -rf repo
>         }
>
>         setup_first () {
>                 git init repo &&
>                 [make a bunch of files or whatever in repo]
>         }
>
>         setup_next () {
>                 test_when_finished "setup_clean" &&
>                 setup_first
>         }
>
>         test_expect_success 'setup initial stuff' '
>                 setup_first
>         '
>
>         test_perf 'my perf test' '
>                 test_when_finished "setup_next" &&
>                 [your perf test here]
>         '
>
>         test_expect_success 'cleanup' '
>                 # Not really needed, but just for completeness, we are
>                 # about to nuke the trash dir anyway...
>                 setup_clean
>         '
>
> I haven't tested (and need to run), but i'm pretty sure that does
> exactly what you want without these workarounds, i.e. you'll get
> "trampoline setup" without that setup being included in the perf
> numbers.
>
> Is it pretty? No, but it's a lot less complex than this unique file
> business & workarounds, and will give you just the numbers you want, and
> most importantly you car run it N times now for better samples.
>
> I.e. "what you want" sans a *tiny* bit of noise that we use to just call
> a function to do:
>
>     test_cleanup=setup_next
>
> Which we'll then eval *after* we measure your numbers to setup the next
> test.