Improve API documentation and test coverage #11878

michalpasztamobica · 2019-11-18T08:53:01Z

Description (required)

Harden netsocket APIs in documentation and unittests.
Continuation of the task started in #11808.

Summary of change (What the change is for and why)

I've gone through the Doxygen docs of our APIs and tried to make sure all returnable errors are explicitly mentioned and references are given to underlying APIs, if they can return other errors. Then I went to check if existing unittests cover all the possible return values.

The goal is for the users to be able to only check returnable errors, rather than go through all possible nsapi_error_t values.

The only untested APIs are:

L3IP - It includes LWIPStack.h and basically requires full LWIP configuration - quite an overkill for testing what is effectively a wrapper class.
fully abstract classes (like DNS, WiFiInterface, MeshInterface) - as they have no code to test. It is also difficult to predict possible return values, as they may depend on the underlying driver implementation.
I assumed cellular APIs are under control and supervision of cellular team and did not touch those.

There is one functional change in the return value of PPPInterface function. Reviewers, please pay attention 😃 .

I hae also moved OnboardNetworkStackMock function it its own file and explicitly called the file a _mock. I hope this is OK? (Alternatively, we could create a mocks directory, next to the existing stubs, or call the file a _stub, even though it contains a mock).

Documentation (Details of any document updates required)

Pull request type (required)

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results (required)

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Reviewers (optional)

@AnttiKauppila

ciarmcom · 2019-11-18T10:00:32Z

@michalpasztamobica, thank you for your changes.
@AnttiKauppila @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

michalpasztamobica · 2019-11-18T12:13:28Z

I fixed the doxyspell error and removed one of the _mock files, which I realised wasn't used anywhere in the end.

michalpasztamobica · 2019-11-19T14:12:59Z

@AnttiKauppila , could you review, please?

bulislaw · 2019-11-19T15:05:54Z

As it stands it's a breaking change and should target 6.

0xc0170 · 2019-11-21T14:57:48Z

features/netsocket/PPPInterface.cpp

@@ -80,7 +80,7 @@ nsapi_error_t PPPInterface::disconnect()
    if (_interface) {
        return _interface->bringdown();
    }
-    return NSAPI_ERROR_OK;
+    return NSAPI_ERROR_NO_CONNECTION;


This functional change might need more than just Fix PPPInterface::disconnect to return NO_CONNECTION or is it that clear ?

Thanks for the remark, @0xc0170 . I amended the commit message, to make sure the reason for this change is clear. Is this ok now?

I think that this code must have never been triggered until I added unittests for it, just now. It's such an obvious bug to return OK if there is no interface...

61d0ac8 is still the same or is it different commit?

Doesn't the hash of the commit change when amended? The modification looks the same...

I looked through the changes before and after the recent push and they are identical, only the commit message changed now.

Fix PPPInterface::disconnect to return NO_CONNECTION in case of failure - this is the new commit msg then?

Yes with "in case of failure" added to clarify what has exactly changed.
Did I misunderstand your request? :)

0xc0170 · 2019-11-22T08:05:19Z

Will start CI soon, some 5.15 needs to get in first today

0xc0170 · 2019-11-22T10:24:45Z

CI started

mbed-ci · 2019-11-22T11:23:30Z

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

0xc0170 · 2019-11-22T14:29:35Z

@michalpasztamobica Please rebase

michalpasztamobica · 2019-11-22T15:36:11Z

@0xc0170 , rebased, resolved conflicts and fixed uninitialized variable that caused valgrind to crash.

0xc0170 · 2019-11-25T15:22:45Z

CI started

mbed-ci · 2019-11-25T16:02:33Z

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

michalpasztamobica · 2019-11-26T05:27:54Z

@0xc0170 , I rebased and fixed the unittest compilation issue (one of the stubs was renamed form .c to .cpp while the PR was being processed). The CI can be restarted.

0xc0170 · 2019-11-26T09:04:19Z

Will start CI when later today

0xc0170 · 2019-11-28T15:18:29Z

I triggered CI but right away noticed the conflicts :/ Please rebase and we shall restart CI

michalpasztamobica · 2019-11-29T09:05:24Z

@0xc0170 , I just rebased and resolved conflicts.

0xc0170 · 2019-12-02T15:55:28Z

CI restarted

mbed-ci · 2019-12-02T16:44:23Z

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

The mock was only used by EthernetInterface tests, but we can reuse it for other tests as well.

Mainly focusing on hardening the tests for return values.

michalpasztamobica · 2019-12-02T18:49:31Z

Logs are not fully clear to me, but there was an uninitialized variable in one of the newly added tests, which valgrind detected.
Fixed now, @0xc0170 , would you please restart the CI?

0xc0170 · 2019-12-03T12:22:14Z

CI restarted

mbed-ci · 2019-12-03T15:51:40Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 5
Build artifacts

ciarmcom requested review from AnttiKauppila and a team November 18, 2019 10:00

ciarmcom added the needs: review label Nov 18, 2019

michalpasztamobica force-pushed the api_hardening branch 2 times, most recently from 68e0336 to 1f5f514 Compare November 18, 2019 12:12

AnttiKauppila approved these changes Nov 21, 2019

View reviewed changes

AnttiKauppila added needs: CI release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed needs: review labels Nov 21, 2019

0xc0170 requested a review from AnotherButler November 21, 2019 14:56

0xc0170 reviewed Nov 21, 2019

View reviewed changes

michalpasztamobica force-pushed the api_hardening branch from 1f5f514 to 539cff8 Compare November 21, 2019 15:03

0xc0170 added needs: work and removed needs: CI labels Nov 22, 2019

michalpasztamobica force-pushed the api_hardening branch from 539cff8 to b7e50ed Compare November 22, 2019 15:35

0xc0170 added needs: CI and removed needs: work labels Nov 25, 2019

michalpasztamobica force-pushed the api_hardening branch from b7e50ed to a9f5319 Compare November 26, 2019 05:26

michalpasztamobica mentioned this pull request Nov 28, 2019

Remove string-based API usage within mbed-os #11941

Merged

michalpasztamobica force-pushed the api_hardening branch from a9f5319 to 67e60b9 Compare November 29, 2019 09:04

michalpasztamobica added 4 commits December 2, 2019 20:47

Fix PPPInterface::disconnect to return NO_CONNECTION in case of failure

33096c2

Move OnboardNetworkStackMock to a separate file

546fdd2

The mock was only used by EthernetInterface tests, but we can reuse it for other tests as well.

Improve doxygen and unittest coverage for API

caf0147

Mainly focusing on hardening the tests for return values.

Adjust mbed_assert_stub file extension to .cpp

75472d0

michalpasztamobica force-pushed the api_hardening branch from 67e60b9 to 75472d0 Compare December 2, 2019 18:48

0xc0170 removed the needs: CI label Dec 4, 2019

0xc0170 merged commit 4ae8984 into ARMmbed:master Dec 4, 2019

michalpasztamobica deleted the api_hardening branch December 4, 2019 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve API documentation and test coverage #11878

Improve API documentation and test coverage #11878

michalpasztamobica commented Nov 18, 2019 •

edited

Loading

ciarmcom commented Nov 18, 2019

michalpasztamobica commented Nov 18, 2019

michalpasztamobica commented Nov 19, 2019

bulislaw commented Nov 19, 2019

0xc0170 Nov 21, 2019

michalpasztamobica Nov 21, 2019

0xc0170 Nov 21, 2019

michalpasztamobica Nov 21, 2019

michalpasztamobica Nov 21, 2019

0xc0170 Nov 22, 2019

michalpasztamobica Nov 25, 2019

0xc0170 commented Nov 22, 2019

0xc0170 commented Nov 22, 2019

mbed-ci commented Nov 22, 2019

0xc0170 commented Nov 22, 2019

michalpasztamobica commented Nov 22, 2019

0xc0170 commented Nov 25, 2019

mbed-ci commented Nov 25, 2019

michalpasztamobica commented Nov 26, 2019

0xc0170 commented Nov 26, 2019

0xc0170 commented Nov 28, 2019

michalpasztamobica commented Nov 29, 2019

0xc0170 commented Dec 2, 2019

mbed-ci commented Dec 2, 2019

michalpasztamobica commented Dec 2, 2019

0xc0170 commented Dec 3, 2019

mbed-ci commented Dec 3, 2019

Improve API documentation and test coverage #11878

Improve API documentation and test coverage #11878

Conversation

michalpasztamobica commented Nov 18, 2019 • edited Loading

Description (required)

Summary of change (What the change is for and why)

Documentation (Details of any document updates required)

Pull request type (required)

Test results (required)

Reviewers (optional)

ciarmcom commented Nov 18, 2019

michalpasztamobica commented Nov 18, 2019

michalpasztamobica commented Nov 19, 2019

bulislaw commented Nov 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Nov 22, 2019

0xc0170 commented Nov 22, 2019

mbed-ci commented Nov 22, 2019

0xc0170 commented Nov 22, 2019

michalpasztamobica commented Nov 22, 2019

0xc0170 commented Nov 25, 2019

mbed-ci commented Nov 25, 2019

michalpasztamobica commented Nov 26, 2019

0xc0170 commented Nov 26, 2019

0xc0170 commented Nov 28, 2019

michalpasztamobica commented Nov 29, 2019

0xc0170 commented Dec 2, 2019

mbed-ci commented Dec 2, 2019

michalpasztamobica commented Dec 2, 2019

0xc0170 commented Dec 3, 2019

mbed-ci commented Dec 3, 2019

michalpasztamobica commented Nov 18, 2019 •

edited

Loading