Issue337 multicastnetworktest by em-redstone · Pull Request #505 · faucetsdn/daq

em-redstone · 2020-06-25T10:05:27Z

Hi @pisuke @noursaidi @grafnu , please take a look at these recent changes,

Changelog:

communication.type.broadcast now counts the number of multicast packets. It does this by checking if the addresses are within the range 224.0.0.0 to 239.255.255.255 (check high order bit on first address byte).
Removed the debug_generate_capture.py as it just runs a simple tcpdump command to create a sample .pcap file. There are no code references to the file.
Added some extra documentation.
Fixed some typos. Did some slight code cleanup.
Added network tests back into all.conf to run during test_aux.sh

Many thanks!

codecov · 2020-06-25T10:34:57Z

Codecov Report

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

@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
- Coverage   80.36%   79.83%   -0.53%     
==========================================
  Files          21       21              
  Lines        3621     3621              
==========================================
- Hits         2910     2891      -19     
- Misses        711      730      +19

Flag	Coverage Δ
#aux	`74.97% <ø> (-0.06%)`	⬇️
#base	`76.28% <ø> (-0.55%)`	⬇️
#dhcp	`71.56% <ø> (-0.32%)`	⬇️
#many	`71.50% <ø> (-0.55%)`	⬇️
#modules	`23.44% <ø> (ø)`
#topo	`73.27% <ø> (-0.15%)`	⬇️

Impacted Files	Coverage Δ
daq/gateway.py	`89.67% <0.00%> (-4.90%)`	⬇️
daq/runner.py	`86.55% <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 588189c...52ec747. Read the comment docs.

grafnu

See comments about test results, and then a few stickler comments.

grafnu · 2020-06-25T13:56:49Z

testing/test_aux.out

+RESULT skip security.passwords.http Port 80 is not open on target device.
+RESULT skip security.passwords.https Port 443 is not open on target device.
+RESULT skip security.passwords.telnet Port 23 is not open on target device.
+RESULT skip security.passwords.ssh Port 22 is not open on target device.


Why this change? What happened to testing the fail condition?

I'll address this issue with the next commits, the docker containers tend to be a bit flaky on my machine - sorry, I completely missed this!

grafnu · 2020-06-25T13:57:30Z

testing/test_aux.out

+RESULT fail connection.dhcp_long 
+RESULT fail connection.min_send 
+RESULT info communication.type.broadcast 
+RESULT fail protocol.app_min_send 


why are all these fail? Where's the tests for pass or skip?

Hi Trevor,

I've only tried to made code changes with regards to the communication.type.broadcast test - since there's been more frequent changes to the network module, I've added the network module back into test_aux.sh - but it's been removed for a long while so I'm not sure why the others are all failing or whether they were always like this, but I'll look into this.

These tests rely on reading monitor_scan.pcap which is currently turned off within by test_aux.sh which might explain the fail. The setting might need to be changed

@em-redstone to note that connection.dhcp_long will need to be removed from the module (will form part of a different test module). The same for app_min_send as it's not part of the test plan

Thanks @noursaidi, I can remove these two tests in the next commit if that's what you meant, or did you mean to just make note of it in the readme.md?

@em-redstone I was noting it in case you were going to try and get the aux tests to pass/fail for them. We can do a seperate issue/PR to remove them

I think we should keep them as is (exposed) and then fix them in another PR. Before they were just hidden which is the worst situation. Now we can at least see what is wrong! I'll send a note to the team to figure out how to address this.

grafnu · 2020-06-25T16:41:44Z

What we should do is separate out the problems into two categories... 1. Something new that was inadvertently introduced, and shouldn't happen (making the system worse). I think this would manifest itself as changes in the test result from pass <-> fail etc... 2. Something that was there before, but is now just being exposed. This would manifest itself as a new test result that, although wrong, wasn't there before. I don't want to shoot the messenger here. For this PR, you should fix things that are in category #1. We don't want things that were working before to break. But, you should NOT fix things in #2 -- it's ok to expose them (that's good), but I don't want to bog this PR down with fixing everything. We should then address that in another PR. What we need to do then, is accurately triage the cases into #1 or #2. It sounds like one set is category #1, while the other set is #2.

…

On Thu, Jun 25, 2020 at 9:26 AM Noureddine ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In testing/test_aux.out <#505 (comment)>: > RESULT pass security.passwords.http Default passwords have been changed. RESULT pass security.passwords.https Default passwords have been changed. RESULT pass security.passwords.telnet Default passwords have been changed. RESULT pass security.passwords.ssh Default passwords have been changed. RESULT skip security.firmware Could not retrieve a firmware version with nmap. Check bacnet port. RESULT pass security.firmware version found: ?\xFF\xFF\x19,>u\x08\x00no +RESULT fail connection.dhcp_long +RESULT fail connection.min_send +RESULT info communication.type.broadcast +RESULT fail protocol.app_min_send @em-redstone <https://github.com/em-redstone> I was noting it in case you were going to try and get the aux tests to pass/fail for them. We can do a seperate issue/PR to remove them — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#505 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIEPD7O6PXZUCDMFHP7YYLRYN3D3ANCNFSM4OIGVHPA> .

…_generate_capture, added readme, fixed some typos, added network module to run during testing/test_aux.sh

em-redstone · 2020-06-26T14:50:36Z

What we should do is separate out the problems into two categories... 1. Something new that was inadvertently introduced, and shouldn't happen (making the system worse). I think this would manifest itself as changes in the test result from pass <-> fail etc... 2. Something that was there before, but is now just being exposed. This would manifest itself as a new test result that, although wrong, wasn't there before. I don't want to shoot the messenger here. For this PR, you should fix things that are in category #1. We don't want things that were working before to break. But, you should NOT fix things in #2 -- it's ok to expose them (that's good), but I don't want to bog this PR down with fixing everything. We should then address that in another PR. What we need to do then, is accurately triage the cases into #1 or #2. It sounds like one set is category #1, while the other set is #2.

Hi @grafnu, @noursaidi,

I've just compared the network module results in test_aux.out in a run of the master vs. multicast branch and they have the same results so I assume this was an issue that was present for some time only to be found out now #2.

I can address the issues related to the multicast test in this PR such as the lint issues and the result changes in test_aux.out #1, but for the failing network tests and removal of the two tests we could probably make another PR for those.

Regards

em-redstone · 2020-06-26T17:08:26Z

Hi all,

I've fixed the lint errors, and the test_aux.out results are back to how they should be for security.password. I think it was due to the docker containers in my machine - I get these from time to time.

With the failing network module results, should I just remove them for now from test_aux for this PR?

Thanks.

grafnu · 2020-06-26T17:15:48Z

testing/test_aux.out

+RESULT fail connection.dhcp_long 
+RESULT fail connection.min_send 
+RESULT info communication.type.broadcast 
+RESULT fail protocol.app_min_send 


I think we should keep them as is (exposed) and then fix them in another PR. Before they were just hidden which is the worst situation. Now we can at least see what is wrong! I'll send a note to the team to figure out how to address this.

pisuke · 2020-08-03T10:34:29Z

Closing this as it is addressed in #565

em-redstone requested review from grafnu, noursaidi and pisuke June 25, 2020 12:04

grafnu reviewed Jun 25, 2020

View reviewed changes

em-redstone added 5 commits June 26, 2020 15:32

Added multicast count for communication.type.broadcast, removed debug…

3036b9d

…_generate_capture, added readme, fixed some typos, added network module to run during testing/test_aux.sh

Updated comments.

a8772bc

Updated docs/device_report.md

5975010

Fix bug with unicast count

c3a9509

Updated testing/test_aux.out

6ee19c2

em-redstone added 2 commits June 26, 2020 16:13

Updated code to fix lint errors

124cb29

Updated testing/test_aux.out

52ec747

grafnu approved these changes Jun 26, 2020

View reviewed changes

noursaidi mentioned this pull request Jul 29, 2020

Cleanup Network Tests and Network Test Module #565

Merged

pisuke closed this Aug 3, 2020

Comments

Conversation

em-redstone commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

grafnu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

em-redstone Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grafnu commented Jun 25, 2020 via email

Uh oh!

em-redstone commented Jun 26, 2020

Uh oh!

em-redstone commented Jun 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pisuke commented Aug 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

em-redstone commented Jun 25, 2020 •

edited

Loading

codecov bot commented Jun 25, 2020 •

edited

Loading

em-redstone Jun 25, 2020 •

edited

Loading