[DTrace-devel] [PATCH] test: Clean up tst.coverage.d

[Kris Van Hees]

On Tue, Jan 23, 2024 at 04:13:55PM -0500, Eugene Loh wrote:
> On 1/23/24 11:55, Kris Van Hees wrote:
> 
> > On Thu, Nov 02, 2023 at 06:36:44PM -0400, eugene.loh at oracle.com wrote:
> >> The .t file was not executable and therefore not being used.
> >> Further, the .d test specifies another @@trigger explicitly.
> >> So remove the unused .t file.
> >>
> >> Clean the .d file up a little, mostly to use aggregations and
> >> thereby reduce the volume of output.
> > This patch has an issue in the sense that it will actually PASS without the
> > support for offset-based pid providers being present even.  In fact, if the
> > only probe that ever fires is the tick probe, this test will report PASS.
> >
> > That's an issue.
> 
> Well, I suppose so.? But that was an issue in the original test. This 
> patch fixes some things in the test -- notably, reducing the staggering 
> amount of output that was appearing in test logs -- but does not fix 
> everything in the test.? The point of the patch was to clean the test up 
> some, but not to perfect it.? The test claims to check whether we can 
> trace safely, which of course begs the question whether we are tracing 
> accurately, etc.? Anyhow, the limitation you point out is arguably 
> consistent with the original intent of this very tolerant test.
> 
> Now, clearly we would like somewhere to test that every offset is 
> getting traced and that the counts are correct.? That is a much more 
> stringent test than tst.coverage.d ever was.? This more stringent test 
> is provided by the commit "pid: Support probe names specifying offsets", 
> in test/unittest/pid/tst.offsets.sh
> 
> So how about we go with this patch, let tst.coverage.d continue to be 
> the anemic test it always was (without letting it swamp the log file 
> anymore), and relying on tst.offsets.sh for the more stringent tests we 
> would like to see.

My concern is that this test currently can PASS even when things are broken,
or non-existant, and from what I can see, even if the probing of individual
instructions were to raise a fault, the test would still PASS because there
is not even an ERROR probe clause to catch that.

I see your point of what you were trying to fix in this test, but it then
still remains a test that (from what I can see) does not actually test anything
more than perhaps that DTrace does not crash or exit with an error code.
But then the test is very marginally useful, and certainly not named well
either.

I think we should put the bar at least a little bit higher by at a minimum
catching EROR probe firings (just an exit(1) would be enough) and having some
verification that there is *some* output generated from probing instructions.
Because otherwise it *is* possible that an unsafe probing operation may not
get caught because it didn't cause a crash or other fatal condition.

> >> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> >> ---
> >>   test/unittest/pid/tst.coverage.d | 16 ++++------------
> >>   test/unittest/pid/tst.coverage.t |  8 --------
> >>   2 files changed, 4 insertions(+), 20 deletions(-)
> >>   delete mode 100644 test/unittest/pid/tst.coverage.t
> >>
> >> diff --git a/test/unittest/pid/tst.coverage.d b/test/unittest/pid/tst.coverage.d
> >> index 9670665a..3313d7b5 100644
> >> --- a/test/unittest/pid/tst.coverage.d
> >> +++ b/test/unittest/pid/tst.coverage.d
> >> @@ -1,6 +1,6 @@
> >>   /*
> >>    * Oracle Linux DTrace.
> >> - * Copyright (c) 2006, 2021, Oracle and/or its affiliates. All rights reserved.
> >> + * Copyright (c) 2006, 2023, 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.
> >>    */
> >> @@ -13,22 +13,14 @@
> >>    * ASSERTION: test that we can trace every instruction safely
> >>    *
> >>    * SECTION: pid provider
> >> - *
> >>    */
> >>   
> >> -BEGIN
> >> +pid$1:a.out::
> >>   {
> >> -	/*
> >> -	 * Let's just do this for 2 seconds.
> >> -	 */
> >> -	timeout = timestamp + 2000000000;
> >> +	@[probename] = count();
> >>   }
> >>   
> >> -pid$1:a.out::
> >> -{}
> >> -
> >> -profile:::tick-4
> >> -/timestamp > timeout/
> >> +profile:::tick-2sec
> >>   {
> >>   	exit(0);
> >>   }
> >> diff --git a/test/unittest/pid/tst.coverage.t b/test/unittest/pid/tst.coverage.t
> >> deleted file mode 100644
> >> index ac05014b..00000000
> >> --- a/test/unittest/pid/tst.coverage.t
> >> +++ /dev/null
> >> @@ -1,8 +0,0 @@
> >> -#!/bin/bash
> >> -#
> >> -# Oracle Linux DTrace.
> >> -# Copyright (c) 2006, 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.
> >> -#
> >> -while true; do env > /dev/null; done
> >> -- 
> >> 2.18.4
> >>
> >>
>