New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configurable network driver tests #4795

Merged
merged 13 commits into from Sep 28, 2017

Conversation

@sarahmarshy
Contributor

sarahmarshy commented Jul 21, 2017

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

This comment has been minimized.

Member

0xc0170 commented Jul 24, 2017

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 0xc0170 added the needs: work label Jul 24, 2017

TESTS/netsocket/connectivity/main.cpp Outdated
@@ -0,0 +1,54 @@
#include "mbed.h"

This comment has been minimized.

@0xc0170

0xc0170 Jul 24, 2017

Member

should contain copyright header file (any new file)

This comment has been minimized.

@sarahmarshy

sarahmarshy Jul 31, 2017

Contributor

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

This comment has been minimized.

@0xc0170

0xc0170 Aug 3, 2017

Member

Look at any file inside drivers for instance.

TESTS/netsocket/gethostbyname/main.cpp Outdated
void test_dns_literal_pref() {
SocketAddress addr;
int err = net->gethostbyname(ip_literal, &addr, ip_pref);
printf("DNS: literal %s \"%s\" => \"%s\"\n",

This comment has been minimized.

@0xc0170

0xc0170 Jul 24, 2017

Member

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

This comment has been minimized.

@sarahmarshy

sarahmarshy Jul 31, 2017

Contributor

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

This comment has been minimized.

@0xc0170

0xc0170 Aug 3, 2017

Member

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.

This comment has been minimized.

@geky

geky Aug 3, 2017

Member

What's the value of using debug here?

targets/targets.json Outdated
@@ -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"],

This comment has been minimized.

@0xc0170

0xc0170 Jul 24, 2017

Member

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?

This comment has been minimized.

@sarahmarshy

sarahmarshy Jul 24, 2017

Contributor

536063a

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

This comment has been minimized.

@sarahmarshy

sarahmarshy Jul 24, 2017

Contributor

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

This comment has been minimized.

@geky

geky Jul 24, 2017

Member

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

This comment has been minimized.

@sarahmarshy

sarahmarshy Jul 24, 2017

Contributor

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

This comment has been minimized.

@geky

geky Jul 24, 2017

Member

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.

@0xc0170 0xc0170 requested review from kjbracey-arm and geky Jul 24, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 31, 2017

@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).

tools/test_configs/__init__.py Outdated
return None
@classmethod
def get_default_config(cls, target_name):

This comment has been minimized.

@bridadan

bridadan Aug 3, 2017

Contributor

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"
        }
    }
}

This comment has been minimized.

@sarahmarshy

sarahmarshy Aug 3, 2017

Contributor

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Aug 3, 2017

Contributor

Yes. Please keep things in an external json file.

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

This comment has been minimized.

@bridadan

bridadan Aug 3, 2017

Contributor

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)

This comment has been minimized.

@sarahmarshy

sarahmarshy Aug 3, 2017

Contributor

@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.

This comment has been minimized.

@theotherjimmy

theotherjimmy Aug 3, 2017

Contributor

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Aug 3, 2017

Contributor

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

This comment has been minimized.

@sarahmarshy

sarahmarshy Aug 3, 2017

Contributor

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

This comment has been minimized.

Contributor

sarahmarshy commented Aug 4, 2017

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

@sarahmarshy

This comment has been minimized.

Contributor

sarahmarshy commented Aug 4, 2017

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

@bridadan

Looks to be moving in the right direction!

tools/test_api.py Outdated
# 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

This comment has been minimized.

@bridadan

bridadan Aug 7, 2017

Contributor

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?

This comment has been minimized.

@sarahmarshy

sarahmarshy Aug 7, 2017

Contributor

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

This comment has been minimized.

@bridadan

bridadan Aug 7, 2017

Contributor

Skipped!

@sarahmarshy

This comment has been minimized.

Contributor

sarahmarshy commented Aug 7, 2017

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 10, 2017

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

@sarahmarshy sarahmarshy force-pushed the sarahmarshy:test-configs branch 2 times, most recently Aug 10, 2017

@sarahmarshy

This comment has been minimized.

Contributor

sarahmarshy commented Aug 11, 2017

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 11, 2017

👍 Thanks

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

@bridadan

Awesome work!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 14, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 14, 2017

Result: FAILURE

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

/morph test-nightly

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 14, 2017

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

@sarahmarshy sarahmarshy force-pushed the sarahmarshy:test-configs branch 2 times, most recently Aug 14, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 14, 2017

Result: FAILURE

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

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 14, 2017

Result: FAILURE

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

/morph test-nightly

@sarahmarshy

This comment has been minimized.

Contributor

sarahmarshy commented Aug 15, 2017

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

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 15, 2017

Result: ABORTED

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

/morph test-nightly

@bridadan

This comment has been minimized.

Contributor

bridadan commented Sep 21, 2017

@screamerbg

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

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.

Or the TESTS folder?

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

This comment has been minimized.

Contributor

theotherjimmy commented Sep 21, 2017

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 21, 2017

@tommikas Could help out too?

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 21, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1351

Test failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 22, 2017

Some tests failed, 3 looks related, please review

@tommikas

This comment has been minimized.

Contributor

tommikas commented Sep 22, 2017

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

This comment has been minimized.

Contributor

sarahmarshy commented Sep 22, 2017

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

@sarahmarshy sarahmarshy force-pushed the sarahmarshy:test-configs branch to fbe8dfa Sep 22, 2017

@sarahmarshy

This comment has been minimized.

Contributor

sarahmarshy commented Sep 22, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 22, 2017

Result: NOT_BUILT

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

/morph test-nightly

@sarahmarshy

This comment has been minimized.

Contributor

sarahmarshy commented Sep 22, 2017

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 27, 2017

/morph test-nightly

1 similar comment
@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 28, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 28, 2017

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1418

All builds and test passed!

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

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test-nightly Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Oct 17, 2017

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

@sarahmarshy

This comment has been minimized.

Contributor

sarahmarshy commented Oct 17, 2017

@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

This comment has been minimized.

Member

geky commented Oct 17, 2017

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

@sarahmarshy

This comment has been minimized.

Contributor

sarahmarshy commented Oct 17, 2017

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

@geky

This comment has been minimized.

Member

geky commented Oct 17, 2017

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

This comment has been minimized.

Contributor

jeromecoutant commented Oct 18, 2017

you can run them with mbed test -m [MCU] -t gcc_arm -n mbed-os-tests-netsocket* --test-config ethernet

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

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

This comment has been minimized.

@jeromecoutant

jeromecoutant Oct 18, 2017

Contributor

This temporary debug code should be removed ?

// 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));

This comment has been minimized.

@jeromecoutant

jeromecoutant Oct 18, 2017

Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment