[DTrace-devel] [PATCH 3/5] Fix size for typecast tuple (and aggregation key) components

[Kris Van Hees]

On Thu, Sep 08, 2022 at 11:13:25PM -0400, Eugene Loh wrote:
> On 9/8/22 17:20, Kris Van Hees wrote:
> 
> > On Thu, Sep 08, 2022 at 05:07:13PM -0400, Eugene Loh wrote:
> > > On 9/8/22 16:31, Kris Van Hees wrote:
> > > 
> > > > On Thu, Sep 08, 2022 at 02:10:25PM -0400, Eugene Loh via DTrace-devel wrote:
> > > > 
> > > > > Also, the patch should have a test.  Here is a suggestion:
> > > > Test added (different from the one you suggested because a single case is quite
> > > > sufficient).
> > > A single case is sufficient for catching one, already known error. More test
> > > cases are helpful for checking a little more broadly for errors one has not
> > > yet thought of.  The additional cases are cheap and better resemble "test
> > > coverage."  No cost.  Greater confidence.
> > I'd be interested to know what errors you think the more broad test would
> > catch, and how those are related to this patch.  If there are genuine concerns
> > about these cases, I think it would warrant a separate patch.
> 
> There is a philosophical difference here, perhaps.  One approach is to say,
> "I found a particular problem;  so what is the narrowest test that would
> catch that problem?"  Another is to say, "There was a problem;  how can I
> test more broadly for some range of similar problems?  How can I reassure
> myself I'm not overlooking somewhat related problems?"  Increasing the test
> coverage for basically zero cost makes sense to me.  The test you added has
> "sufficient" coverage for a specific implementation.  But what if one tuple
> writes some garbage and then the next tuple overwrites (corrects) that
> garbage?  You're okay in the "downsizing" (64 to 32 bits) case.  The new,
> limited test checks for the specific fix that was implemented.  But what
> about when a tuple is being "upsized"?  What is the aversion to more
> rigorous testing?  If we're only going to test for bugs we have already
> anticipated, we're overconfident in our ability to predict problems.
> 
> I guess I just think there are many more bugs to find, and we do ourselves a
> service by widening test coverage whenever there are convenient
> opportunities (like here) to do so.  When we add tests, rather than making
> them as narrow as possible, let us make them as wide as possible within
> reason.  When we fix a bug, let us not assume it was very narrow.  Let us
> add testing that demonstrates we've searched more broadly for related
> problems.

I overall agree, but.... the test you propose is not really doing much more
than the one I put in the patch.  You are essentially testing the same thing
multiple times rather than adding testing for other potential issues.  So, it
certainly needs work.  As it is now, it primarily exercises the typecasting
code, actually.

Besides that, I am certainly open to a more broad test (or tests) to be added,
but that (again) is not related to this patch but is rather an entity on its
own, expanding the coverage of our testsuite.  The point of the test in this
patch is to ensure that the problem it exercises has been resolved.

So, I would propose you rework the proposed test so that it exercises different
scenarios rather than the same one over and over again.  And submit it as a
separate patch to expand test coverage in the testsuite.

> > > > v2 posted.
> > > In any case, it appears that v2 has no tests at all.
> > v3 posted to fix that (posted an older copy by accident).
> > > > > BEGIN {
> > > > >     @a[11111111, 22222222,        0x7fff7fff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +1);
> > > > >     @a[11111111, 22222222, (int  )0x7fff7fff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +2);
> > > > >     @a[11111111, 22222222, (short)0x7fff7fff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +4);
> > > > >     @a[11111111, 22222222, (long )0x7fff7fff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +8);
> > > > >     @a[11111111, 22222222,        0x7fffffff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +16);
> > > > >     @a[11111111, 22222222, (int  )0x7fffffff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +32);
> > > > >     @a[11111111, 22222222, (short)0x7fffffff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +64);
> > > > >     @a[11111111, 22222222, (long )0x7fffffff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +128);
> > > > >     @a[11111111, 22222222,        0xffff7fff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +256);
> > > > >     @a[11111111, 22222222, (int  )0xffff7fff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +512);
> > > > >     @a[11111111, 22222222, (short)0xffff7fff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +1024);
> > > > >     @a[11111111, 22222222, (long )0xffff7fff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +2048);
> > > > >     @a[11111111, 22222222,        0xffffffff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +4096);
> > > > >     @a[11111111, 22222222, (int  )0xffffffff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +8192);
> > > > >     @a[11111111, 22222222, (short)0xffffffff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +16384);
> > > > >     @a[11111111, 22222222, (long )0xffffffff, 44444444, 55555555] =
> > > > > lquantize(3, 0, 10, 1, +32768);
> > > > >     exit(0);
> > > > > }