| Message ID | 20180503113637.21573-1-asmorodskyi@suse.com |
|---|---|
| State | Rejected |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | [1/1] ipneigh01: Replace TCONF error message with TINFO | expand |
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.
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
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 ?
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
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.
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)"
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(-)