[PATCH 0/3] Test exit code fixlets

[PATCH 0/3] Test exit code fixlets

[Sam James]

A small set of exit code fixes to tests.

There are some other issues that are worth discussing, like pipelines
and ignoring the exit code there, but a lot of that stuff doesn't
matter *too* much given the test output is checked anyway.

Sam James (3):
  test: fix exit code in tst.ipv4remotetcp.sh
  test: fix exit code typo in tst.core.sh
  test: fix exit code in tst.newprobes.sh

 test/unittest/ip/tst.ipv4remotetcp.sh | 2 +-
 test/unittest/options/tst.core.sh     | 2 +-
 test/unittest/pid/tst.newprobes.sh    | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)


base-commit: c84954cdc6fb017034e2c3fe4445a7c8981d35e9
-- 
2.46.0

[PATCH 1/3] test: fix exit code in tst.ipv4remotetcp.sh

[Sam James]

$? was clobbered by the status=$? assignment above, see:
```
$ bash -c 'false ; x=$? ; echo $? ; echo ${x}'
0
1
```

We could just drop the status assignment but I just chose to do this
instead.
---
 test/unittest/ip/tst.ipv4remotetcp.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/unittest/ip/tst.ipv4remotetcp.sh b/test/unittest/ip/tst.ipv4remotetcp.sh
index 32c3435a..99ba1161 100755
--- a/test/unittest/ip/tst.ipv4remotetcp.sh
+++ b/test/unittest/ip/tst.ipv4remotetcp.sh
@@ -98,4 +98,4 @@ EODTRACE
 
 status=$?
 
-exit $?
+exit ${status}
-- 
2.46.0

Re: [PATCH 1/3] test: fix exit code in tst.ipv4remotetcp.sh

[Kris Van Hees]

On Tue, Aug 20, 2024 at 01:10:20AM +0100, Sam James wrote:
> $? was clobbered by the status=$? assignment above, see:
> ```
> $ bash -c 'false ; x=$? ; echo $? ; echo ${x}'
> 0
> 1
> ```
> 
> We could just drop the status assignment but I just chose to do this
> instead.

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

... and committed with Signed-off-by added.

> ---
>  test/unittest/ip/tst.ipv4remotetcp.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/test/unittest/ip/tst.ipv4remotetcp.sh b/test/unittest/ip/tst.ipv4remotetcp.sh
> index 32c3435a..99ba1161 100755
> --- a/test/unittest/ip/tst.ipv4remotetcp.sh
> +++ b/test/unittest/ip/tst.ipv4remotetcp.sh
> @@ -98,4 +98,4 @@ EODTRACE
>  
>  status=$?
>  
> -exit $?
> +exit ${status}
> -- 
> 2.46.0
> 

[PATCH 2/3] test: fix exit code typo in tst.core.sh

[Sam James]

$rcc is not set, but $rc is. Use that.
---
 test/unittest/options/tst.core.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/unittest/options/tst.core.sh b/test/unittest/options/tst.core.sh
index 48a58508..18727c6a 100755
--- a/test/unittest/options/tst.core.sh
+++ b/test/unittest/options/tst.core.sh
@@ -16,4 +16,4 @@ file core | tee /dev/stderr | \
 rc=$?
 rm -f core
 
-exit $rcc
+exit $rc
-- 
2.46.0

Re: [PATCH 2/3] test: fix exit code typo in tst.core.sh

[Kris Van Hees]

On Tue, Aug 20, 2024 at 01:10:21AM +0100, Sam James wrote:
> $rcc is not set, but $rc is. Use that.

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

... and committed with Signed-off-by added.

> ---
>  test/unittest/options/tst.core.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/test/unittest/options/tst.core.sh b/test/unittest/options/tst.core.sh
> index 48a58508..18727c6a 100755
> --- a/test/unittest/options/tst.core.sh
> +++ b/test/unittest/options/tst.core.sh
> @@ -16,4 +16,4 @@ file core | tee /dev/stderr | \
>  rc=$?
>  rm -f core
>  
> -exit $rcc
> +exit $rc
> -- 
> 2.46.0
> 

[PATCH 3/3] test: fix exit code in tst.newprobes.sh

[Sam James]

$status is never assigned, just use $?.
---
 test/unittest/pid/tst.newprobes.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/unittest/pid/tst.newprobes.sh b/test/unittest/pid/tst.newprobes.sh
index a4944688..0980d3b3 100755
--- a/test/unittest/pid/tst.newprobes.sh
+++ b/test/unittest/pid/tst.newprobes.sh
@@ -44,4 +44,4 @@ tick-1s
 }
 EOF
 
-exit $status
+exit $?
-- 
2.46.0

Re: [PATCH 3/3] test: fix exit code in tst.newprobes.sh

[Eugene Loh]

Another question about this test is whether "pid*:awk::" is even a 
legitimate probe description for pid probes;  do we allow wildcards for 
pid numbers in pid provider names?  Anyhow, that's a separate issue from 
the one raised in this patch;  it does, however, perhaps influence where 
we want to go with the test.

On 8/19/24 20:10, Sam James wrote:
> $status is never assigned, just use $?.
> ---
>   test/unittest/pid/tst.newprobes.sh | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/test/unittest/pid/tst.newprobes.sh b/test/unittest/pid/tst.newprobes.sh
> index a4944688..0980d3b3 100755
> --- a/test/unittest/pid/tst.newprobes.sh
> +++ b/test/unittest/pid/tst.newprobes.sh
> @@ -44,4 +44,4 @@ tick-1s
>   }
>   EOF
>   
> -exit $status
> +exit $?

Re: [PATCH 3/3] test: fix exit code in tst.newprobes.sh

[Kris Van Hees]

On Tue, Aug 20, 2024 at 01:10:22AM +0100, Sam James wrote:
> $status is never assigned, just use $?.

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

... and committed with Signed-off-by added.

> ---
>  test/unittest/pid/tst.newprobes.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/test/unittest/pid/tst.newprobes.sh b/test/unittest/pid/tst.newprobes.sh
> index a4944688..0980d3b3 100755
> --- a/test/unittest/pid/tst.newprobes.sh
> +++ b/test/unittest/pid/tst.newprobes.sh
> @@ -44,4 +44,4 @@ tick-1s
>  }
>  EOF
>  
> -exit $status
> +exit $?
> -- 
> 2.46.0
> 

Re: [PATCH 3/3] test: fix exit code in tst.newprobes.sh

[Eugene Loh]

Maybe I am a few patches behind.  I thought this test used pid*:awk:: 
and was therefore XFAIL.  I do not object to the patch, but I was only 
curious if we wanted a "real fix" to the test. There are other cases 
where tests have multiple problems and we do not generally seem to 
settle for fixing only one of the problems. Again, though, I'm okay with 
the patch... it's an improvement over what's there.

On 10/11/24 23:29, Kris Van Hees wrote:
> On Tue, Aug 20, 2024 at 01:10:22AM +0100, Sam James wrote:
>> $status is never assigned, just use $?.
> Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
>
> ... and committed with Signed-off-by added.
>
>> ---
>>   test/unittest/pid/tst.newprobes.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/test/unittest/pid/tst.newprobes.sh b/test/unittest/pid/tst.newprobes.sh
>> index a4944688..0980d3b3 100755
>> --- a/test/unittest/pid/tst.newprobes.sh
>> +++ b/test/unittest/pid/tst.newprobes.sh
>> @@ -44,4 +44,4 @@ tick-1s
>>   }
>>   EOF
>>   
>> -exit $status
>> +exit $?
>> -- 
>> 2.46.0
>>

Re: [PATCH 3/3] test: fix exit code in tst.newprobes.sh

[Kris Van Hees]

On Fri, Oct 11, 2024 at 11:55:33PM -0400, Eugene Loh wrote:
> Maybe I am a few patches behind.  I thought this test used pid*:awk:: and
> was therefore XFAIL.  I do not object to the patch, but I was only curious
> if we wanted a "real fix" to the test. There are other cases where tests
> have multiple problems and we do not generally seem to settle for fixing
> only one of the problems. Again, though, I'm okay with the patch... it's an
> improvement over what's there.

In general, yes, when we encounter a case like this we tend to fix other more
fundamental problems with the test.  In this case, since it is part of a larger
series of test clean up work, I took it as-is so as to not delay the cleanup
any longer.  But yes, the test will still need extra work to ensure it is
actually correct in terms of functionality.

> On 10/11/24 23:29, Kris Van Hees wrote:
> > On Tue, Aug 20, 2024 at 01:10:22AM +0100, Sam James wrote:
> > > $status is never assigned, just use $?.
> > Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
> > 
> > ... and committed with Signed-off-by added.
> > 
> > > ---
> > >   test/unittest/pid/tst.newprobes.sh | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/test/unittest/pid/tst.newprobes.sh b/test/unittest/pid/tst.newprobes.sh
> > > index a4944688..0980d3b3 100755
> > > --- a/test/unittest/pid/tst.newprobes.sh
> > > +++ b/test/unittest/pid/tst.newprobes.sh
> > > @@ -44,4 +44,4 @@ tick-1s
> > >   }
> > >   EOF
> > > -exit $status
> > > +exit $?
> > > -- 
> > > 2.46.0
> > >