[DTrace-devel] [PATCH 03/12] Check that stackdepth is nonzero in test

[Kris Van Hees]

On Thu, Jun 03, 2021 at 03:58:58PM -0400, Eugene Loh wrote:
> On 6/3/21 2:25 PM, Kris Van Hees wrote:
> 
> > On Fri, May 28, 2021 at 02:35:07PM -0400, eugene.loh at oracle.com wrote:
> >> From: Eugene Loh <eugene.loh at oracle.com>
> >>
> >> This test was passing gratuitously since neither stack() nor
> >> stackdepth was working properly:  stack() reported nothing and
> >> stackdepth always reported 0.  Change the test to require a
> >> nonempty stack.  For now, the test will XFAIL.
> > This is a bit of a judgement call I guess.  The purpose of the test is to
> > validate that the stackdepth is consistent with the stack() output.  When
> > stack() reports no data and stackdepth returns 0, I would argue that the
> > test is allowed to PASS.  Furthermore, once stack() is implemented (as you
> > do in followup patches), this test remains valid and becomes more important
> > to ensure that stack() and stackdepth remain in sync.
> >
> > So, I would actually just leave out this patch because it is not needed.
> > And if there is a circumstance where a stack() could validly be empty (i.e.
> > where the stack up to the probe itself is to be ignored because it is part
> > of the calling mechanism or something like that).  Even if that is very
> > unlikely, I don't think we should discount it.
> >
> > And there is no loss to keeping this test as-is.
> 
> I would like to push back.  The stackdepth bvar is a user-visible 
> feature.  It needs to have at least one test.  (Stuff that accepts 
> basically any value for stackdepth, regardless of how ridiculous, or 
> simply rejects obviously bad values like -1 don't really count.)  At the 
> point of this patch, stackdepth was quite broken, but no test failed for 
> that reason.  That is a clear indictment of our test coverage.

Well, that is a matter of opinion.  Stack was not resulting in any data, which
is consistent with stackdepth being 0.  So there was a consistency between the
two pieces of data.  Unfortunately, any validation beyond that would require
actual knowledge of what stack is expected.  This test validated that the value
obtained was consistent which is reasonable.

> The point of the test is to check that stackdepth returns the correct 
> value.  How one checks that is tricky.  The test attempts to use stack() 
> for this purpose, but clearly it makes sense to check that it's a 
> reasonable benchmark.
> 
> There could be an empty stack(), but not in the case of this test.

Well, that is not correct because in this patch the probe is still BEGIN, and
with your stack implementation from a patch further in the series, the stack
for the BEGIN probe *is* empty.  So, with the test in this patch, you are
actually causing a failure even with the stack implementation added in.

So, at a minimum you then should change the probe that this test uses, and
document why you use a particular probe.

> The loss to keeping this test as is is bogus test coverage.  The gain to 
> the patch is to have a better test.
> 
> Anyhow, omitting this patch doesn't solve anything since it still leaves 
> open the questions of what to do as stack() and stackdepth are fixed.  
> Test regressions?  No changes in test results?
> 
> >> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> >> ---
> >>   test/unittest/stackdepth/tst.value.d   | 3 ++-
> >>   test/unittest/stackdepth/tst.value.r.p | 4 +++-
> >>   2 files changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/test/unittest/stackdepth/tst.value.d b/test/unittest/stackdepth/tst.value.d
> >> index 857728bd..97cc54df 100644
> >> --- a/test/unittest/stackdepth/tst.value.d
> >> +++ b/test/unittest/stackdepth/tst.value.d
> >> @@ -1,9 +1,10 @@
> >>   /*
> >>    * Oracle Linux DTrace.
> >> - * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
> >> + * Copyright (c) 2006, 2021, Oracle and/or its affiliates. All rights reserved.
> >>    * Licensed under the Universal Permissive License v 1.0 as shown at
> >>    * http://oss.oracle.com/licenses/upl.
> >>    */
> >> +/* @@xfail: dtv2 */
> >>   
> >>   #pragma D option quiet
> >>   
> >> diff --git a/test/unittest/stackdepth/tst.value.r.p b/test/unittest/stackdepth/tst.value.r.p
> >> index d5143e38..fa83eb0c 100755
> >> --- a/test/unittest/stackdepth/tst.value.r.p
> >> +++ b/test/unittest/stackdepth/tst.value.r.p
> >> @@ -1,6 +1,6 @@
> >>   #!/usr/bin/gawk -f
> >>   # Oracle Linux DTrace.
> >> -# Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
> >> +# Copyright (c) 2016, 2021, Oracle and/or its affiliates. All rights reserved.
> >>   # Licensed under the Universal Permissive License v 1.0 as shown at
> >>   # http://oss.oracle.com/licenses/upl.
> >>   
> >> @@ -26,6 +26,8 @@ END {
> >>   		printf "Stack depth too large (%d > %d)\n", depth, count;
> >>   	else if (count > depth)
> >>   		printf "Stack depth too small (%d < %d)\n", depth, count;
> >> +	else if (count == 0)
> >> +		printf "Stack depth is 0\n";
> >>   	else
> >>   		printf "Stack depth OK\n";
> >>   }
> >> -- 
> >> 2.18.4
> >>
> >>
> >> _______________________________________________
> >> DTrace-devel mailing list
> >> DTrace-devel at oss.oracle.com
> >> https://oss.oracle.com/mailman/listinfo/dtrace-devel
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel