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

[Kris Van Hees]

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>
> 
> Plus a bunch of tiny comments:
> 
> In the commit message:
>     Tests added for:
>         -xargref
>         -xbufsize               <<< here <<<<<<<<+
>         -xcore                                   ^
>         -xcppargs                                ^
>         -xcpphdrs                                ^
>         -xcpppath                                ^
>         -xctypes                                 ^
>         -xbufsize (and -b)      >>> move this >>>+
>         -xdebug
>         [...]
> 
> In the commit message:
>     -   -xbpglog
>     +   -xbpflog
>     -   -xbpglogsize
>     +   -xbpflogsize
> 
> The tests for xbpflog[size] have been moved.  I'm curious about the scope of
> test/unittest/options and our test coverage.  There are "pragma" tests for
> bpflog[size].  Should the new directory also have "pragma" tests for other
> options as well?

When bpflog and bpflogsize were introduced we didn't really have much in terms
of options testing, and especially pragma-based testing was sorely lacking.
However, the very fact that we have pragma-based testing for bpflog and
bpflogsize helps fill that gap.  Teting that every option can be set using
oragma is not necessary because the pragma implementation uses a standard way
to set options in response to seeing a pragma statement.  If it works for one,
it word for all (modulo the fact that some options cannot worked when they are
specified with a pragma in the source file).

> The test/unittest/options/err.D_EMPTY.errtags.d test is okay, but a little
> weird in that all the err.D_* tests already depend on -xerrtags anyhow.  But
> I guess that's okay... some redundant/superfluous testing.

Redundancy is not a bad thing - and here the purpose is to specifically test
that option.  Other tests may fail if this option is not implemented correctly
but they may fail in ways that do not immediately show that the problem is with
this ootin rather than some other issue causing the failure.

> Instead of fixing files
>     test/unittest/options/err.D_PRAGCTL_INVAL.no-cpp.d
>     test/unittest/options/tst.cpp.d
> just introduce them "correctly" (in the earlier patch in this same series).

Yes (see my earlier reply)

> The tests
>     test/unittest/options/err.b[ufsize]-too-low.d
>     test/unittest/options/tst.b[ufsize].d
> are nice.  They do, however, depend sensitively on the current
> implementation.  In a way that's okay.  If the implementation changes and
> one forgets to update the tests, at least some test failures will remind the
> developer to update the tests.  But the developer might not be sufficiently
> circumspect to realize some non-failing tests also need updating.  So, I
> suggest adding a comment to all four tests reminding developers that all
> four have to be maintained in sync.

I think that is overkill and not something we do with any other tests, and
there are a lot of tests that should be maintained in symc and sometimes they
are indeed not).  Adding such comments actually creates an addition piece of
information that needs to be maintained in sync, so it may make the situation
worse rather than helping.

> The test test/unittest/options/err.no-errtags.d has the comment:
>     ASSERTION: Without the -xerrtags option, errors are printed with tag.
> Is that right, or was "s/with/without/" intended?

Yes, I need to fix that,  Thanks for catching it.
> 
> 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.

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

> The test test/unittest/options/tst.core.sh should exit $rc not $rcc.  Also,
> out of curiosity, for the awk script, how about:
>     /ELF/ && /core file/ && /dtrace/ { exit(0); }
>     END { exit(1); }

The exit(0) will cause the script to terminate, which triggers the END action
to be executed by awk, which will then do exit(1).  In other words, you will
always get an error, even when the valid condition is triggered.

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

> For the tests
>     test/unittest/options/tst.destructive.d
>     test/unittest/options/tst.w.d
>     test/unittest/options/tst.zdefs.d
>     test/unittest/options/tst.Z.d
> it would also be nice to have tests that confirm that the absence of the
> option triggers an error -- e.g., the same way there are tests to check
> behavior in the absence of argref, cpp, defaultargs, empty, or errtags. 
> That is, it would be nice to have tests like
>     err*no-destructive.d
>     err*no-w.d
>     err*no-zdefs.d
>     err*no-Z.d

Good point.  I will add them.
> 
> 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.