[LTP] [PATCH 1/1] ipneigh01: Replace TCONF error message with TINFO

[LTP] [PATCH 1/1] ipneigh01: Replace TCONF error message with TINFO

[Anton Smorodskyi]

When test case catch that arp command not support ipv6
it is more appropriate to send this as TINFO message

Signed-off-by: Anton Smorodskyi <asmorodskyi@suse.com>
---
 testcases/network/tcp_cmds/ipneigh/ipneigh01.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/network/tcp_cmds/ipneigh/ipneigh01.sh b/testcases/network/tcp_cmds/ipneigh/ipneigh01.sh
index 9af3aa31e..24e540dd0 100755
--- a/testcases/network/tcp_cmds/ipneigh/ipneigh01.sh
+++ b/testcases/network/tcp_cmds/ipneigh/ipneigh01.sh
@@ -76,7 +76,7 @@ rhost=$(tst_ipaddr rhost)
 if [ -z "$TST_IPV6" ]; then
 	do_test "arp -a" "arp -d $rhost"
 else
-	tst_resm TCONF "'arp cmd doesn't support IPv6, skipping test-case"
+	tst_resm TINFO "'arp cmd doesn't support IPv6, skipping test-case"
 fi
 
 do_test "ip neigh show" "ip neigh del $rhost dev $(tst_iface)"
-- 
2.13.6

[LTP] [PATCH 1/1] ipneigh01: Replace TCONF error message with TINFO

[Alexey Kodanev]

On 03.05.2018 14:36, Anton Smorodskyi wrote:
> When test case catch that arp command not support ipv6
> it is more appropriate to send this as TINFO message
Hi Anton,

And why it is more appropriate? TCONF is not an error message.

> @@ -76,7 +76,7 @@ rhost=$(tst_ipaddr rhost)
>  if [ -z "$TST_IPV6" ]; then
>  	do_test "arp -a" "arp -d $rhost"
>  else
> -	tst_resm TCONF "'arp cmd doesn't support IPv6, skipping test-case"
> +	tst_resm TINFO "'arp cmd doesn't support IPv6, skipping test-case"
>  fi


TCONF here, only to make sure that the test results are consistent with
'TST_TOTAL=2', regardless whether we use '-6' option or not.

[LTP] [PATCH 1/1] ipneigh01: Replace TCONF error message with TINFO

[Petr Vorel]

Hi Anton, Alexey,

> On 03.05.2018 14:36, Anton Smorodskyi wrote:
> > When test case catch that arp command not support ipv6
> > it is more appropriate to send this as TINFO message
> Hi Anton,

> And why it is more appropriate? TCONF is not an error message.

> > @@ -76,7 +76,7 @@ rhost=$(tst_ipaddr rhost)
> >  if [ -z "$TST_IPV6" ]; then
> >  	do_test "arp -a" "arp -d $rhost"
> >  else
> > -	tst_resm TCONF "'arp cmd doesn't support IPv6, skipping test-case"
> > +	tst_resm TINFO "'arp cmd doesn't support IPv6, skipping test-case"
> >  fi


> TCONF here, only to make sure that the test results are consistent with
> 'TST_TOTAL=2', regardless whether we use '-6' option or not.
Alexey, thanks for a clarification.
So TST_TOTAL could be set according to $TST_IPV6.


Kind regards,
Petr

[LTP] [PATCH 1/1] ipneigh01: Replace TCONF error message with TINFO

[Anton Smorodskyi]

Hi Alexey

This is my first attempt to contribute to LTP project. Disadvantage of 
this is the fact that I can ask some obvious for you question ( hope you 
will forgive me that) , but advantage that I have a fresh look so I can 
point out to something that is wrong but you just get used too.

First of all let's clarify something not related to this PR directly :


>> And why it is more appropriate? TCONF is not an error message.
In LTP documentation TCONF defined as - "The test case was not 
appropriate for the current hardware or software configuration". So I 
would not call it error too but I would say that it is some flavor of 
"SKIPPED" state.
Where you want to say "this test case can't run currently so I can't say 
if it is passed or failed"

Second thing which needs to be taken in consideration  I clarified with 
Petr Vorel in conversation outside this ML - test counter changed only 
on TPASS or TFAIL. This lead us to really confusing log output :

 1.
    ipneigh01 1 TINFO: initialize 'lhost' 'ltp_ns_veth2' interface
 2.
    ipneigh01 1 TINFO: set local addr 10.0.0.2/24
 3.
    ipneigh01 1 TINFO: set local addr fd00:1:1:1::2/64
 4.
    ipneigh01 1 TINFO: initialize 'rhost' 'ltp_ns_veth1' interface
 5.
    ipneigh01 1 TINFO: set remote addr 10.0.0.1/24
 6.
    ipneigh01 1 TINFO: set remote addr fd00:1:1:1::1/64
 7.
    ipneigh01 1 TINFO: Network config (local -- remote):
 8.
    ipneigh01 1 TINFO: ltp_ns_veth2 -- ltp_ns_veth1
 9.
    ipneigh01 1 TINFO: 10.0.0.2/24 -- 10.0.0.1/24
10.
    ipneigh01 1 TINFO: fd00:1:1:1::2/64 -- fd00:1:1:1::1/64
11.
    ipneigh01 1 TCONF: 'arp cmd doesn't support IPv6, skipping test-case
12.
    ipneigh01 1 TINFO: Stress auto-creation of NDISC cache entry
13.
    ipneigh01 1 TINFO: by pinging 'fd00:1:1:1::1' and deleting entry again
14.
    ipneigh01 1 TINFO: with 'ip neigh del fd00:1:1:1::1 dev ltp_ns_veth2'
15.
    ipneigh01 1 TPASS: verified adding/removing of NDISC cache entry

Please correct me if I missing something but I read this like "ok we 
can't run this test case because arp cmd doesn't support IPv6 , ah but 
wait test case is passed ".

After all discussions around that patch and all info which I gain I can 
agree that changing TCONF with TINFO was bad idea , but if we will just 
"fix" issue by changing TST_TOTAL value to 1 ( if I correctly understand 
your suggestion )
this will not remove this confusion.

Only valid fix which I see is change behavior of LTP to change test case 
counter after TCONF which makes total sense for me , but most probably I 
missing something and you had some reasons to not changing counter on 
TCONF messages. Can you please elaborate them ?

-- 
Anton Smorodskyi <asmorodskyi@suse.com>
QA Engineer
SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg
Tel: +49-911-74053-0; Fax: +49-911-7417755;  https://www.suse.com/
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard,
Graham Norton, HRB 21284 (AG Nürnberg)

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20180507/c51ccb69/attachment.html>

[LTP] [PATCH 1/1] ipneigh01: Replace TCONF error message with TINFO

[Petr Vorel]

Hi Anton, Alexey,

> Hi Alexey

> This is my first attempt to contribute to LTP project. Disadvantage of this
> is the fact that I can ask some obvious for you question ( hope you will
> forgive me that) , but advantage that I have a fresh look so I can point out
> to something that is wrong but you just get used too.

> First of all let's clarify something not related to this PR directly :


> > > And why it is more appropriate? TCONF is not an error message.
> In LTP documentation TCONF defined as - "The test case was not appropriate
> for the current hardware or software configuration". So I would not call it
> error too but I would say that it is some flavor of "SKIPPED" state.
> Where you want to say "this test case can't run currently so I can't say if
> it is passed or failed"
You can see it in in tst_resm() in testcases/lib/test.sh
    case "$ret" in
    TPASS|TFAIL)
    TST_COUNT=$((TST_COUNT+1));;
    esac

This is legacy API, new API behaves differently, see testcases/lib/tst_test.sh and doc:
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c
Legacy API is in maintenance mode (only bugfixes are allowed), some things aren't optimal
and not going to change).

> Second thing which needs to be taken in consideration  I clarified with Petr
> Vorel in conversation outside this ML - test counter changed only on TPASS
> or TFAIL. This lead us to really confusing log output :

> 1.
>    ipneigh01 1 TINFO: initialize 'lhost' 'ltp_ns_veth2' interface
> 2.
>    ipneigh01 1 TINFO: set local addr 10.0.0.2/24
> 3.
>    ipneigh01 1 TINFO: set local addr fd00:1:1:1::2/64
> 4.
>    ipneigh01 1 TINFO: initialize 'rhost' 'ltp_ns_veth1' interface
> 5.
>    ipneigh01 1 TINFO: set remote addr 10.0.0.1/24
> 6.
>    ipneigh01 1 TINFO: set remote addr fd00:1:1:1::1/64
> 7.
>    ipneigh01 1 TINFO: Network config (local -- remote):
> 8.
>    ipneigh01 1 TINFO: ltp_ns_veth2 -- ltp_ns_veth1
> 9.
>    ipneigh01 1 TINFO: 10.0.0.2/24 -- 10.0.0.1/24
> 10.
>    ipneigh01 1 TINFO: fd00:1:1:1::2/64 -- fd00:1:1:1::1/64
> 11.
>    ipneigh01 1 TCONF: 'arp cmd doesn't support IPv6, skipping test-case
> 12.
>    ipneigh01 1 TINFO: Stress auto-creation of NDISC cache entry
> 13.
>    ipneigh01 1 TINFO: by pinging 'fd00:1:1:1::1' and deleting entry again
> 14.
>    ipneigh01 1 TINFO: with 'ip neigh del fd00:1:1:1::1 dev ltp_ns_veth2'
> 15.
>    ipneigh01 1 TPASS: verified adding/removing of NDISC cache entry

> Please correct me if I missing something but I read this like "ok we can't
> run this test case because arp cmd doesn't support IPv6 , ah but wait test
> case is passed ".

> After all discussions around that patch and all info which I gain I can
> agree that changing TCONF with TINFO was bad idea , but if we will just
> "fix" issue by changing TST_TOTAL value to 1 ( if I correctly understand
> your suggestion )
> this will not remove this confusion.
There are 2 options:
1) a quick solution: for IPv6 change TST_TOTAL=1 and remove TCONF (skip it quietly)
2) add command parameter to test and call it 2x in runtest file for IPv4 (arp and ip) and
only once for (ip) for IPv6. See this commit, which does the same:
d1291fda8 network/interface: Split tests to test only one command per test
In this case might be better to migrate test into new shell API first (but not required).

> Only valid fix which I see is change behavior of LTP to change test case
> counter after TCONF which makes total sense for me , but most probably I
> missing something and you had some reasons to not changing counter on TCONF
> messages. Can you please elaborate them ?


Kind regards,
Petr

[LTP] [PATCH 1/1] ipneigh01: Replace TCONF error message with TINFO

[Alexey Kodanev]

On 05/07/2018 02:13 PM, Anton Smorodskyi wrote:
> Hi Alexey
> 
> This is my first attempt to contribute to LTP project. Disadvantage of this is the fact that I can ask some obvious for you question ( hope you will forgive me that) , but advantage that I have a fresh look so I can point out to something that is wrong but you just get used too.
> 
> First of all let's clarify something not related to this PR directly :
> 
> 

Hi Anton,

>>> And why it is more appropriate? TCONF is not an error message.
> In LTP documentation TCONF defined as - "The test case was not appropriate for the current hardware or software configuration". So I would not call it error too but I would say that it is some flavor of "SKIPPED" state.
> Where you want to say "this test case can't run currently so I can't say if it is passed or failed"

No, I don't.


> 
> Second thing which needs to be taken in consideration  I clarified with Petr Vorel in conversation outside this ML - test counter changed only on TPASS or TFAIL. This lead us to really confusing log output :
> 
>  1.
>     ipneigh01 1 TINFO: initialize 'lhost' 'ltp_ns_veth2' interface
>  2.
>     ipneigh01 1 TINFO: set local addr 10.0.0.2/24
>  3.
>     ipneigh01 1 TINFO: set local addr fd00:1:1:1::2/64
>  4.
>     ipneigh01 1 TINFO: initialize 'rhost' 'ltp_ns_veth1' interface
>  5.
>     ipneigh01 1 TINFO: set remote addr 10.0.0.1/24
>  6.
>     ipneigh01 1 TINFO: set remote addr fd00:1:1:1::1/64
>  7.
>     ipneigh01 1 TINFO: Network config (local -- remote):
>  8.
>     ipneigh01 1 TINFO: ltp_ns_veth2 -- ltp_ns_veth1
>  9.
>     ipneigh01 1 TINFO: 10.0.0.2/24 -- 10.0.0.1/24
> 10.
>     ipneigh01 1 TINFO: fd00:1:1:1::2/64 -- fd00:1:1:1::1/64
> 11.
>     ipneigh01 1 TCONF: 'arp cmd doesn't support IPv6, skipping test-case
> 12.
>     ipneigh01 1 TINFO: Stress auto-creation of NDISC cache entry
> 13.
>     ipneigh01 1 TINFO: by pinging 'fd00:1:1:1::1' and deleting entry again
> 14.
>     ipneigh01 1 TINFO: with 'ip neigh del fd00:1:1:1::1 dev ltp_ns_veth2'
> 15.
>     ipneigh01 1 TPASS: verified adding/removing of NDISC cache entry
> 
> Please correct me if I missing something but I read this like "ok we can't run this test case because arp cmd doesn't support IPv6 , ah but wait test case is passed ".
>> After all discussions around that patch and all info which I gain I can agree that changing TCONF with TINFO was bad idea , but if we will just "fix" issue by changing TST_TOTAL value to 1 ( if I correctly understand your suggestion )
> this will not remove this confusion.	

I've not suggested to change TST_TOTAL.

> 
> Only valid fix which I see is change behavior of LTP to change test case counter after TCONF which makes total sense for me , but most probably I missing something and you had some reasons to not changing counter on TCONF messages. Can you please elaborate them ?

Yes, this is a flaw in the test.sh library.

And it would be better to convert the test to use tst_test.sh.