Skip to content
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

Publish tests documentation for DNS, Emac, NetworkInterface and Wifi #9630

Merged
merged 4 commits into from Feb 12, 2019

Conversation

@michalpasztamobica
Copy link
Contributor

commented Feb 6, 2019

Description

Documentation moved from internal Confluence pages.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[x] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@VeijoPesonen
@KariHaapalehto
@mtomczykmobica
@tymoteuszblochmobica
@kjbracey-arm

@ciarmcom ciarmcom requested review from KariHaapalehto, kjbracey-arm, mtomczykmobica, SeppoTakalo, tymoteuszblochmobica, VeijoPesonen and ARMmbed/mbed-os-maintainers Feb 6, 2019

@NirSonnenschein NirSonnenschein requested a review from ARMmbed/mbed-docs Feb 6, 2019

TESTS/netsocket/README.md Outdated
@@ -18,6 +18,7 @@ The target for this plan is to test:
- [UDPSocket](https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/UDPSocket.h).
- [TCPSocket](https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/TCPSocket.h).
- [TLSSocket](https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/TLSSocket.h).
- [DNS](https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/DNS.h)

Reference documentation: https://os.mbed.com/docs/latest/reference/network-socket.html

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Feb 6, 2019

Contributor

OK, not part of your changes but I guess this should be changed to https://os.mbed.com/docs/mbed-os/latest/apis/network-socket.html . And maybe also convert it to proper link named as "Network Socket Documentation" - or something similar

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 6, 2019

Contributor

Yes! Please fix the link.

TESTS/network/interface/README.md Outdated
Test environment
----------------

- [Greentea](https://github.com/ARMmbed/greentea)

This comment has been minimized.

Copy link
@VeijoPesonen

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 6, 2019

Contributor

Perhaps link to this https://os.mbed.com/docs/mbed-os/v5.11/tools/greentea-testing-applications.html

Replace "v5.11" with "latest"

TESTS/network/wifi/README.md Outdated
- Channels to be used must be different for both APs. For secure on channel number is later referred as `<ch:secure>` and for unsecure on `<ch:unsecure>`
- MAC addresses of Wifi APs must be known. Later referred as `<mac:secure>` and `<mac:unsecure>`

**NOTE:�**Echo server is referred here as it is a requirement for running Socket API tests. It is not directly used by test cases defined in this document.

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Feb 6, 2019

Contributor

Does not render properly

TESTS/network/wifi/README.md Outdated

**Preconditions:**

1. -

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Feb 6, 2019

Contributor

Looks bad.

TESTS/network/wifi/README.md Outdated

This test case is to test whether the driver accepts valid credentials and reject ones that are not valid.

**�Preconditions:**

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Feb 6, 2019

Contributor

Does not render correctly.

@VeijoPesonen

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

@melwee01 would you also like to take a look if you have time?

TESTS/network/interface/README.md Outdated
----------------

- Mbed OS.
- Standard Mbed OS development tools as described in https://os.mbed.com/docs/latest/tools/index.html.

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Feb 6, 2019

Contributor

Could be turned into a proper link like on line 9

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:publish_documentation branch Feb 6, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

Thanks for your feedback, @VeijoPesonen . Alle remarks fixed.
Is there some smart way to see the rendered output of the modified files? I am locally using a Chrome "Markdown Preview Plus" plugin, but I can see it renders differently from github. I am especially worried about the tables...

@SeppoTakalo
Copy link
Contributor

left a comment

Small changes requested.

TESTS/netsocket/README.md Outdated
@@ -1678,3 +1693,308 @@ Subset for driver test
### For socket layer driver (AT-driven, external IP stack):

All Socket, UDPSocket, TCPSocket and TLSSocket testcases.

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 6, 2019

Contributor

Move these before the "Subset for driver test" section.

Otherwise the formatting seems OK.

TESTS/netsocket/README.md Outdated
@@ -18,6 +18,7 @@ The target for this plan is to test:
- [UDPSocket](https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/UDPSocket.h).
- [TCPSocket](https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/TCPSocket.h).
- [TLSSocket](https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/TLSSocket.h).
- [DNS](https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/DNS.h)

Reference documentation: https://os.mbed.com/docs/latest/reference/network-socket.html

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 6, 2019

Contributor

Yes! Please fix the link.

TESTS/network/interface/README.md Outdated
Test environment
----------------

- [Greentea](https://github.com/ARMmbed/greentea)

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 6, 2019

Contributor

Perhaps link to this https://os.mbed.com/docs/mbed-os/v5.11/tools/greentea-testing-applications.html

Replace "v5.11" with "latest"

TESTS/network/wifi/README.md Outdated
| 5 | WIFI_GET_RSSI | | SHOULD |
| 6 | WIFI_CONNECT_PARAMS_NULL | | MUST |
| 7 | WIFI_CONNECT_PARAMS_VALID_UNSECURE | | MUST |
| 8 | WIFI_CONNECT_PARAMS_VALID_SECURE | With secruity type: | |

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 6, 2019

Contributor

secruity change to security

TESTS/network/wifi/README.md Outdated

**Preconditions:**

1. -

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 6, 2019

Contributor

1. - renders to

1.
    o

So use 1. none instead.

TESTS/network/wifi/README.md Outdated
| ssid=NULL | pass=NULL | security= NSAPI_SECURITY_NONE | NSAPI_ERROR_PARAMETER |
| ssid="OK" | pass=NULL | security=NSAPI_SECURITY_WPA_WPA2 | NSAPI_ERROR_PARAMETER |
| ssid="OK" | pass=NULL | security=NSAPI_SECURITY_WEP | NSAPI_ERROR_PARAMETER |
| | | | because password is missing. |

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 6, 2019

Contributor

These two line boxes are not rendering very well.
Just combine the content into one box, like | NSAPI_ERROR_PARAMETER because password is missing |

Markdown will then split the lines.

Or you can try <br /> HTML code to split the lines manually.

TESTS/network/wifi/README.md Outdated
First call should return zero, second call should return negative number between -10 and -100.

**
**

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 6, 2019

Contributor

What are these extra markings? Remove...

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Feb 6, 2019

Author Contributor

Probably some artifacts from pandoc...

TESTS/network/wifi/README.md Outdated
`scan()` returns positive number that is higher or equivalent of two. (>=2).

**
**

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 6, 2019

Contributor

Remove these extra markings.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:publish_documentation branch Feb 6, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

Thanks, @SeppoTakalo , remarks fixed.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:publish_documentation branch Feb 6, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Not all remarks..

Change the order like I proposed earlier..

First DNS testcases, then "Subset for driver ...."

TESTS/network/wifi/README.md Outdated

**Expected result:**

| Parameters | | | Expected return code |

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 6, 2019

Contributor

Can we change this table to
|ssid|password|security| Expected return code|
|----|---------|--------|-----------------------|

The we can remove those ssid= and pass= which fill up the table and make it harder to read.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

I hope now I got it right...
This...

### For socket layer driver (AT-driven, external IP stack):

All Socket, UDPSocket, TCPSocket and TLSSocket testcases.

... part belongs to the sockets, right? (So it should go before the DNS testcases)? Or should we add DNS to the list and move it down to the end of document?

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:publish_documentation branch Feb 6, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

No, that part belongs to bottom of the file as it was in the Confluence.

No we are just appending Socket and DNS test cases together so that it is a long list of testcases that live within this directory.

Then after the testcase listing. there would be subsets proposed.

@VeijoPesonen

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Thanks for your feedback, @VeijoPesonen . Alle remarks fixed.
Is there some smart way to see the rendered output of the modified files? I am locally using a Chrome "Markdown Preview Plus" plugin, but I can see it renders differently from github. I am especially worried about the tables...

One way is to just create a new branch to your fork and check the end result from your browser. Nothing better comes to my mind. Once the changes have already landed to a PR just use the "View File"-button.


Reference documentation: https://os.mbed.com/docs/latest/reference/network-socket.html
See [Network Socket Documentation](https://os.mbed.com/docs/mbed-os/latest/apis/network-socket.html) for reference.

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Feb 6, 2019

Contributor

Would you please do the same for the line 29.

Publish tests documentation for DNS, Emac, NetworkInterface and Wifi
Documentation moved from internal Confluence pages.
Edit README.md
Edit file, mostly for inclusion of articles.
@VeijoPesonen

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

I would be happy to use a template that says **Preconditions:** as there can be zero or more conditions.

Does not much change the message if we tailor the template per test case for titles to be plural or singular.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

@0xc0170 , is there any other approval required here? @ARMmbed/mbed-docs ?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Yes, @AnotherButler to approve. I can schedule CI now as it was already edited by docs team

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Feb 11, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

CI started

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Just to confirm, this update is targetting 5.11.x ?

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Not sure. @SeppoTakalo , @TuomoHautamaki , can you help us here? Should this documentation publishing go into 5.11.x?

@0xc0170 0xc0170 added needs: review and removed needs: CI labels Feb 11, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

CI completed, waiting for docs to approve and ready for merge

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

It does not matter whether it goes out in 5.11 or 5.12 as we always link into the master branch.

Basically I think that it can be merged in the 5.11 branch, because it describes existing test cases. But there is no code changes, so I don't see the point.

@cmonr

cmonr approved these changes Feb 11, 2019

@cmonr cmonr added ready for merge and removed needs: review labels Feb 11, 2019

@cmonr cmonr merged commit 799476d into ARMmbed:master Feb 12, 2019

15 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10476 cycles (+419 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 12, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

@ARMmbed/mbed-docs Could you please still review this and mark any comments so that they can come into a follow on PR - this got prematurely merged...

@SeppoTakalo SeppoTakalo deleted the michalpasztamobica:publish_documentation branch Feb 12, 2019

@AnotherButler
Copy link
Contributor

left a comment

I requested several global edits because I didn't want to paste them every time.


**Expected result:**

Return value is NSAPI_ERROR_DNS_FAILURE for first, third and fifth host name. Return value is NSAPI_ERROR_OK and IP address is obtained for second and fourth host name.

This comment has been minimized.

Copy link
@AnotherButler

AnotherButler Feb 12, 2019

Contributor

Please add code formatting. I'm not sure what this means.


**Test steps:**

1. Call `gethostbyname()` in a row 6 times with a different host names. Host names shall not be found from cache. First, third and fifth host name shall be invalid.

This comment has been minimized.

Copy link
@AnotherButler

AnotherButler Feb 12, 2019

Contributor

"...with different host names."

Also, I'm not sure what the rest of this line means.


**Preconditions:**

1. Network interface is initialised.

This comment has been minimized.

Copy link
@AnotherButler

AnotherButler Feb 12, 2019

Contributor

"initialized"


**Description:**

Verify that DNS failure error is provided for invalid hosts. Call `NetworkInterface::gethostbyname()` in a row 6 times with a different host names. First, third and fifth host name shall be invalid.

This comment has been minimized.

Copy link
@AnotherButler

AnotherButler Feb 12, 2019

Contributor

"...with different host names."

I'm not sure what the rest of this line means.


**Expected result:**

Return value is NSAPI_ERROR_OK and IP address is obtained from the function call 5 times. First request shall complete slower than the requests made after it (where the response is found from cache).

This comment has been minimized.

Copy link
@AnotherButler

AnotherButler Feb 12, 2019

Contributor

Please add code formatting.

"slower" -> "more slowly".

**Test steps:**

1. Call `gethostbyname_async()` in a row 6 times with a different host names. Host names shall not be found from cache.
2. Verify that last `gethostbyname_async()` operation is rejected since there is room only for 5 simultaneous operations.

This comment has been minimized.

Copy link
@AnotherButler

AnotherButler Feb 12, 2019

Contributor

"since" -> "because"


**Description:**

Verify that the caching of DNS results works correctly with simultaneous asynchronous DNS queries. Call `NetworkInterface::gethostbyname_async()` in a row 6 times with a different host names. Wait for all requests to complete and verify result. Cache shall contain at least one host name used in asynchronous request. This can be achieved e.g. by running test "Asynchronous DNS simultaneous" before this test and using same host names in this run.

This comment has been minimized.

Copy link
@AnotherButler

AnotherButler Feb 12, 2019

Contributor

"in asynchronous request. You can achieve this, for example, by running ..."


**Description:**

Verify that non-asynchronous i.e. blocking DNS queries and asynchronous i.e. non-blocking queries work at the same time. Call `NetworkInterface::gethostbyname_async()`. Right after that make 6 non-asynchronous `NetworkInterface::gethostbyname()` calls with different host names.

This comment has been minimized.

Copy link
@AnotherButler

AnotherButler Feb 12, 2019

Contributor

"Verify nonasnchronous (in other words, blocking) DNS queries and asynchronous (in other words, nonblocking) queries ... after that, make 6 nonasynchronous..."


**Description:**

Verify that asynchronous DNS query cancel works correctly. Call `NetworkInterface::gethostbyname_async()` in a row 6 times with a different host names. Cache shall contain 3 host names used in requests. This can be achieved e.g. by running test "Asynchronous DNS non-asynchronous and asynchronous" before this test and using same host names in this run. For each request that was given an unique id, call cancel. Verify that callback is not called for cancelled requests.

This comment has been minimized.

Copy link
@AnotherButler

AnotherButler Feb 12, 2019

Contributor

Please globally delete the hyphen in "nonasynchronous".
Please globally capitalize "ID".
Please globally delete the extra "l" in "canceled".

1. Call `gethostbyname_async()` in a row 6 times with a different host names. Cache shall contain in maximum 3 host names used in requests.
2. Call `gethostbyname_async_cancel()` for each request that was given an unique id.
3. Verify that for cancelled requests, callback is not called.
4. Verify that for other request, callback is called.

This comment has been minimized.

Copy link
@AnotherButler

AnotherButler Feb 12, 2019

Contributor

"requests"

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

@SeppoTakalo @michalpasztamobica @ARMmbed/mbed-os-ipcore Would someone mind following up with the above suggested doc edits in a new PR?

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

@AnotherButler , many thanks for your thorough review. I hope I addressed all remarks in #9698.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.