Networking test fixes #4240

bridadan · 2017-04-27T22:43:50Z

Description

This PR attempts to fix a number of issues that have cropped up with the networking tests lately. This pr is still in development.

Status

IN DEVELOPMENT

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

NO

Todos

Transition all Greentea tests that use TEST_ASSERT statements to utest-style tests
Generate MAC addresses from the Greentea UUID to help prevent MAC conflicts (should help with DHCP failures)
Get the nightly passing, maybe run additional stress testing

bridadan · 2017-04-27T22:44:18Z

/morph test-nightly

mbed-bot · 2017-04-27T23:02:55Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 102

Build failed!

bridadan · 2017-04-28T01:07:48Z

/morph test-nightly

mbed-bot · 2017-04-28T04:44:16Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 103

Test failed!

kegilbert · 2017-05-01T21:07:42Z

/morph test-nightly

mbed-bot · 2017-05-01T22:23:25Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 114

Test failed!

bridadan · 2017-05-01T22:28:54Z

/morph test-nightly

mbed-bot · 2017-05-02T01:57:33Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 115

All builds and test passed!

bridadan · 2017-05-02T03:25:03Z

@geky I think we need to take a look at the udp_echo_parallel test, I'm not so sure that this is very reliable as it is currently written. I think we may need to reorganize or it come up with something different.

geky · 2017-05-03T19:47:28Z

/morph test-nightly

kegilbert

Good update 👍

bridadan · 2017-05-03T20:34:30Z

features/FEATURE_LWIP/TESTS/mbedmicro-net/udp_echo_parallel/main.cpp

            }

-            i++;


You should keep this i here. It is keeping track of the total number of loops, not the number of successes.

The dtls test already has the ability to retry upon a UDP failure. However the sockets are currently configured to be blocking and to wait forever. I added a timeout of 1.5 seconds in order the test to correctly timeout.

This adds some extra logs to the test for debugging purposes. Also allows the test to fail immediately if it fails to obtain an IP address.

The Greentea UUID can be used as a source of entropy/randomness during the test. This is useful for uniquely identifying a test run.

Before, the UDP test was very strict on the number of packets it would try to match before failing. Now it will keep trying for the whole test to get enough passing packets. It also includes the test's UUID so you can validate which packets are being received.

Making the test more forgiven for minor networking issues. Also adding more debug prints to make it easier to see which packets are coming from where.

…et device as Locally Administrated and Unicast

This matches the timeout used in linux: https://linux.die.net/man/5/dhclient.conf This resolves several issues noticed during testing when we have a very large number of devices that try to get an IP address around the same time.

Avoids getting stuck in loop where the device always picks up the previous test's packets

Avoids accidentally using a part of the uuid that isn't very unique. ie, the mac address component of type 1 uuids.

bridadan

Looks ok ~~, when you rebase can you also do it on top of master? I think a targets.json has crept in here for some reason.~~ (you are too quick for me!)

bridadan · 2017-05-03T21:24:45Z

features/FEATURE_LWIP/TESTS/mbedmicro-net/tcp_echo/main.cpp

+}
+
+Case cases[] = {
+    Case("Testing TCP echo", test_tcp_echo),


Nit: seems a bit redundant to put "Testing" in the title of a test case.

Haha, a quick grep shows it's traditional :P

TESTS/mbedmicro-rtos-mbed/threads/main.cpp: Case("Testing parallel threads with murder", test_parallel_threads<3, increment_with_murder>), TESTS/mbedmicro-rtos-mbed/threads/main.cpp: Case("Testing serial threads with murder", test_serial_threads<10, increment_with_murder>), TESTS/mbedmicro-rtos-mbed/threads/main.cpp: Case("Testing thread self terminate", test_self_terminate), TESTS/mbedmicro-rtos-mbed/mutex/main.cpp: Case("Test single thread lock", test_single_thread_lock), TESTS/mbedmicro-rtos-mbed/mutex/main.cpp: Case("Test single thread trylock", test_single_thread_trylock), TESTS/mbedmicro-rtos-mbed/mutex/main.cpp: Case("Test single thread lock recursive", test_single_thread_lock_recursive), TESTS/network/wifi/main.cpp: Case("Scan test", wifi_scan, greentea_failure_handler), TESTS/network/wifi/main.cpp: Case("Connect test", wifi_connect, greentea_failure_handler), TESTS/network/wifi/main.cpp: Case("Scan while connected test", wifi_connect_scan, greentea_failure_handler), TESTS/network/wifi/main.cpp: Case("HTTP test", wifi_http, greentea_failure_handler),

but I can update

Ah woops haha. Nevermind then!

Too late, is updated : P

@bridadan

per @bridadan

bridadan · 2017-05-03T21:57:04Z

/morph test-nightly

mbed-bot · 2017-05-03T22:17:33Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 142

Build failed!

This is a workaround for IAR's lack of flexibility with memory regions. Otherwise these tests would use very little heap and be mostly global allocations.

geky · 2017-05-04T16:18:48Z

/morph test-nightly

mbed-bot · 2017-05-04T22:48:15Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 154

Test failed!

bridadan · 2017-05-12T21:47:51Z

/morph test-nightly

bridadan · 2017-05-12T21:51:29Z

@adbridge and/or @mazimkhan, could you take a look at the Greentea client changes I've made? Everything seems to be ok, but doesn't hurt to have a few extra eyes on this.

mbed-bot · 2017-05-13T05:12:11Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 207

All builds and test passed!

adbridge

There are a lot of instances of magic numbers, where a macro should be used.

General lack of commenting of the changes.

Multiple instances of the same comments ( so please check through all changes for duplicates where I haven't marked them all individually...

adbridge · 2017-05-15T10:38:58Z

features/frameworks/greentea-client/source/greentea_test_env.cpp

- *           This function is blocking.
- */
-void GREENTEA_SETUP(const int timeout, const char *host_test_name) {
+


No header for this function ??

adbridge · 2017-05-15T10:41:08Z

features/frameworks/greentea-client/source/greentea_test_env.cpp

+    char _key[8] = {0};
+
+    while (1) {
+        greentea_parse_kv(_key, buffer, sizeof(_key), size);
        greentea_write_string("mbedmbedmbedmbedmbedmbedmbedmbed\r\n");
        if (strcmp(_key, GREENTEA_TEST_ENV_SYNC) == 0) {
            // Found correct __sunc message


adbridge · 2017-05-15T10:42:22Z

features/frameworks/greentea-client/source/greentea_test_env.cpp

+ *           After host test name is received master will invoke host test script
+ *           and add hos test's callback handlers to main event loop
+ *           This function is blocking.
+ */


Should describe how this differs from standard GREENTEA_SETUP

adbridge · 2017-05-15T10:45:01Z

features/FEATURE_LWIP/TESTS/mbedmicro-net/udp_echo/main.cpp

+
+    tx_buffer[i++] = ' ';
+
+    for (; i<tx_size; ++i) {
        tx_buffer[i] = (rand() % 10) + '0';


Would be nice to have a comment saying what this is actually doing and why

adbridge · 2017-05-15T10:45:41Z

features/FEATURE_LWIP/TESTS/mbedmicro-net/udp_echo/main.cpp

        tx_buffer[i] = (rand() % 10) + '0';
    }
 }

 int main() {
-    GREENTEA_SETUP(60, "udp_echo");
-
+    char uuid[48] = {0};


Why 48 characters ? Shouldn't this be a #define ?

adbridge · 2017-05-15T13:52:03Z

features/FEATURE_LWIP/TESTS/mbedmicro-net/tcp_echo/main.cpp

+    for (int i = 0; i < sizeof(uuid); i++) {
+        mac += uuid[i];
+    }
+    mbed_set_mac_address((const char*)mac, /*coerce control bits*/ 1);


comment in function

adbridge · 2017-05-15T13:52:22Z

features/FEATURE_LWIP/TESTS/mbedmicro-net/tcp_echo_parallel/main.cpp

+
+// Test setup
+utest::v1::status_t test_setup(const size_t number_of_cases) {
+    char uuid[48] = {0};


magic number

adbridge · 2017-05-15T13:52:36Z

features/FEATURE_LWIP/TESTS/mbedmicro-net/tcp_echo_parallel/main.cpp

+    for (int i = 0; i < sizeof(uuid); i++) {
+        mac += uuid[i];
+    }
+    mbed_set_mac_address((const char*)mac, /*coerce control bits*/ 1);


comment inside function

adbridge · 2017-05-15T13:53:38Z

features/FEATURE_LWIP/TESTS/mbedmicro-net/tcp_hello_world/main.cpp

@@ -90,5 +84,32 @@ int main() {
    }

    eth.disconnect();
-    GREENTEA_TESTSUITE_RESULT(result);
+    TEST_ASSERT_EQUAL(true, result);


Should be TEST_ASSERT_TRUE(result)

adbridge · 2017-05-15T13:54:49Z

features/FEATURE_LWIP/TESTS/mbedmicro-net/udp_echo_parallel/main.cpp

@@ -204,6 +194,32 @@ int main() {
        }

        net.disconnect();
-        GREENTEA_TESTSUITE_RESULT(result);
+        TEST_ASSERT_EQUAL(true, result);


TEST_ASSERT_TRUE

mazimkhan

I have general design comment:
For getting a random mac address do we really need to change greentea API? It can be done by generating the mac address in the host test script and sending a key,value pair to the target. That will delegate following repeated code to the host side where the UUID is coming from:

+    char uuid[48] = {0};
 +    GREENTEA_SETUP_UUID(120, "default_auto", uuid, 48);
 +
 +    // create mac address based on uuid
 +    uint64_t mac = 0;
 +    for (int i = 0; i < sizeof(uuid); i++) {
 +        mac += uuid[i];
 +    }

Also above adding UUID bytes can compromise the uniqueness and randomness of UUID. For instance if there are two different sequences like 05 04 06 and 06 05 04 they will produce same sum. Hence the position of the bytes is just as important as their value.
In my opinion host side can produce more strong random and unique mac address that can be given to the target test to use.

bridadan · 2017-05-23T00:31:10Z

Thanks for all of the feedback, especially @adbridge and @mazimkhan. After doing more examination, the method we went about for overriding the mbed mac address doesn't actually work since most vendors that have LWIP implementations already override the mbed_mac_address function. (should have seen that one, face palm).

I'm closing this PR in favor of the updated (and reduced scope) #4369. Let's continue the discussion there, thanks!

bridadan · 2017-05-23T00:40:48Z

Also, added the issue #4370 to help track some of the outstanding issues (including what we have already tried to solve them).

bridadan force-pushed the networking_test_fixes branch from 3893d9e to 11f4ee0 Compare April 28, 2017 01:06

theotherjimmy added the do not merge label Apr 28, 2017

kegilbert reviewed May 3, 2017

View reviewed changes

geky force-pushed the networking_test_fixes branch from 8e31a49 to 457b1c1 Compare May 3, 2017 20:23

bridadan commented May 3, 2017

View reviewed changes

bridadan and others added 14 commits May 3, 2017 16:30

Allowing dtls test to retry correctly.

fecb005

The dtls test already has the ability to retry upon a UDP failure. However the sockets are currently configured to be blocking and to wait forever. I added a timeout of 1.5 seconds in order the test to correctly timeout.

Adding failure logs to TCP echo test.

86c1680

This adds some extra logs to the test for debugging purposes. Also allows the test to fail immediately if it fails to obtain an IP address.

Adding ability to preserve Greentea UUID in test.

41fd05c

The Greentea UUID can be used as a source of entropy/randomness during the test. This is useful for uniquely identifying a test run.

Making udp_echo_parallel test more robust against network issues.

27a97f9

Making the test more forgiven for minor networking issues. Also adding more debug prints to make it easier to see which packets are coming from where.

Arranging logic in test to prevent left-over behavior

4d672e0

Add ability to set MAC address to custom value

6862912

Added in flag to allow optional coercion of provided MAC address to s…

984af4b

…et device as Locally Administrated and Unicast

Test modifications to set MAC address to UUID

abcdd01

Fixing some issues in the tcp/udp tests

d55b3cf

lwip: Increased DHCP timeout to 60 seconds

9c98f3f

This matches the timeout used in linux: https://linux.die.net/man/5/dhclient.conf This resolves several issues noticed during testing when we have a very large number of devices that try to get an IP address around the same time.

lwip: Added udp buffer flushing after dropped packets

2761a8c

Avoids getting stuck in loop where the device always picks up the previous test's packets

lwip: Added hash to mac addresses during tests

81bedff

Avoids accidentally using a part of the uuid that isn't very unique. ie, the mac address component of type 1 uuids.

lwip: Fixed count of udp iterations to reflect attempts

3c7f2ab

bridadan commented May 3, 2017

View reviewed changes

geky force-pushed the networking_test_fixes branch from 0a65f88 to 071b79b Compare May 3, 2017 21:31

lwip: Migrated to utest framework

56ba283

per @bridadan

geky force-pushed the networking_test_fixes branch from 071b79b to 56ba283 Compare May 3, 2017 21:50

Moved network test buffers into heap

d852bf6

This is a workaround for IAR's lack of flexibility with memory regions. Otherwise these tests would use very little heap and be mostly global allocations.

bridadan changed the title ~~[WIP, DO NOT MERGE] Networking test fixes~~ Networking test fixes May 12, 2017

bridadan added needs: CI and removed do not merge labels May 12, 2017

bridadan added the needs: review label May 12, 2017

adbridge self-requested a review May 15, 2017 13:56

adbridge requested changes May 15, 2017

View reviewed changes

adbridge added needs: work and removed needs: review labels May 15, 2017

0xc0170 removed the needs: CI label May 15, 2017

mazimkhan suggested changes May 15, 2017

View reviewed changes

bridadan mentioned this pull request May 23, 2017

Increase DHCP timeout, and rework networking tests logic #4369

Merged

1 task

bridadan closed this May 23, 2017

sg- removed the needs: work label May 23, 2017

bridadan mentioned this pull request May 23, 2017

Complications during networking tests #4370

Closed

Networking test fixes #4240

Networking test fixes #4240

Uh oh!

Conversation

bridadan commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Migrations

Todos

Uh oh!

bridadan commented Apr 27, 2017

Uh oh!

mbed-bot commented Apr 27, 2017

Output

Uh oh!

bridadan commented Apr 28, 2017

Uh oh!

mbed-bot commented Apr 28, 2017

Output

Uh oh!

kegilbert commented May 1, 2017

Uh oh!

mbed-bot commented May 1, 2017

Output

Uh oh!

bridadan commented May 1, 2017

Uh oh!

mbed-bot commented May 2, 2017

Output

Uh oh!

bridadan commented May 2, 2017

Uh oh!

geky commented May 3, 2017

Uh oh!

kegilbert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridadan left a comment • 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!

geky May 3, 2017 • 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!

bridadan commented May 3, 2017

Uh oh!

mbed-bot commented May 3, 2017

Output

Uh oh!

geky commented May 4, 2017

Uh oh!

mbed-bot commented May 4, 2017

Output

Uh oh!

bridadan commented May 12, 2017

Uh oh!

bridadan commented May 12, 2017

Uh oh!

mbed-bot commented May 13, 2017

Output

Uh oh!

adbridge left a comment

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!

bridadan commented Apr 27, 2017 •

edited

Loading

bridadan left a comment •

edited

Loading

geky May 3, 2017 •

edited

Loading

mazimkhan left a comment •

edited

Loading