Cleanup Network Tests and Network Test Module by noursaidi · Pull Request #565 · faucetsdn/daq

noursaidi · 2020-07-29T23:48:53Z

PR to clean up the network tests and network test module. Summary of changes:

Remove depreciated code from current network module
Merged pull request Issue337 multicastnetworktest #505 (multicast network test)
Re-added network module into Travis CI test.
- the min_send test always passes and the info_broadcast always the same message for now. I'm not sure if it's possible to test fail/skip as any network traffic from the faux device triggers a pass.
Remove NTP test module and subset/ntp combined into existing network module
Remove mac_oui test module and subset/connection directory, combined into existing network module

…tnetworktest

grafnu · 2020-07-30T00:27:39Z

docker/modules/Dockerfile.faux1

 RUN $AG update && $AG install -y telnetd && $AG install xinetd nginx

-COPY subset/ntp/NTPClient NTPClient
+COPY subset/network/NTPClient NTPClient


this shouldn't still be in subset/ -- it should be moved to docker/include/network

there should be no dependency on the subset/ folder from faux

@grafnu I was planning to address the dependancy as a separate PR as it involved other test modules too. I'll update to pick up the network module changes here

grafnu · 2020-07-30T00:31:15Z

testing/test_aux.sh

 interfaces:
  faux-1:
-    opts: brute broadcast_client ntpv4
+    opts: brute broadcast_client ntpv4 arp


why is this enabled for everything? Whatever it's for, that's not the right way... If it's truly always required then it should be the default behavior, otherwise all three of these should not have it (what's the point?)

Ah, that would have been a much better, thanks. This is related to the min_send test which always passes, but might have a different summary depending if there are any ARP packets. As there seems to be some chance it might not, this periodically sends ARP packets so the result is always as expected.

codecov · 2020-07-30T04:37:19Z

Codecov Report

Merging #565 into master will decrease coverage by 0.50%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #565      +/-   ##
==========================================
- Coverage   80.78%   80.28%   -0.51%     
==========================================
  Files          22       22              
  Lines        3753     3753              
==========================================
- Hits         3032     3013      -19     
- Misses        721      740      +19

Flag	Coverage Δ
#aux	`73.79% <ø> (ø)`
#base	`75.25% <ø> (-0.06%)`	⬇️
#dhcp	`72.12% <ø> (-0.50%)`	⬇️
#many	`72.04% <ø> (ø)`
#modules	`23.58% <ø> (ø)`
#topo	`72.09% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
daq/gateway.py	`90.00% <0.00%> (-4.74%)`	⬇️
daq/runner.py	`87.05% <0.00%> (-1.69%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e38c4a2...445393d. Read the comment docs.

pisuke

All looks good to me, thanks!

grafnu · 2020-07-30T21:33:21Z

docker/include/bin/start_faux

    bin/bacnet_discover loop &
 fi

-if [ -n "${options[curl]}" ]; then


curl should still be optional since it's not default behavior for every case

noursaidi · 2020-07-30T22:17:03Z

Thanks @grafnu for reviewing again, I'd appreciate your input because I'm not sure what the best way to proceed is.

The issue I'm trying to resolve is a an unpredictable result summary in the Travis report output from the general network tests (communication_type and min_send).

From 7 test_aux runs I've done, 2 have failed . One was due to one faux device not sending ARP packets, the other not sending unicast packets. This respective changed the report summary by omitting a few words. I may be wrong, but I don't think this can be predicted.

Therefore I think 'arpsend' and 'curl' (or alternative to send a data packet) is needed as default behavior for test_aux only, but not for the faux devices generally, to ensure predictable behavior.

Alternatively, as these tests rely on reading monitor.pcap, For 2/3 of the faux devices I could add pre-prepared packet capture files and configure the test module to read that instead of the actual monitor scan. That way can at least have some test of skip/fail conditions. The test would still run normally on one of the faux devices. Not ideal, might be better than three tests with the same result.

Do you think there is a better way?

pisuke · 2020-08-03T10:31:40Z

@grafnu does @noursaidi 's suggestion sound good in the short term to get this PR merged?

noursaidi · 2020-08-03T10:48:28Z

@pisuke @grafnu I was thinking that instead I could add a configuration to enable/disable the sub tests from module_config, and disable the problematic general network tests in the Travis CI tests. This means the coverage of testing in Travis CI remains identical after this PR as it did before.

I do think it is important to have proper coverage of all test, and then this can be discussed and addressed properly in a separate issue/PR rather.

grafnu · 2020-08-03T14:18:09Z

For the short term, I'd make the arp send a default for faux. It's not "wrong" but having it always on in faux should make it the easiest to clean up. At the same time we should file a bug/issue to get it (arp being the default) cleaned up. Module configs are bad because then it's just making everything more messy to work around this one case. Doing pre-prepared capture packets correctly is likely harder than you think, so I wouldn't go down that route. (Basically you can't closely tie it in with the test itself, which means you need some mechanism to mock them out, which means more work than you expect.) The likely solution is to increase the monitoring time, but I'm not sure. That would just be my guess about a flaky connection. I don't understand why making 'curl' default should be necessary. But for now, can you just make arp the default and we'll go from there. And I don't mean "arp is enabled on everything" like you had before, but more that 'arp' is the default and there would be an explicit option (xarp) to NOT do it.

…

On Mon, Aug 3, 2020 at 3:48 AM Noureddine ***@***.***> wrote: @pisuke <https://github.com/pisuke> @grafnu <https://github.com/grafnu> I was thinking that instead I could add a configuration to enable/disable the sub tests from module_config, and disable the problematic general network tests in the Travis CI tests. This means the coverage of testing in Travis CI remains identical after this PR as it did before. I do think it is important to have proper coverage of all test, and then this can be discussed and addressed properly in a separate issue/PR rather. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#565 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIEPD4FJXRD5GPKG7RVC5TR62IYVANCNFSM4PMKLNSA> .

noursaidi · 2020-08-04T17:28:11Z

Really appreciate your thoughts and feedback @grafnu, thanks. I've restored curl and made xarp an option as as suggested.

Here is a run of 12 test_aux runs, passes 11. https://travis-ci.com/github/noursaidi/daq/builds/178423661.

'curl' default was overkill, only faux-3 needed something to periodically send data. The other 2 had NTP which satisfies this.

grafnu · 2020-08-04T17:33:23Z

docker/include/bin/start_faux

+if [ -n "${options[curl]}" ]; then
+    echo Starting curl loop.
+    (while true; do curl -o - http://google.com; sleep 1; done) &
+fi


nit: blank line after this block

grafnu · 2020-08-04T17:34:38Z

Great -- 11/12 passing is fine. If there's still flakiness then we can deal with it separately. PR approved with just a minor formatting comment.

…

On Tue, Aug 4, 2020 at 10:28 AM Noureddine ***@***.***> wrote: Really appreciate your thoughts and feedback @grafnu <https://github.com/grafnu>, thanks. I've restored curl and made xarp an option as as suggested. Here is a run of 12 test_aux runs, passes 11. https://travis-ci.com/github/noursaidi/daq/builds/178423661. 'curl' default was overkill, only faux-3 needed something to periodically send data. The other 2 had NTP which satisfies this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#565 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIEPDZZ3F3NR2OHSG6PLI3R7BALTANCNFSM4PMKLNSA> .

* removed depreciated code and moved ntp and macoui tests into network module

noursaidi added 22 commits July 28, 2020 17:39

removed depreciated code

d244f7b

move ntp tests into network module

bbc5afd

update docker containers

da656e4

fix docker containers

6913a2b

Merge communication broadcast test from em-redstone:issue337-multicas…

350036f

…tnetworktest

add network to ci test

3327915

Move macoui test into network module

676b1a1

remove connection/macoui module

3d9d954

delete macoui folder, saved changes before comitting ...

c1438e3

add network test to all.conf

b67d79e

add missing java to dockerfile

f882614

fix bug with mac_oui reporty

4cb3431

add travis ci output

eb1df80

fix travis ci 2

7e8d6c3

final travis ci changes

77fb78c

Stickler fixes

390d081

fix min-send flakiness

6937a85

fix minsend flakiness

d1cb952

tests broken

beec688

unbreak daq (syntax error ..)

aa7ea50

remove curl option from faux

bacdff7

travis fix

aaf0265

noursaidi requested review from grafnu and pisuke July 29, 2020 23:48

grafnu reviewed Jul 30, 2020

View reviewed changes

move faux dependancies, make arp option default

e75feca

noursaidi requested a review from grafnu July 30, 2020 09:07

noursaidi added 2 commits July 30, 2020 10:08

remove arp from faux (unsaved change)

fdfbb75

make curl option default

7168ae4

pisuke approved these changes Jul 30, 2020

View reviewed changes

grafnu reviewed Jul 30, 2020

View reviewed changes

pisuke mentioned this pull request Aug 3, 2020

Remove possibly deprecated test modules #559

Closed

pisuke mentioned this pull request Aug 3, 2020

Issue337 multicastnetworktest #505

Closed

resture curl option, make arpsend default

762cc4f

noursaidi requested a review from grafnu August 4, 2020 17:29

grafnu approved these changes Aug 4, 2020

View reviewed changes

grafnu self-assigned this Aug 4, 2020

fix formating

445393d

pisuke merged commit 4886fec into faucetsdn:master Aug 4, 2020

frgitdaq pushed a commit to frgitdaq/daq that referenced this pull request Aug 10, 2020

Cleanup Network Tests and Network Test Module (faucetsdn#565)

8757ec0

* removed depreciated code and moved ntp and macoui tests into network module

This was referenced Aug 12, 2020

Faux dependencies in subset folder #560

Closed

Add network tests to CI tests #516

Closed

Validate protocol.app_min_send #338

Closed

Comments

Conversation

noursaidi commented Jul 29, 2020

Uh oh!

grafnu Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

noursaidi Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

grafnu Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

noursaidi Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pisuke left a comment

Choose a reason for hiding this comment

Uh oh!

grafnu Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

noursaidi commented Jul 30, 2020

Uh oh!

pisuke commented Aug 3, 2020

Uh oh!

noursaidi commented Aug 3, 2020

Uh oh!

grafnu commented Aug 3, 2020 via email

Uh oh!

noursaidi commented Aug 4, 2020

Uh oh!

grafnu Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

grafnu commented Aug 4, 2020 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jul 30, 2020 •

edited

Loading