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

Clarify test configuration in Socket/Networking test document #8060

Merged
merged 1 commit into from Sep 21, 2018

Conversation

Projects
None yet
7 participants
@SeppoTakalo
Contributor

SeppoTakalo commented Sep 10, 2018

Description

Clarify test configuration in Socket/Networking test document.
Current documentation instructs user to supply mbed_app.json and rely content of tools/test_configuration to be appended. However, this is not working.
Full mbed_app.json needs to be provided.

Fixes #8055

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change
@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 10, 2018

@MarceloSalazar Please review. This should clarify how to supply all the required parameters for Socket+Network testing.

@MarceloSalazar

This comment has been minimized.

Contributor

MarceloSalazar commented Sep 10, 2018

@SeppoTakalo thanks for looking into this.

I'm not able to get it working on the K64F. I'm probably missing something.
Note I'm trying to use ETHERNET.

It still complains:

$ mbed test --compile -t GCC_ARM -m K64F -n mbed-os-tests-network-* 
...
Building project wifi (K64F, GCC_ARM)
Scan: GCC_ARM
Scan: wifi                                                         //  <--- Why is this here?
Compile [  5.3%]: get_interface.cpp
[Error] get_interface.cpp@21,2: #error [NOT_SUPPORTED] No network configuration found for this target.
[Error] get_interface.cpp@24,2: #error [NOT_SUPPORTED] Requires parameters from mbed_app.json

With the new changes you've introduced, it suggests to create a mbed_app.json but I think it's missing the default network interface.

{
    "config": {
        "echo-server-addr" : {
            "help" : "IP address of echo server",
            "value" : "\"echo.mbedcloudtesting.com\""
        },
        "echo-server-port" : {
            "help" : "Port of echo server",
            "value" : "7"
        }
    },
    "macros": ["MBED_EXTENDED_TESTS"]
}

What do you think is missing?

I've also tried the existing config with no luck:

$ mbed test --compile -t GCC_ARM -m K64F -n mbed-os-tests-network-* --app-config mbed-os/tools/test_configs/EthernetInterface.json 
@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 11, 2018

@MarceloSalazar What you described is the correct behaviour.

K64F does not have wifi, so compiling Wifi test should fail in #error [NOT_SUPPORTED] but DNS and TCP test cases should compile just fine.

Also, leaving the default interface is intentional, as K64F should provide one from the target.json
When you would try to compile same with F411 or anything that does not have any interface, it should fail and mark those testcases as skipped.

But if you claim in the configuration that default network interface is ethernet, you force those to be build in, which would lead to linker failure, because device would not have ethernet driver.

WiFi configuration instead is meant to force the wifi to be default, because WiFi board may choose to have the Ethernet as default interface. There is no hard guideline there. It is up to the board.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 11, 2018

The sample configuration that I provided:

mbed test --compile -t GCC_ARM -m K64F -n mbed-os-tests-net* 
...
Build successes:
  * K64F::GCC_ARM::MBED-BUILD
  * K64F::GCC_ARM::MBED-OS-TESTS-NETSOCKET-DNS
  * K64F::GCC_ARM::MBED-OS-TESTS-NETSOCKET-TCP
  * K64F::GCC_ARM::MBED-OS-TESTS-NETSOCKET-UDP


Build skips:
  * K64F::GCC_ARM::MBED-OS-TESTS-NETWORK-EMAC
  * K64F::GCC_ARM::MBED-OS-TESTS-NETWORK-WIFI

Which works as expected.

mbed-os-network is network layer tests, WiFi and EMAC. Both should be skipped, because I have not supplied EMAC test parameters. WiFi skips because it is not Wifi.
But TCP, UDP and DNS test cases build because proper network configuration and interface is provided.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 11, 2018

Please note that socket tests are under netsocket not network

@MarceloSalazar

This comment has been minimized.

Contributor

MarceloSalazar commented Sep 11, 2018

thanks @SeppoTakalo . I was messing up things completely.

In order to clarify things for the users, do you think we could introduce these simple changes in the Building and Running section of the docs. Maybe merging both building and running sections but making clear what people should be running:

Building and Running

WiFi and EMAC (Ethernet)

mbed test -t -m -n mbed-os-tests-network-*

Socket tests (DNS, TCP, UDP)

mbed test -t -m -n mbed-os-tests-netsocket*

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 12, 2018

@MarceloSalazar Yes. It would make sense.
The test document probably too much assumes that who ever reads it, has already gone through porting guides, and knows the setup required for those. But this is not always the case.

I'll keep this in mind for the next updates into this document.

@cmonr

cmonr approved these changes Sep 12, 2018

@cmonr cmonr added needs: CI and removed needs: review labels Sep 12, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 12, 2018

/morph build

@MarceloSalazar

This comment has been minimized.

Contributor

MarceloSalazar commented Sep 12, 2018

I'd like those minor changes in this PR for RC3. Should be a matter of copy & paste.
@SeppoTakalo can we do this asap, please?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 12, 2018

Stopping build since changes have been requested.

@cmonr cmonr added needs: work and removed needs: CI labels Sep 12, 2018

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 13, 2018

@MarceloSalazar What is wrong with the form that is already written in the document: https://github.com/ARMmbed/mbed-os/blob/5e9fb0d5627a3d83add99bc1b93803108fdf2e71/TESTS/netsocket/README.md#running-tests


Running tests

When device is connected to network, or in case of wireless device near the access point.

mbed test -n mbed-os-tests-network-*,mbed-os-tests-netsocket*

That runs both set of test cases. I don't see the point of separating the test runs.

@MarceloSalazar

This comment has been minimized.

Contributor

MarceloSalazar commented Sep 13, 2018

@SeppoTakalo I've reviewed the ticket #8055 and this PR again and realize there are many assumptions on what tests & configuration people need to run.

Having a single line means everyone will have the environment and configuration ready to work straight away, which is not the case.
mbed test -n mbed-os-tests-network-*,mbed-os-tests-netsocket*

You see above I failed at running this on K64F (Ethernet), because I didn't have the correct configuration in place and didn't understand the pre-conditions.

I addition to splitting the lines for tests (as suggested #8060 (comment)), I really want us to add a small table with snippets of configurations for different cases (I don't think we need to create json files)

Test name mbed-os-tests-network* mbed-os-tests-netsocket*
Test cases WiFi, EMAC DNS, UDP, TCP
WiFi config wifi_emac.json wifi_net.json
Ethernet config eth_emac.json eth_net.json

If you want, I can send a PR on top of this with the proposal.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 14, 2018

Yes, I agree now.

The table makes sense, but can easily be done post-release.

@MarceloSalazar

This comment has been minimized.

Contributor

MarceloSalazar commented Sep 14, 2018

Thanks, let's get this merged asap and work on this extra bit after the release.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 14, 2018

@MarceloSalazar Would you mind accepting the review, so that you have a checkmark next to your name in the reviewers menu?

@cmonr cmonr added the needs: CI label Sep 14, 2018

@cmonr cmonr removed the needs: work label Sep 14, 2018

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Sep 16, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 16, 2018

Build : SUCCESS

Build number : 3079
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8060/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 17, 2018

Gotta love it when doc PRs cause test issues...

Will retest later today.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 20, 2018

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Sep 21, 2018

@0xc0170 0xc0170 merged commit cbb676c into ARMmbed:master Sep 21, 2018

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job: successful
Details
travis-ci/astyle Passed, 598 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10285 cycles (+1126 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment