[PATCH 1/3] add testcases for function definitions

[PATCH 1/3] add testcases for function definitions

[Zhou Wenjian]

* testsuite/systemtap.base/func_definition.exp: New test case.
* testsuite/systemtap.base/func_definition.stp: New test file.
---
 testsuite/systemtap.base/func_definition.exp | 13 ++++++
 testsuite/systemtap.base/func_definition.stp | 60 ++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)
 create mode 100755 testsuite/systemtap.base/func_definition.exp
 create mode 100755 testsuite/systemtap.base/func_definition.stp

diff --git a/testsuite/systemtap.base/func_definition.exp b/testsuite/systemtap.base/func_definition.exp
new file mode 100755
index 0000000..2fddf9b
--- /dev/null
+++ b/testsuite/systemtap.base/func_definition.exp
@@ -0,0 +1,13 @@
+# Check function definitions
+
+set test "func_definition"
+if {![installtest_p]} { untested "$test"; return }
+
+foreach runtime [get_runtime_list] {
+    if {$runtime != ""} {
+	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string} \
+	    --runtime=$runtime
+    } else {
+	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}
+    }
+}
diff --git a/testsuite/systemtap.base/func_definition.stp b/testsuite/systemtap.base/func_definition.stp
new file mode 100755
index 0000000..eaa8d94
--- /dev/null
+++ b/testsuite/systemtap.base/func_definition.stp
@@ -0,0 +1,60 @@
+/*
+ * func_definition.stp
+ *
+ * Check function definitions
+ */
+probe begin {
+    println("systemtap starting probe")
+}
+
+
+function f1(arg:long)
+{
+    if (arg == 2015)
+        println("systemtap test success")
+    else
+        printf("systemtap test failure - arg of f1:%d != 2015\n", arg)
+}
+
+function f2(arg)
+{
+    if (arg == 2015)
+        println("systemtap test success")
+    else
+        printf("systemtap test failure - arg of f2:%d != 2015\n", arg)
+}
+
+function f3:long()
+{
+    return 2015
+}
+
+function f4()
+{
+    return 2015
+}
+
+function f5()
+{
+    println("systemtap test success")
+}
+
+probe end {
+    println("systemtap ending probe")
+
+    f1(2015)
+
+    f2(2015)
+
+    if (f3() == 2015)
+        println("systemtap test success")
+    else
+        printf("systemtap test failure - return_value of f3:%d != 2015\n", f3())
+
+    if (f4() == 2015)
+        println("systemtap test success")
+    else
+        printf("systemtap test failure - return_value of f4:%d != 2015\n", f4())
+
+    f5()
+}
-- 
1.8.3.1

[PATCH 3/3] add more test cases for timer

[Zhou Wenjian]

	* testsuite/systemtap.base/timers.exp: update for new test cases
	* testsuite/systemtap.base/timers.stp: add more test cases
---
 testsuite/systemtap.base/timers.exp |  2 +-
 testsuite/systemtap.base/timers.stp | 54 +++++++++++++++++++++++++++++++++++--
 2 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/testsuite/systemtap.base/timers.exp b/testsuite/systemtap.base/timers.exp
index 70ed341..7000b3a 100755
--- a/testsuite/systemtap.base/timers.exp
+++ b/testsuite/systemtap.base/timers.exp
@@ -11,6 +11,6 @@ proc sleep_ten_secs {} {
 #check to see whether get the marker indicating the probe is loaded and running
 #should check to see whether exited for some reason
 #should be error is something else is printed.
-set output_string "(\\w+ = \\d+\r\n){7}${all_pass_string}(WARNING.*skipped.*)?"
+set output_string "(\\w+ = \\d+\r\n){16}${all_pass_string}(WARNING.*skipped.*)?"
 
 stap_run $srcdir/$subdir/$test.stp sleep_ten_secs $output_string
diff --git a/testsuite/systemtap.base/timers.stp b/testsuite/systemtap.base/timers.stp
index 6f4ce9d..92009a6 100755
--- a/testsuite/systemtap.base/timers.stp
+++ b/testsuite/systemtap.base/timers.stp
@@ -5,8 +5,9 @@
  */
 
 global p
-global j1, j2, jmax
-global ms1, ms500, msmax
+global j1, j2, jmax, jr
+global ms1, ms500, msmax, msr
+global sec, secr, us, usr, ns, nsr, hz
 
 probe begin
 {
@@ -18,12 +19,25 @@ probe timer.profile { ++p }
 probe timer.jiffies(1) { ++j1 }
 probe timer.jiffies(2) { ++j2 }
 probe timer.jiffies(1000000) { ++jmax }
+probe timer.jiffies(1000).randomize(500) { ++jr }
 
 /* as long as HZ>2, ms(1) and ms(500) 
  * will produce different counts */
 probe timer.ms(1) { ++ms1 }
 probe timer.ms(500) { ++ms500 }
 probe timer.ms(1000000) { ++msmax }
+probe timer.ms(1000).randomize(500) { ++msr }
+
+probe timer.sec(5) { ++sec }
+probe timer.sec(5).randomize(4) { ++secr }
+
+probe timer.us(1000) { ++us }
+probe timer.us(1000).randomize(500) { ++usr }
+
+probe timer.ns(1000000) { ++ns }
+probe timer.ns(1000000).randomize(500000) { ++nsr }
+
+probe timer.hz(1000) { ++hz }
 
 probe end
 {
@@ -32,10 +46,20 @@ probe end
 	printf("p = %d\n", p)
 	printf("j1 = %d\n", j1)
 	printf("j2 = %d\n", j2)
+	printf("jr = %d\n", jr)
 	printf("jmax = %d\n", jmax)
 	printf("ms1 = %d\n", ms1)
 	printf("ms500 = %d\n", ms500)
 	printf("msmax = %d\n", msmax)
+	printf("msr = %d\n", msr)
+
+	printf("sec = %d\n", sec)
+	printf("secr = %d\n", secr)
+	printf("us = %d\n", us)
+	printf("usr = %d\n", usr)
+	printf("ns = %d\n", nsr)
+	printf("nsr = %d\n", nsr)
+	printf("hz = %d\n", hz)
 
 	/* profile counter should be non-zero, and at
 	 * least as many as the jiffies(1) counter */
@@ -69,6 +93,32 @@ probe end
 		printf("systemtap test failure\n");
 	}
 
+	if (jr * msr * secr * usr * nsr <= 0) {
+		ret = 1
+		printf("unexpected randomize count\n")
+		printf("systemtap test failure\n");
+	}
+
+	if (sec <= 0) {
+		printf("unexpected sec count\n")
+		printf("systemtap test failure\n");
+	}
+
+	if (us <= 0) {
+		printf("unexpected us count\n")
+		printf("systemtap test failure\n");
+	}
+
+	if (ns <= 0) {
+		printf("unexpected ns count\n")
+		printf("systemtap test failure\n");
+	}
+
+	if (hz <= 0) {
+		printf("unexpected hz count\n")
+		printf("systemtap test failure\n");
+	}
+
 	if ( ret == 0 )
 		printf("systemtap test success\n")
 }
-- 
1.8.3.1

[PATCH 2/3] Fix the testcases so that the result will be more exact

[Zhou Wenjian]

fix by adding 'all_pass_string':
	* testsuite/systemtap.base/be_order.exp
	* testsuite/systemtap.base/global_init.exp
	* testsuite/systemtap.base/logical_and.exp
	* testsuite/systemtap.base/tri.exp
	* testsuite/systemtap.stress/current.exp

fix by adjusting the way of printing all_pass_string:
	* testsuite/systemtap.base/global_vars.stp
	* testsuite/systemtap.base/timers.stp
---
 testsuite/systemtap.base/be_order.exp    |  4 ++--
 testsuite/systemtap.base/global_init.exp |  4 ++--
 testsuite/systemtap.base/global_vars.stp | 40 ++++++++++++++++++--------------
 testsuite/systemtap.base/logical_and.exp |  4 ++--
 testsuite/systemtap.base/timers.stp      | 24 +++++++++----------
 testsuite/systemtap.base/tri.exp         |  4 ++--
 testsuite/systemtap.stress/current.exp   |  2 +-
 7 files changed, 43 insertions(+), 39 deletions(-)

diff --git a/testsuite/systemtap.base/be_order.exp b/testsuite/systemtap.base/be_order.exp
index eeae818..9398e50 100755
--- a/testsuite/systemtap.base/be_order.exp
+++ b/testsuite/systemtap.base/be_order.exp
@@ -3,9 +3,9 @@
 set test "be_order"
 foreach runtime [get_runtime_list] {
     if {$runtime != ""} {
-	stap_run $srcdir/$subdir/$test.stp no_load $all_pass_string \
+	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string} \
 	    --runtime=$runtime
     } else {
-	stap_run $srcdir/$subdir/$test.stp no_load $all_pass_string
+	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}
     }
 }
diff --git a/testsuite/systemtap.base/global_init.exp b/testsuite/systemtap.base/global_init.exp
index 040ac44..d1dabf6 100755
--- a/testsuite/systemtap.base/global_init.exp
+++ b/testsuite/systemtap.base/global_init.exp
@@ -4,9 +4,9 @@ set test "global_init"
 
 foreach runtime [get_runtime_list] {
     if {$runtime != ""} {
-	stap_run $srcdir/$subdir/$test.stp no_load $all_pass_string \
+	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string} \
 	    --runtime=$runtime
     } else {
-	stap_run $srcdir/$subdir/$test.stp no_load $all_pass_string
+	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}
     }
 }
diff --git a/testsuite/systemtap.base/global_vars.stp b/testsuite/systemtap.base/global_vars.stp
index 737e255..952f3c5 100755
--- a/testsuite/systemtap.base/global_vars.stp
+++ b/testsuite/systemtap.base/global_vars.stp
@@ -24,34 +24,38 @@ probe begin(-9223372036854775808) {
 }
 
 probe end {
-  if (c == 42)
-    println("systemtap test success")
-  else
+  ret = 0
+  if (c != 42) {
+    ret = 1;
     printf("systemtap test failure - c:%d != 42\n", c)
+  }
 
-  if (d == (c + g))
-    println("systemtap test success")
-  else
+  if (d != (c + g)) {
+    ret = 1;
     printf("systemtap test failure - d:%d != %d\n", d, (c+g))
+  }
 
-  if (a == d)
-    println("systemtap test success")
-  else
+  if (a != d) {
+    ret = 1;
     printf("systemtap test failure - a:%d != %d\n", a, d)
+  }
 
-  if (f == c)
-    println("systemtap test success")
-  else
+  if (f != c) {
+    ret = 1;
     printf("systemtap test failure - f:%d != %d\n", f, c)
+  }
 
-  if (e[0] == "a")
-    println("systemtap test success")
-  else
+  if (e[0] != "a") {
+    ret = 1;
     printf("systemtap test failure - e:%s != a\n", e[0])
+  }
 
-  if (gstr_saved == "foobar")
-    println("systemtap test success")
-  else
+  if (gstr_saved != "foobar") {
+    ret = 1;
     printf("systemtap test failure - gstr_saved:%s != foobar\n", gstr_saved)
+  }
+
+  if (ret == 0)
+    println("systemtap test success")
 }
 
diff --git a/testsuite/systemtap.base/logical_and.exp b/testsuite/systemtap.base/logical_and.exp
index 0e52e68..e5a586f 100755
--- a/testsuite/systemtap.base/logical_and.exp
+++ b/testsuite/systemtap.base/logical_and.exp
@@ -4,9 +4,9 @@ set test "logical_and"
 
 foreach runtime [get_runtime_list] {
     if {$runtime != ""} {
-	stap_run $srcdir/$subdir/$test.stp no_load $all_pass_string \
+	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string} \
 	    --runtime=$runtime
     } else {
-	stap_run $srcdir/$subdir/$test.stp no_load $all_pass_string
+	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}
     }
 }
diff --git a/testsuite/systemtap.base/timers.stp b/testsuite/systemtap.base/timers.stp
index fb590ed..6f4ce9d 100755
--- a/testsuite/systemtap.base/timers.stp
+++ b/testsuite/systemtap.base/timers.stp
@@ -27,6 +27,7 @@ probe timer.ms(1000000) { ++msmax }
 
 probe end
 {
+	ret = 0;
 	printf("systemtap ending probe\n")
 	printf("p = %d\n", p)
 	printf("j1 = %d\n", j1)
@@ -39,36 +40,35 @@ probe end
 	/* profile counter should be non-zero, and at
 	 * least as many as the jiffies(1) counter */
 	/* (fudge comparison by ~1% since start & end aren't synchronized) */
-	if ((p > 0) && (p*100 >= j1*99)) {
-		printf("systemtap test success\n")
-	} else {
+	if ( !((p > 0) && (p*100 >= j1*99)) ) {
+		ret = 1
 		printf("unexpected profile count: %d profiles and %d jiffies\n", p, j1)
 		printf("systemtap test failure\n");
 	}
 
 	/* jiffies(1) should count more than jiffies(2),
 	 * and both should be non-zero. */
-	if ((j2 > 0) && (j2 < j1)) {
-		printf("systemtap test success\n")
-	} else {
+	if ( !((j2 > 0) && (j2 < j1)) ) {
+		ret = 1
 		printf("unexpected jiffies count\n")
 		printf("systemtap test failure\n");
 	}
 
 	/* ms(1) should count more than ms(500),
 	 * and both should be non-zero. */
-	if ((ms500 > 0) && (ms500 < ms1)) {
-		printf("systemtap test success\n")
-	} else {
+	if ( !((ms500 > 0) && (ms500 < ms1)) ) {
+		ret = 1
 		printf("unexpected ms count\n")
 		printf("systemtap test failure\n");
 	}
 
 	/* both 'infinite' intervals should register zero counts */
-	if ((jmax == 0) && (msmax == 0)) {
-		printf("systemtap test success\n")
-	} else {
+	if ( !((jmax == 0) && (msmax == 0)) ) {
+		ret = 1
 		printf("unexpected count on 'infinite' interval\n")
 		printf("systemtap test failure\n");
 	}
+
+	if ( ret == 0 )
+		printf("systemtap test success\n")
 }
diff --git a/testsuite/systemtap.base/tri.exp b/testsuite/systemtap.base/tri.exp
index c980284..34c7e09 100755
--- a/testsuite/systemtap.base/tri.exp
+++ b/testsuite/systemtap.base/tri.exp
@@ -4,9 +4,9 @@ set test "tri"
 
 foreach runtime [get_runtime_list] {
     if {$runtime != ""} {
-	stap_run $srcdir/$subdir/$test.stp no_load $all_pass_string \
+	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string} \
 	    --runtime=$runtime
     } else {
-	stap_run $srcdir/$subdir/$test.stp no_load $all_pass_string
+	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}
     }
 }
diff --git a/testsuite/systemtap.stress/current.exp b/testsuite/systemtap.stress/current.exp
index 313b67a..b7a3d78 100755
--- a/testsuite/systemtap.stress/current.exp
+++ b/testsuite/systemtap.stress/current.exp
@@ -19,6 +19,6 @@ proc current_load {} {
     return 0
 }
 
-set output_string "(\\w+ = \\d+\r\n){5}${all_pass_string}(WARNING.*skipped.*)?"
+set output_string "(\\w+ = \\d+\r\n){5}${all_pass_string}${all_pass_string}(WARNING.*skipped.*)?"
 
 stap_run $srcdir/$subdir/$test.stp current_load $output_string -g -w
-- 
1.8.3.1

Re: [PATCH 1/3] add testcases for function definitions

[Josh Stone]

On 11/09/2015 12:57 AM, Zhou Wenjian wrote:
> +foreach runtime [get_runtime_list] {
> +    if {$runtime != ""} {
> +	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string} \
> +	    --runtime=$runtime
> +    } else {
> +	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}
> +    }
> +}

I disagree with using repetition like this for "exact" results.  The
string already has regex repetition built in:

  set all_pass_string "(systemtap test success\r\n)+"

'+' means match one or more, greedily.  Repeating this expression on top
of itself creates a bad case for the regex engine to backtrack.
(It will work, but slowly.)


IMO we ought to make stap_run ensure nothing comes *after* the expected
output string.  If there are tests that are legitimately printing more
output, those are the ones we should be fixing.

Maybe we could also add the string without repetition, something like:

  set pass_string "systemtap test success\r\n"
  set all_pass_string "($pass_string)+"

Then you can use "($pass_string){5}" if you really want exactly 5
matches.  But stap_run should still make sure nothing comes after that.

Re: [PATCH 1/3] add testcases for function definitions

[David Smith]

On 11/09/2015 12:07 PM, Josh Stone wrote:
> On 11/09/2015 12:57 AM, Zhou Wenjian wrote:
>> +foreach runtime [get_runtime_list] {
>> +    if {$runtime != ""} {
>> +	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string} \
>> +	    --runtime=$runtime
>> +    } else {
>> +	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}
>> +    }
>> +}
> 
> I disagree with using repetition like this for "exact" results.  The
> string already has regex repetition built in:
> 
>   set all_pass_string "(systemtap test success\r\n)+"

Ah, I didn't know it did that.

> '+' means match one or more, greedily.  Repeating this expression on top
> of itself creates a bad case for the regex engine to backtrack.
> (It will work, but slowly.)

Hmm, having repetitions of the all_pass_string *should* work, but
doesn't. My guess would be that this used to work on older versions of
tcl/expect, but doesn't work now. The end-of-line handling has always
been iffy in tcl/expect.

> IMO we ought to make stap_run ensure nothing comes *after* the expected
> output string.  If there are tests that are legitimately printing more
> output, those are the ones we should be fixing.

That's a good idea. I'll try to look into it in the near future.

> Maybe we could also add the string without repetition, something like:
> 
>   set pass_string "systemtap test success\r\n"
>   set all_pass_string "($pass_string)+"
> 
> Then you can use "($pass_string){5}" if you really want exactly 5
> matches.  But stap_run should still make sure nothing comes after that.
> 
> 


-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Re: [PATCH 1/3] add testcases for function definitions

[Josh Stone]

On 11/09/2015 01:24 PM, David Smith wrote:
> On 11/09/2015 12:07 PM, Josh Stone wrote:
>> On 11/09/2015 12:57 AM, Zhou Wenjian wrote:
>>> +foreach runtime [get_runtime_list] {
>>> +    if {$runtime != ""} {
>>> +	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string} \
>>> +	    --runtime=$runtime
>>> +    } else {
>>> +	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}
>>> +    }
>>> +}
>>
>> I disagree with using repetition like this for "exact" results.  The
>> string already has regex repetition built in:
>>
>>   set all_pass_string "(systemtap test success\r\n)+"
> 
> Ah, I didn't know it did that.
> 
>> '+' means match one or more, greedily.  Repeating this expression on top
>> of itself creates a bad case for the regex engine to backtrack.
>> (It will work, but slowly.)
> 
> Hmm, having repetitions of the all_pass_string *should* work, but
> doesn't. My guess would be that this used to work on older versions of
> tcl/expect, but doesn't work now. The end-of-line handling has always
> been iffy in tcl/expect.

It depends on how things get buffered, and I don't think you can rely on
this to be consistent.

If two success lines get presented at once, then '+' will gobble them
both.  If they're presented one at a time, then '+' will eat one and be
satisfied, then the second line is left unconsumed.

On the other hand, if "(systemtap test success\r\n){2}" gets them
buffered separately, the first time will try and fail to get a complete
match.  Then IIRC expect will append the second line to the buffer and
try all matches again, and "{2}" will now match.

It might be better if we figured out how to process this line-by-line,
instead of a big multiline regex.  We already had to bump stap_run's
exp_match_max to 8192 to cope with this kind of problem.  But any large
change here might have a wide effect across the testsuite callers...


A simpler (untested) idea: stap_run could just add some final output to
anchor the end of the regex, like -E 'probe final { println("EOF") }'.
This can anchor the end of the regex, like "systemtap ending probe" is
anchoring the beginning.  Then little else has to change in the tests.
The '+' above has to be followed by literal EOF or it's not a match.

("probe final" doesn't exist yet, but think opposite of "probe init",
alias "probe final = end(INT64_MAX)", perhaps covering error too.)
((bikeshed: perhaps init/fini are a better pair.))

Re: [PATCH 1/3] add testcases for function definitions

["Zhou, Wenjian/周文剑"]

On 11/10/2015 02:07 AM, Josh Stone wrote:
> On 11/09/2015 12:57 AM, Zhou Wenjian wrote:
>> +foreach runtime [get_runtime_list] {
>> +    if {$runtime != ""} {
>> +	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string} \
>> +	    --runtime=$runtime
>> +    } else {
>> +	stap_run $srcdir/$subdir/$test.stp no_load ${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}${all_pass_string}
>> +    }
>> +}
>
> I disagree with using repetition like this for "exact" results.  The
> string already has regex repetition built in:
>
>    set all_pass_string "(systemtap test success\r\n)+"
>
> '+' means match one or more, greedily.  Repeating this expression on top
> of itself creates a bad case for the regex engine to backtrack.
> (It will work, but slowly.)
>
>
> IMO we ought to make stap_run ensure nothing comes *after* the expected
> output string.  If there are tests that are legitimately printing more
> output, those are the ones we should be fixing.
>
> Maybe we could also add the string without repetition, something like:
>
>    set pass_string "systemtap test success\r\n"
>    set all_pass_string "($pass_string)+"
>
> Then you can use "($pass_string){5}" if you really want exactly 5
> matches.  But stap_run should still make sure nothing comes after that.
>
>

I think either of them is enough to generate the correct result.
Why should stap_run still make sure nothing comes after matching
the exact output?

And between them, I prefer matching the exact output.

To make sure nothing comes, we have to modify all cases which use the
stap_run. I don't think it's a good idea that modifying the cases which
are working well.

-- 
Thanks
Zhou

Re: [PATCH 1/3] add testcases for function definitions

[Josh Stone]

On 11/09/2015 06:10 PM, "Zhou, Wenjian/周文剑" wrote:
> I think either of them is enough to generate the correct result.
> Why should stap_run still make sure nothing comes after matching
> the exact output?

Because people make mistakes.  Perhaps the test.exp looks for 5 success
lines, but the test.stp outputs 6 lines - this should be flagged.  The
extra line might have been added later, forgetting to update test.exp
too.  And if the extra line of output happens to report a failure, we
don't want to miss that.

Checking that nothing comes after is a way to be sure that we really are
matching exact output.

> And between them, I prefer matching the exact output.

You mean between "+" and "{5}"?  Explicit counts are fine with me, but I
don't like manually repeating the match string.

> To make sure nothing comes, we have to modify all cases which use the
> stap_run. I don't think it's a good idea that modifying the cases which
> are working well.

If my probe-final-"EOF" idea works, then we can implement that entirely
in stap_run, without modifying any testcases.

Re: [PATCH 1/3] add testcases for function definitions

["Zhou, Wenjian/周文剑"]

On 11/10/2015 10:31 AM, Josh Stone wrote:
> On 11/09/2015 06:10 PM, "Zhou, Wenjian/周文剑" wrote:
>> I think either of them is enough to generate the correct result.
>> Why should stap_run still make sure nothing comes after matching
>> the exact output?
>
> Because people make mistakes.  Perhaps the test.exp looks for 5 success
> lines, but the test.stp outputs 6 lines - this should be flagged.  The
> extra line might have been added later, forgetting to update test.exp
> too.  And if the extra line of output happens to report a failure, we
> don't want to miss that.
>
> Checking that nothing comes after is a way to be sure that we really are
> matching exact output.
>

I don't think it is necessary to concern about case authors' mistakes
in the test suite.

>> And between them, I prefer matching the exact output.
>
> You mean between "+" and "{5}"?  Explicit counts are fine with me, but I
> don't like manually repeating the match string.
>

Yes, I just mean the "+" and "{5}".

>> To make sure nothing comes, we have to modify all cases which use the
>> stap_run. I don't think it's a good idea that modifying the cases which
>> are working well.
>
> If my probe-final-"EOF" idea works, then we can implement that entirely
> in stap_run, without modifying any testcases.
>

Eh, if it works, I think the "{5}" won't be needed.
But I doubt whether it will introduce errors to some cases.
I will think more about it.

-- 
Thanks
Zhou

Re: [PATCH 1/3] add testcases for function definitions

["Zhou, Wenjian/周文剑"]

On 11/10/2015 10:50 AM, "Zhou, Wenjian/周文剑" wrote:
> On 11/10/2015 10:31 AM, Josh Stone wrote:
>> On 11/09/2015 06:10 PM, "Zhou, Wenjian/周文剑" wrote:
>>> I think either of them is enough to generate the correct result.
>>> Why should stap_run still make sure nothing comes after matching
>>> the exact output?
>>
>> Because people make mistakes.  Perhaps the test.exp looks for 5 success
>> lines, but the test.stp outputs 6 lines - this should be flagged.  The
>> extra line might have been added later, forgetting to update test.exp
>> too.  And if the extra line of output happens to report a failure, we
>> don't want to miss that.
>>
>> Checking that nothing comes after is a way to be sure that we really are
>> matching exact output.
>>
>
> I don't think it is necessary to concern about case authors' mistakes
> in the test suite.
>
>>> And between them, I prefer matching the exact output.
>>
>> You mean between "+" and "{5}"?  Explicit counts are fine with me, but I
>> don't like manually repeating the match string.
>>
>
> Yes, I just mean the "+" and "{5}".
>
>>> To make sure nothing comes, we have to modify all cases which use the
>>> stap_run. I don't think it's a good idea that modifying the cases which
>>> are working well.
>>
>> If my probe-final-"EOF" idea works, then we can implement that entirely
>> in stap_run, without modifying any testcases.
>>
>
> Eh, if it works, I think the "{5}" won't be needed.
> But I doubt whether it will introduce errors to some cases.
> I will think more about it.
>

It works, but also has some side effects.
It is better not to affect the cases which are working well, I think.

-- 
Thanks
Zhou

Re: [PATCH 1/3] add testcases for function definitions

[Josh Stone]

On 11/09/2015 11:06 PM, "Zhou, Wenjian/周文剑" wrote:
> On 11/10/2015 10:50 AM, "Zhou, Wenjian/周文剑" wrote:
>> On 11/10/2015 10:31 AM, Josh Stone wrote:
>>> On 11/09/2015 06:10 PM, "Zhou, Wenjian/周文剑" wrote:
>>>> To make sure nothing comes, we have to modify all cases which use the
>>>> stap_run. I don't think it's a good idea that modifying the cases which
>>>> are working well.
>>>
>>> If my probe-final-"EOF" idea works, then we can implement that entirely
>>> in stap_run, without modifying any testcases.
>>>
>>
>> Eh, if it works, I think the "{5}" won't be needed.
>> But I doubt whether it will introduce errors to some cases.
>> I will think more about it.
>>
> 
> It works, but also has some side effects.
> It is better not to affect the cases which are working well, I think.

What are those side effects?

Re: [PATCH 1/3] add testcases for function definitions

["Zhou, Wenjian/周文剑"]

On 11/11/2015 01:34 AM, Josh Stone wrote:
> On 11/09/2015 11:06 PM, "Zhou, Wenjian/周文剑" wrote:
>> On 11/10/2015 10:50 AM, "Zhou, Wenjian/周文剑" wrote:
>>> On 11/10/2015 10:31 AM, Josh Stone wrote:
>>>> On 11/09/2015 06:10 PM, "Zhou, Wenjian/周文剑" wrote:
>>>>> To make sure nothing comes, we have to modify all cases which use the
>>>>> stap_run. I don't think it's a good idea that modifying the cases which
>>>>> are working well.
>>>>
>>>> If my probe-final-"EOF" idea works, then we can implement that entirely
>>>> in stap_run, without modifying any testcases.
>>>>
>>>
>>> Eh, if it works, I think the "{5}" won't be needed.
>>> But I doubt whether it will introduce errors to some cases.
>>> I will think more about it.
>>>
>>
>> It works, but also has some side effects.
>> It is better not to affect the cases which are working well, I think.
>
> What are those side effects?
>

It will lead to some cases error.
For example:
In case "div0", the probe will be exit with error.
So the expected "EOF" will never be got.

Another example:
In case "proc_by_pid", if "EOF" is added, another "\r\n" will be needed
in the output_string.

And there are a lot of such problems.
I don't think probe-final-"EOF" is needed, for it won't help much, though
each error it introduces can be fixed easily.

-- 
Thanks
Zhou

Re: [PATCH 1/3] add testcases for function definitions

[Frank Ch. Eigler]

=?UTF-8?B?Ilpob3UsIFdlbmppYW4v5ZGo5paH5YmRIg==?= <zhouwj-fnst@cn.fujitsu.com> writes:

> [...]
> In case "div0", the probe will be exit with error.
> So the expected "EOF" will never be got.

(A "probe error { }" catches even those.)


> [...]  And there are a lot of such problems.  I don't think
> probe-final-"EOF" is needed, for it won't help much, though each
> error it introduces can be fixed easily.

Yeah.

I think the more basic problem is the general pattern of these test
cases, where multiple identical messages are printed to report subtest
status.  In a more ideal world, instead of:

systemtap starting probe
systemtap ending probe
systemtap test success
systemtap test success
systemtap test success
systemtap test success

those .stp scripts that can self-diagnose should say simply:

success
or
failure (detail)

those .stp scripts that cannot self-diagnose should say simply:

result1 FOO
result2 BAR
result3 ZOO

No "systemtap" / "starting" / "ending" boilerplate is needed; nor
repeated lines whose counting is critical.


- FChE

Re: [PATCH 1/3] add testcases for function definitions

[David Smith]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

On 11/11/2015 08:03 AM, Frank Ch. Eigler wrote:
> I think the more basic problem is the general pattern of these test
> cases, where multiple identical messages are printed to report subtest
> status.  In a more ideal world, instead of:
> 
> systemtap starting probe
> systemtap ending probe
> systemtap test success
> systemtap test success
> systemtap test success
> systemtap test success
> 
> those .stp scripts that can self-diagnose should say simply:
> 
> success
> or
> failure (detail)
> 
> those .stp scripts that cannot self-diagnose should say simply:
> 
> result1 FOO
> result2 BAR
> result3 ZOO
> 
> No "systemtap" / "starting" / "ending" boilerplate is needed; nor
> repeated lines whose counting is critical.

stap_run.exp is probably too complicated. I've attached a small patch to
it that tries to catch any "extra" output. Note that I haven't tried
running the entire testsuite with this patch to see if this breaks anything.

The starting/ending boilerplate doesn't really bother me, but I think we
should modify all the scripts to only print one success/fail line (which
Zhou Wenjian suggested several messages back). That is going to be
simple to test for, but a little more complicated in the test script
itself (but not too bad).

We should also switch the "all_pass_string" to only be one copy of the
"success" string.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

[-- Attachment #2: stap_run.patch --]
[-- Type: text/x-patch, Size: 664 bytes --]

diff --git a/testsuite/lib/stap_run.exp b/testsuite/lib/stap_run.exp
index dab8e06..dc03d65 100644
--- a/testsuite/lib/stap_run.exp
+++ b/testsuite/lib/stap_run.exp
@@ -90,8 +90,14 @@ proc stap_run { TEST_NAME {LOAD_GEN_FUNCTION ""} {OUTPUT_CHECK_STRING ""} args }
 			-re $warning_regexp {
 			    set probe_errors $expect_out(1,string)
 			    set skipped_probes $expect_out(2,string)}
+			default {
+			    fail "$TEST_NAME unexpected output (after passing output)"
+			}
 		    }
 		}
+		default {
+		    fail "$TEST_NAME unexpected output"
+		}
 		timeout { 
                     fail "$TEST_NAME shutdown (timeout)"
                     kill -INT -[exp_pid]

Re: [PATCH 1/3] add testcases for function definitions

["Zhou, Wenjian/周文剑"]

On 11/12/2015 03:07 AM, David Smith wrote:
> On 11/11/2015 08:03 AM, Frank Ch. Eigler wrote:
>> I think the more basic problem is the general pattern of these test
>> cases, where multiple identical messages are printed to report subtest
>> status.  In a more ideal world, instead of:
>>
>> systemtap starting probe
>> systemtap ending probe
>> systemtap test success
>> systemtap test success
>> systemtap test success
>> systemtap test success
>>
>> those .stp scripts that can self-diagnose should say simply:
>>
>> success
>> or
>> failure (detail)
>>
>> those .stp scripts that cannot self-diagnose should say simply:
>>
>> result1 FOO
>> result2 BAR
>> result3 ZOO
>>
>> No "systemtap" / "starting" / "ending" boilerplate is needed; nor
>> repeated lines whose counting is critical.

Maybe we can implement it for the new cases.

>
> stap_run.exp is probably too complicated. I've attached a small patch to
> it that tries to catch any "extra" output. Note that I haven't tried
> running the entire testsuite with this patch to see if this breaks anything.
>

The patch will also cause 20+ more cases error(in systemtap.base).

> The starting/ending boilerplate doesn't really bother me, but I think we
> should modify all the scripts to only print one success/fail line (which
> Zhou Wenjian suggested several messages back). That is going to be
> simple to test for, but a little more complicated in the test script
> itself (but not too bad).
>

Why isn't the implementation in PATCH v2 a good choice?
It will do the least change to the testsuite and has the minimum risk of
affecting other cases. In other hand, printing several success lines doesn't
have any bad effects.

> We should also switch the "all_pass_string" to only be one copy of the
> "success" string.
>

-- 
Thanks
Zhou

[PATCH 1/3] add testcases for function definitions

[Zhou Wenjian]

	* testsuite/systemtap.base/func_definition.exp: New test case.
	* testsuite/systemtap.base/func_definition.stp: New test file.
---
 testsuite/systemtap.base/func_definition.exp | 13 ++++++
 testsuite/systemtap.base/func_definition.stp | 60 ++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)
 create mode 100755 testsuite/systemtap.base/func_definition.exp
 create mode 100755 testsuite/systemtap.base/func_definition.stp

diff --git a/testsuite/systemtap.base/func_definition.exp b/testsuite/systemtap.base/func_definition.exp
new file mode 100755
index 0000000..65abdb5
--- /dev/null
+++ b/testsuite/systemtap.base/func_definition.exp
@@ -0,0 +1,13 @@
+# Check function definitions
+
+set test "func_definition"
+if {![installtest_p]} { untested "$test"; return }
+
+foreach runtime [get_runtime_list] {
+    if {$runtime != ""} {
+	stap_run $srcdir/$subdir/$test.stp no_load (${all_pass_string}){5} \
+	    --runtime=$runtime
+    } else {
+	stap_run $srcdir/$subdir/$test.stp no_load (${all_pass_string}){5}
+    }
+}
diff --git a/testsuite/systemtap.base/func_definition.stp b/testsuite/systemtap.base/func_definition.stp
new file mode 100755
index 0000000..eaa8d94
--- /dev/null
+++ b/testsuite/systemtap.base/func_definition.stp
@@ -0,0 +1,60 @@
+/*
+ * func_definition.stp
+ *
+ * Check function definitions
+ */
+probe begin {
+    println("systemtap starting probe")
+}
+
+
+function f1(arg:long)
+{
+    if (arg == 2015)
+        println("systemtap test success")
+    else
+        printf("systemtap test failure - arg of f1:%d != 2015\n", arg)
+}
+
+function f2(arg)
+{
+    if (arg == 2015)
+        println("systemtap test success")
+    else
+        printf("systemtap test failure - arg of f2:%d != 2015\n", arg)
+}
+
+function f3:long()
+{
+    return 2015
+}
+
+function f4()
+{
+    return 2015
+}
+
+function f5()
+{
+    println("systemtap test success")
+}
+
+probe end {
+    println("systemtap ending probe")
+
+    f1(2015)
+
+    f2(2015)
+
+    if (f3() == 2015)
+        println("systemtap test success")
+    else
+        printf("systemtap test failure - return_value of f3:%d != 2015\n", f3())
+
+    if (f4() == 2015)
+        println("systemtap test success")
+    else
+        printf("systemtap test failure - return_value of f4:%d != 2015\n", f4())
+
+    f5()
+}
-- 
1.8.3.1

Re: [PATCH 1/3] add testcases for function definitions

["Zhou, Wenjian/周文剑"]

Hello David,

These three patches also want to be reviewed.

-- 
Thanks
Zhou

On 11/26/2015 04:42 PM, Zhou Wenjian wrote:
> 	* testsuite/systemtap.base/func_definition.exp: New test case.
> 	* testsuite/systemtap.base/func_definition.stp: New test file.
> ---
>   testsuite/systemtap.base/func_definition.exp | 13 ++++++
>   testsuite/systemtap.base/func_definition.stp | 60 ++++++++++++++++++++++++++++
>   2 files changed, 73 insertions(+)
>   create mode 100755 testsuite/systemtap.base/func_definition.exp
>   create mode 100755 testsuite/systemtap.base/func_definition.stp
>
> diff --git a/testsuite/systemtap.base/func_definition.exp b/testsuite/systemtap.base/func_definition.exp
> new file mode 100755
> index 0000000..65abdb5
> --- /dev/null
> +++ b/testsuite/systemtap.base/func_definition.exp
> @@ -0,0 +1,13 @@
> +# Check function definitions
> +
> +set test "func_definition"
> +if {![installtest_p]} { untested "$test"; return }
> +
> +foreach runtime [get_runtime_list] {
> +    if {$runtime != ""} {
> +	stap_run $srcdir/$subdir/$test.stp no_load (${all_pass_string}){5} \
> +	    --runtime=$runtime
> +    } else {
> +	stap_run $srcdir/$subdir/$test.stp no_load (${all_pass_string}){5}
> +    }
> +}
> diff --git a/testsuite/systemtap.base/func_definition.stp b/testsuite/systemtap.base/func_definition.stp
> new file mode 100755
> index 0000000..eaa8d94
> --- /dev/null
> +++ b/testsuite/systemtap.base/func_definition.stp
> @@ -0,0 +1,60 @@
> +/*
> + * func_definition.stp
> + *
> + * Check function definitions
> + */
> +probe begin {
> +    println("systemtap starting probe")
> +}
> +
> +
> +function f1(arg:long)
> +{
> +    if (arg == 2015)
> +        println("systemtap test success")
> +    else
> +        printf("systemtap test failure - arg of f1:%d != 2015\n", arg)
> +}
> +
> +function f2(arg)
> +{
> +    if (arg == 2015)
> +        println("systemtap test success")
> +    else
> +        printf("systemtap test failure - arg of f2:%d != 2015\n", arg)
> +}
> +
> +function f3:long()
> +{
> +    return 2015
> +}
> +
> +function f4()
> +{
> +    return 2015
> +}
> +
> +function f5()
> +{
> +    println("systemtap test success")
> +}
> +
> +probe end {
> +    println("systemtap ending probe")
> +
> +    f1(2015)
> +
> +    f2(2015)
> +
> +    if (f3() == 2015)
> +        println("systemtap test success")
> +    else
> +        printf("systemtap test failure - return_value of f3:%d != 2015\n", f3())
> +
> +    if (f4() == 2015)
> +        println("systemtap test success")
> +    else
> +        printf("systemtap test failure - return_value of f4:%d != 2015\n", f4())
> +
> +    f5()
> +}
>

Re: [PATCH 1/3] add testcases for function definitions

[David Smith]

On 11/30/2015 09:19 PM, "Zhou, Wenjian/周文剑" wrote:
> Hello David,
> 
> These three patches also want to be reviewed.

Sorry for the delay. I checked in patches 2 and 3 last week, but then
got busy this week. I struggled a bit with the first patch, the one that
tests functions. We don't really have a test case that test functions
directly, but we do have lots that test functions indirectly. But, it
seemed reasonable, so I checked it in as well.

Thanks again for the patches.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)