[DTrace-devel] [PATCH 07/12] Add tests for various options and tunables

[Eugene Loh]

On 7/17/22 21:43, Kris Van Hees wrote:

> On Sun, Jul 17, 2022 at 08:38:45AM -0700, Eugene Loh via DTrace-devel wrote:
>> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
>> The tests
>>      test/unittest/options/tst.D.d
>>      test/unittest/options/tst.define.d
>> should be fine, but might as well add .r results files for them, to get that
>> special reassurance that :BEGIN shows up.
> Well, I care less whether BEGIN shows up or not.  That is not the condition we
> are testing here.  The way the test is written, it will not compile if the
> define is not honoured.
When we know the expected output, a .r file is a "free" extra check.  I 
think there have been a number of times in the last months that 
self-checking tests were passing spuriously but fixed just by adding a 
.r file.  It strikes me as a simple "best practice."
>
>> The tests
>>      test/unittest/options/tst.ctfpath.sh:
>>      test/unittest/options/tst.ctypes.sh:
>>      test/unittest/options/tst.dtypes.sh:
>> all have dangling tabs (line that's empty other than a tab).
> Will fix.
>
>> The tests
>>      test/unittest/options/tst.core.sh
>>      test/unittest/options/tst.ctypes.sh
>>      test/unittest/options/tst.dtypes.sh
>> write files to the current directory.  That seems like a dubious practice.
>> Most *.sh tests that write files write either to $tmpdir or to their own
>> $DIRNAME directories.  In particular, there is no guarantee that I know of
>> that the DTrace source directory won't ever have, say, ctypes.* or dtypes.*
>> files in it (that would then get wiped out by our testing).
> They write files to the current directory, which when running the testsuite,
> is actually a temporary directory created for that specific test.  In other
> words, during testsuite runs, those files are created in a temporary directory
> that is wiped once the test has completed.
Would you be willing to check that?  Again, most (all?) .sh tests that 
write files use either $tmpdir or $DIRNAME.  E.g., with this patch, try:

% cd dtrace-util
% touch ctypes.my_file
% sudo ./runtest.sh test/unittest/options/tst.ctypes.sh
% ls ctypes.my_file

>> The test/unittest/options/tst.debug.sh test seems fine, but would you be
>> open to the following enhancement?
>>          - exit(sum > 0 ? 0 : 1);
>>          + exit(cnt["libdtrace"] > 0 && sum > 0 ? 0 : 1);
> Why single out libdtrace?  And if someone (for whatever reason) decides to
> get rid of all debug output statements for libdtrace, while still keeping those
> for libproc, then this test wuld fail even though -xdebug did actually do its
> job correctly.

The point is to be a tad more discriminating than simply "any line of 
output where DEBUG is the second word (and I can convert the third word 
to a positive integer)".  Checking for libdtrace (and libproc!) is easy 
and makes the test that much more worthwhile.

The scenario that someone gets rid of all debug statements for libdtrace 
and yet expects -xdebug to work does not concern me.  The argument only 
raises the prospect that someone gets rid of all debug statements for 
both libdtrace and libproc.  Technically, that's a strictly smaller 
subset of scenarios, but they both seem like noise.

>> The test test/unittest/options/tst.defaultargs2.d should have a .r results
>> file.
> The exit($3) already verifies that, so there is no expected results necessary.
> The reason I do output the values is simply that it makes it easier to debug
> a test failure.

Again, .r files are simple "best practices."  There is no reason to make 
this test "as narrow as possible."  If a .r file would cause this test 
to fail for some reason we cannot currently imagine, we would want to know.

>> The
>>      test/unittest/options/tst.Z.d
>>      test/unittest/options/tst.zdefs.d
>> tests are fine, but how about a more-clearly-illegitimate probe name?  I
>> could imagine an "oracle" probe.  How about "foo" or
>> "nonexisting_probe_name" instead?
> I honestly cannot imagine an "oracle" probe, and if someone tries to add one,
> I will reject it.  But given that I do not have control over all probe names,
> I'll update the tests to specify the provider as well, to ensure that we're
> referring to an 'oracle' probename in a provider we do have control over.

We routinely pick names (of variables, functions, files, etc.) to be 
descriptive.  In this case,, we need a probe name that does not exist.  
I do not doubt that "oracle" would work.  I simply argue that it is not 
descriptive or suggestive of what it is intended to do.  Let us make 
simple choices that make things clearer for the human reader.