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

[Eugene Loh]

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.

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.

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