Add configurable network driver tests #4795

sarahmarshy · 2017-07-21T22:10:21Z

Description

This change set takes the ESP8266 networking tests (https://github.com/ARMmbed/esp8266-driver/tree/master/TESTS/net) and makes them configurable for arbitrary NSAPI implentations.

The testable interfaces can be:

A new NSAPI implementation for a module
An existing interface inside mbed OS

The testable implementations within mbed OS can be seen in this new json file. The keys in this file correspond to new device_has values in targets.json

Changes have been made to the mbed test command to allow specification of test configuration files. For example, to test EthernetInterface on a K64F:
mbed test -m k64f -t gcc_arm -n mbed-os-tests-netsocket* --test-config ethernet

The tools will now look at the test-config flag. If the specified config is in the mbed OS acceptable configs, then that will be used. The current acceptable values are ethernet and odin_wifi.

If you want to test an implentation of NSAPI for a module whose driver is outside of mbed OS, you would run:
mbed test -m k64f -t gcc_arm -n mbed-os-tests-netsocket* --test-config path\to\config\file

If the tools pick up that test-config is a valid path to a file, then that custom config will be used. These tests will be run with the assumption that the driver for this module specified in the config file is present in the current project.

The way that these configuration files are used in the tests can be seen when including a driver header, when constructing a network interface, and connecting to a network.

Notes

Because no changes have been made yet to mbed CLI, it is necessary to run the commands with --compile for now. This ensures that the new --test-config flag does not get passed to mbedgt, where it is an unknown flag.

TODO

Modify mbed CLI to ignore --test-config
Decide on approriate DEVICE_HAS flag to map the configuration files (new device_has values in targets.json map to config file paths)
Decide what to do when both test-config and app-config are present. Presently, test-config takes precedence

@bridadan @studavekar @sg- @theotherjimmy

0xc0170 · 2017-07-24T10:15:45Z

jenkins CI fails , here is one app failure

[K64F ARM]         Error: L6218E: Undefined symbol ethernet_address (referred from Ethernet.o).
[K64F ARM]         Error: L6218E: Undefined symbol ethernet_free (referred from Ethernet.o).
[K64F ARM]         Error: L6218E: Undefined symbol ethernet_init (referred from Ethernet.o).
[K64F ARM]         Error: L6218E: Undefined symbol ethernet_link (referred from Ethernet.o).
[K64F ARM]         Error: L6218E: Undefined symbol ethernet_read (referred from Ethernet.o).
[K64F ARM]         Error: L6218E: Undefined symbol ethernet_receive (referred from Ethernet.o).
[K64F ARM]         Error: L6218E: Undefined symbol ethernet_send (referred from Ethernet.o).
[K64F ARM]         Error: L6218E: Undefined symbol ethernet_set_link (referred from Ethernet.o).
[K64F ARM]         Error: L6218E: Undefined symbol ethernet_write (referred from Ethernet.o).
[K64F ARM]         Finished: 0 information, 1 warning and 9 error messages

0xc0170 · 2017-07-24T10:16:30Z

TESTS/netsocket/connectivity/main.cpp

@@ -0,0 +1,54 @@
+#include "mbed.h"


should contain copyright header file (any new file)

Can you point me to a place I can c/p this? I see a 2016 copyright on some of the other tests..

Look at any file inside drivers for instance.

0xc0170 · 2017-07-24T10:17:20Z

TESTS/netsocket/gethostbyname/main.cpp

+void test_dns_literal_pref() {
+    SocketAddress addr;
+    int err = net->gethostbyname(ip_literal, &addr, ip_pref);
+    printf("DNS: literal %s \"%s\" => \"%s\"\n",


shouldn't all print be off by default (only if debug is set, then should be verbose) ?

Plenty of other tests use printf. For exmaple, this is the current gethostbyname LWIP test - https://github.com/ARMmbed/mbed-os/blob/master/features/FEATURE_LWIP/TESTS/mbedmicro-net/gethostbyname/main.cpp#L81

I know, anyway asking for your opinion or others. Are these prints valuable if test passes? if it passes, all should be OK. If it fails, than this debug info might be vital.

What's the value of using debug here?

0xc0170 · 2017-07-24T10:20:54Z

targets/targets.json

@@ -1440,7 +1440,7 @@
        "core": "Cortex-M4F",
        "extra_labels_add": ["STM32F4", "STM32F439", "STM32F439ZI","STM32F439xx", "STM32F439xI"],
        "macros": ["HSE_VALUE=24000000", "HSE_STARTUP_TIMEOUT=5000", "CB_INTERFACE_SDIO","CB_CHIP_WL18XX","SUPPORT_80211D_ALWAYS","WLAN_ENABLED","MBEDTLS_ARC4_C","MBEDTLS_DES_C","MBEDTLS_MD4_C","MBEDTLS_MD5_C","MBEDTLS_SHA1_C"],
-        "device_has_add": ["CAN", "EMAC", "TRNG", "FLASH"],
+        "device_has_add": ["CAN", "EMAC", "TRNG", "FLASH", "ETHERNET", "ODIN_WIFI"],


What does this Ethernet flag do, how is it related to this patch? I could not find it in the commits, only that you are adding it ? either odin_wifi. device has might not be the best place to define these?

536063a

It maps to a test configuration file for a network interface that has a driver in mbed OS.

the relevant file in that commit is tools/test_configs/config_paths.json

Ah, this ETHERNET flag is what's causing the ethernet errors in CI. It actual enables the Ethernet class, and old class that is effectively deprecated.

Your best bet would be ETH, although its adoption is a work in progress: #4079

Maybe I could reserve them with an uncommon prefix. Someting like.... NSAPI_ETHand NSAPI_ODIN_WIFI?

Ah, unsure in terms of mbed OS's scope. Not sure we want seperate flags for each dependency. But it may be a good short term solution until some of the network architecture is finished.

theotherjimmy · 2017-07-31T15:09:37Z

@sarahmarshy Could you respond to @0xc0170's comments?

bridadan

Besides the things already mentioned in this PR, I think the general approach seems solid. I mentioned one suggestion regarding how the tools handle configurations and how we store the test data.

The tests look good though! If these were to get merged this PR should also pull out all of the general network tests that live in the FEATURE_LWIP folder (that folder should contain LWIP specific tests if they exist).

bridadan · 2017-08-03T15:01:19Z

tools/test_configs/__init__.py

+            return None
+
+    @classmethod
+    def get_default_config(cls, target_name):


Having this kind of capability is important. It looks like it currently selects the first config as the default?

I know there was a discussion about moving the test configs out of the device_has field, which I think is the right decision. I would suggest creating a new field in either targets.json or another file in the tools directory. If we were to create this new field, perhaps we could have a specific data structure that lets me explicitly set the default configuration?

For instance:

{ "UBLOX_EVK_ODIN_W2": { "default_test_configuration": "ODIN_WIFI", "test_configurations": { "ODIN_WIFI": "OdinInterface.json", "ETHERNET": "EthernetInterface.json" } } }

@theotherjimmy was concerned about polluting targets.json. If that is still the case, I guess a new json file would be the best approach?

Yes. Please keep things in an external json file.

bridadan · 2017-08-03T15:02:50Z

tools/test.py

+        elif not options.app_config:
+            config = TestConfig.get_default_config(mcu)
+        else:
+            config = options.app_config


Instead of selecting either test config or app_config, would it be possible to merge the two, with app_config keys overriding existing test_config keys? This would be useful for things like WiFi credentials (so we don't have to add every bit of information on the command line)

@theotherjimmy would it be possible to allow the tools to accept a dictionary rather than a json file? Otherwise, I'd need to write a new file or change the existing one.

Sure, what do you mean by "the tools" and what kind of json file?

I'm asking, which part of the tools BTW.

The app configuration json file. The affected part of the tools would be build APIs called into by the test tools https://github.com/sarahmarshy/mbed-os/blob/2e161c9a0b6d4a584593de5938739d706b4b0c09/tools/test.py#L227

theotherjimmy

The tools/test_configs/__init__.py modle contains a class with no constructor and only class methods. These methods should just be functions in the module. There is no need for them to be part of a class.

If you want to have "class variables" just make them variables in the module itself. If you want them to be private, just prefix them with an _.

sarahmarshy · 2017-08-04T21:44:26Z

@theotherjimmy the new commit 968e19d should fulfill your request.

sarahmarshy · 2017-08-04T21:45:29Z

@bridadan new commit 1dccd9e removes LWIP tests and 968e19d has your suggestions for default tests.

bridadan

Looks to be moving in the right direction!

bridadan · 2017-08-07T15:11:24Z

tools/test_api.py

+                    # If the target has no network interface configuration, netsocket tests fail to compile
+                    if not module_config and not configs and \
+                        (test_case_directory == 'netsocket' or test_group_directory == 'netsocket'):
+                        continue


I remember us going through this at the time, but I think we might be able to solve this in a more general way.

For all of the tests that require the specific macro you use to construct the network interfaces, would it be possible to check for that macro with an #ifdef and then use the #error [NOT_SUPPORTED] feature of the tools to skip building it?

#if !MBED_CONF_APP_CONNECT_STATEMENT #error [NOT_SUPPORTED] No network configuration found for this target. #endif

Something like that at the top of each test would do the trick.

@sarahmarshy Does that sound right?

Would that mean the test result is a fail though? Or would they just be skipped?

sarahmarshy · 2017-08-07T18:14:10Z

@bridadan and @0xc0170, ff45626 adds copyright headers and uses mbed error to skip tests without network configuration files

theotherjimmy · 2017-08-10T14:15:24Z

@sarahmarshy IT looks like you broke the Uvisor CI. Could you take a look?

sarahmarshy · 2017-08-11T15:12:24Z

@theotherjimmy I've fixed the build and also rebased onto master to get rid of the merge conflicts.

theotherjimmy · 2017-08-11T15:46:26Z

👍 Thanks

bridadan

Awesome work!

theotherjimmy · 2017-08-14T16:29:15Z

/morph test-nightly

mbed-bot · 2017-08-14T18:34:52Z

Result: FAILURE

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

theotherjimmy · 2017-08-14T18:51:48Z

@sarahmarshy Could you rebase onto master instead of merging please? Your PR cannot be accepted into a patch release with a merge commit.

mbed-bot · 2017-08-14T22:50:46Z

Result: FAILURE

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

mbed-bot · 2017-08-14T22:50:56Z

Result: FAILURE

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

sarahmarshy · 2017-08-15T16:34:07Z

Ok, one more time... After rebase...
/morph test-nightly

mbed-bot · 2017-08-15T16:40:42Z

Result: ABORTED

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

screamerbg

Why not have the file as part of the target, not in specific tools folder? Or the TESTS folder?

bridadan · 2017-09-21T13:48:19Z

@screamerbg

Pretty sure that @theotherjimmy doesn't want the targets.json getting any bigger with this stuff.

If you're referring to moving the target specific test configs into the target's folder, then the tools would have to search for these files using a directory walk. It could be done that way, but it seems like quite a complex solution at the moment.

I think the reasoning here is that these configs are for ALL tests, not just one group of tests. They are intended to change the default config for a target so its ready for testing.

@sarahmarshy hopefully I got these right, feel free to correct me!

theotherjimmy · 2017-09-21T14:10:33Z

@marhil01 It looks like your CI is not reporting to github here. Could you take a look?

theotherjimmy · 2017-09-21T14:12:28Z

@tommikas Could help out too?

mbed-bot · 2017-09-21T21:24:21Z

Result: FAILURE

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

Output

mbed Build Number: 1351

Test failed!

0xc0170 · 2017-09-22T09:44:10Z

Some tests failed, 3 looks related, please review

tommikas · 2017-09-22T11:09:24Z

The pr-head failures are not necessarily related to this PR, so I restarted the build again. The build had already passed with these changes but as Jimmy mentioned the update didn't reach github.

@theotherjimmy @0xc0170 FYI: We're running low on armcc licenses. More has been requested but until we get them we may see some failures due to that.

sarahmarshy · 2017-09-22T14:38:17Z

Ah, I didn't update the sigio test to point to os.mbed.com. Will do that now.

sarahmarshy · 2017-09-22T14:41:00Z

/morph test-nightly

mbed-bot · 2017-09-22T18:16:21Z

Result: NOT_BUILT

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

sarahmarshy · 2017-09-22T19:16:26Z

@0xc0170 and @sg- can a release tag be added to this PR?

theotherjimmy · 2017-09-27T14:19:41Z

/morph test-nightly

studavekar · 2017-09-28T03:15:02Z

/morph test-nightly

mbed-bot · 2017-09-28T14:32:40Z

Result: SUCCESS

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

Output

mbed Build Number: 1418

All builds and test passed!

jeromecoutant · 2017-10-17T08:32:00Z

Hi
Since this PR, LWIP tests are no more executed...
How can we execute former features-feature_lwip-tests-mbedmicro-net-* tests ?
Thx

sarahmarshy · 2017-10-17T17:36:54Z

@jeromecoutant, wow, you're right. I will work on getting that fixed for CI. In the meantime, you can run them with mbed test -m [MCU] -t gcc_arm -n mbed-os-tests-netsocket* --test-config ethernet

geky · 2017-10-17T19:25:32Z

Confirmed with @studavekar, we're still running the network tests in CI. We are currently using the command in @sarahmarshy's post.

sarahmarshy · 2017-10-17T19:44:01Z

@geky and @studavekar, can you look at #5335? That would eliminate the need to change the command in CI.

geky · 2017-10-17T19:57:22Z

Looks like we made a mistake, only the boards with a special config are running (currently K64F). No other network tests are running, so we're going to need #5335.

jeromecoutant · 2017-10-18T06:42:46Z

This command only works if tools/test_configs/target_configs.json file is updated.

jeromecoutant

Please, check my comment on the tcp_echo test update
Thx

jeromecoutant · 2017-10-18T11:24:34Z

TESTS/netsocket/tcp_echo/main.cpp

+        mac += uuid[i];
+    }
+    //mbed_set_mac_address((const char*)mac, /*coerce control bits*/ 1);
+


This temporary debug code should be removed ?

jeromecoutant · 2017-10-18T12:49:15Z

TESTS/netsocket/tcp_echo/main.cpp

-        // Server will respond with HTTP GET's success code
-        const int ret = sock.recv(rx_buffer, sizeof(rx_buffer));
-        printf("MBED: Finished receiving\r\n");
+        sock.recv(rx_buffer, sizeof(MBED_CONF_APP_TCP_ECHO_PREFIX));


Add a test like
if MBED_CONF_APP_TCP_ECHO_PREFIX not "", then socket recv ?

0xc0170 added the needs: work label Jul 24, 2017

0xc0170 reviewed Jul 24, 2017

View reviewed changes

0xc0170 requested review from kjbracey and geky July 24, 2017 10:21

bridadan reviewed Aug 3, 2017

View reviewed changes

theotherjimmy suggested changes Aug 3, 2017

View reviewed changes

bridadan reviewed Aug 7, 2017

View reviewed changes

theotherjimmy approved these changes Aug 7, 2017

View reviewed changes

sarahmarshy force-pushed the test-configs branch 2 times, most recently from 7f4bbb3 to 93b412c Compare August 10, 2017 17:01

theotherjimmy added needs: CI and removed needs: work labels Aug 11, 2017

bridadan approved these changes Aug 11, 2017

View reviewed changes

sarahmarshy force-pushed the test-configs branch 2 times, most recently from a15f245 to 34fa5f4 Compare August 14, 2017 19:57

screamerbg reviewed Sep 21, 2017

View reviewed changes

SeppoTakalo mentioned this pull request Sep 21, 2017

Implement functional Wifi tests #5161

Merged

update domain name to os.mbed.com

fbe8dfa

sarahmarshy force-pushed the test-configs branch from c2358ef to fbe8dfa Compare September 22, 2017 14:40

sg- added the release-version: 5.6.2 label Sep 23, 2017

theotherjimmy added ready for merge and removed needs: CI labels Sep 28, 2017

theotherjimmy merged commit e3cb228 into ARMmbed:master Sep 28, 2017

theotherjimmy removed the ready for merge label Sep 28, 2017

yennster mentioned this pull request Oct 17, 2017

Fix for configurable network driver tests #5335

Merged

jeromecoutant reviewed Oct 18, 2017

View reviewed changes

Add configurable network driver tests #4795

Add configurable network driver tests #4795

Uh oh!

Conversation

sarahmarshy commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes

TODO

Uh oh!

0xc0170 commented Jul 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarahmarshy Jul 31, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarahmarshy Jul 31, 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!

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!

sarahmarshy Jul 24, 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!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Jul 31, 2017

Uh oh!

bridadan 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!

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!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

sarahmarshy commented Aug 4, 2017

Uh oh!

sarahmarshy commented Aug 4, 2017

Uh oh!

bridadan 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

sarahmarshy commented Jul 21, 2017 •

edited

Loading

sarahmarshy Jul 31, 2017 •

edited

Loading

sarahmarshy Jul 31, 2017 •

edited

Loading

sarahmarshy Jul 24, 2017 •

edited

Loading