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

[Kris Van Hees]

On Mon, Jul 18, 2022 at 10:30:20AM -0700, Eugene Loh wrote:
> 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."

But what does it check extra?  The -D and -xdefine options are compiler (well,
preprocessor) related options.  Once we are past the compilation stage, we
really do not care anymore.  In fact, maybe I should just add the -e option
to surpress execution altogether - it will speed up the test as well.

> > > 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

Yes, my mistake.  I was under the (wrong) impression that tests are executed
with cwd set to the tmpdir.  But that is not the case.  I talked to Nick about
making that change in the future, for obvious safety reasons.

I will for now adjust the test to use tmpdir explicitly.

> > > 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.

I do not like the notion of expecting specific components to report debug output
when -xdebug is specified.  All we are checking is that there is debug output
when -xdebug is specified - that is what the option does.  Any additional
expectations on the output are not adding to what needs to be tested here.

> > > 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.

I do not see .r files as best practice for all cases.  This test does not
depend on any specific output.  The only reason I generate output is that it
makes it slightly easier when debugging a test failure - by looking in the
runtest.log.  But output verification isn't the way this is tested.

> > > 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.

fake:oracle:probe