Unit tests for TCPSocket, TCPServer, UDPSocket, NetworkStack, InternetSocket #8138

michalpasztamobica · 2018-09-14T13:22:32Z

Description

Added unit tests for the following classes TCPSocket, TCPServer, UDPSocket, NetworkStack, InternetSocket.
Apart from the improved coverage (both functional and coverage) some changes were made in stubs to improve their functionality.

Pull request type

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

michalpasztamobica · 2018-09-14T13:34:10Z

@SeppoTakalo please review

VeijoPesonen · 2018-09-18T07:45:10Z

UNITTESTS/features/netsocket/TCPSocket/test_TCPSocket.cpp

+
+TEST_F(TestTCPSocket, constructor_parameters)
+{
+    TCPSocket socketParam = TCPSocket(dynamic_cast<NetworkStack*>(&stack));


As I'm not that experienced with C++ I would like to know why explicit cast is needed here?

To get a runtime exception if casting is not possible.

For example, you modify your stub so that it is not actually inheriting NetworkStack anymore. Dynamic casting catches this error, but static casting does not. It might cause then sefgault.

How does this work with RTTI disabled , or UNITTests have RTTI enabled?

RTTI is by default enabled in GCC. It can be disabled with -fno-rtti flag, but I do not see it used anywhere in the UNITTESTS directory or in the build files, so I assume it remains enabled.

VeijoPesonen · 2018-09-18T07:55:34Z

UNITTESTS/stubs/equeue_stub.c

@@ -19,10 +19,17 @@

 void *equeue_alloc(equeue_t *q, size_t size)
 {
+    return (void *)malloc(size);


Unnecessary cast.

cmonr · 2018-09-20T18:22:58Z

@SeppoTakalo @michalpasztamobica Would y'all mind answering @VeijoPesonen's questions?

Also, this needs a rebase.

SeppoTakalo · 2018-09-21T10:45:40Z

@cmonr Rebased and fixed the commented issues.

0xc0170 · 2018-09-21T11:00:32Z

jenkins-ci/unittests — Test job: unstable

@OPpuolitaival Please review

SeppoTakalo · 2018-09-21T11:01:53Z

NO.

Don't review..

Unittest were failing, but because #8182 is missing from the master, I could not run it on Mac OS X.

~~Should be fixed now.~~ Actually still failing and 8182 did not fix unittests for me, so I need to continue on this on Monday.

Add functional and line coverage for UDPSocket and TCPSocket. The EventFlagsstub and NetworkStackstub classes are allowed to store multiple return values to allow running internal loops multiple times.

Added more tests, improved the existing ones. setblocking tests were not checking anything, so they were removed and these functions are called in TCPSocket tests instead.

Most functions are empty or simply return "UNSUPPORTED", but it is still worth covering this functions with unit tests to have better control of unwanted changes.

TCPServer class only really implements attach method.

Improved the stubs for event queue and nsapi_dns, to allow checking if callback are handled correctly. This involves some memory allocation and deallocation. The NetworkStackWrapper is not covered as it seems to be deprecated code.

accept() is not anymore returning NULL pointer. It was a bug.

SeppoTakalo · 2018-09-21T11:35:51Z

Actually the #8182 DID fix unittesting in Mac OS X, I just did not pick both commits from it..
However, should be fixed now.

cmonr · 2018-09-22T02:32:50Z

Progress when able! #8182 is now in.

SeppoTakalo · 2018-09-22T07:09:04Z

@cmonr this is already done. #8182 was needed for OS X, but our CI is Ubuntu, so this already passed tests.

cmonr · 2018-09-22T21:29:09Z

@SeppoTakalo You're referring to jenkins-ci/unittests — Test job: successful, correct?

@ARMmbed/mbed-os-maintainers
@ARMmbed/mbed-os-test
Are we ok with bypassing Jenkins CI with this PR, since it only contains unittest changes?

I'm aware that this PR doesn't interact with any of the CI tests, but asking to skip CI opens up the question for other to ask for the favor as well. I'd be considerably more comfortable skipping CI if we had some sort of explicit list detailing exactly when and how we would skip CI. Docs are a good contender, as well as unit tests.

0xc0170 · 2018-09-24T05:21:33Z

We got a ticket to cover this and should be resolved soon. Meanwhile

/morph build

mbed-ci · 2018-09-24T06:45:33Z

Build : SUCCESS

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

Triggering tests

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

OPpuolitaival · 2018-09-24T06:46:24Z

@lorjala please review

mbed-ci · 2018-09-24T10:46:07Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-09-24T17:07:20Z

Test : FAILURE

Build number : 2950
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8138/2950

SeppoTakalo · 2018-09-25T11:15:56Z

@0xc0170 Needs re-testing.
Unrelated failure.

cmonr · 2018-09-25T15:25:20Z

@SeppoTakalo Correct. We're working on getting the unittest PR in first before we restart other PRs.

cmonr · 2018-09-25T15:43:03Z

/morph test

mbed-ci · 2018-09-25T22:27:59Z

Test : SUCCESS

Build number : 2965
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8138/2965

cmonr added the release-version: 5.10.1 label Sep 14, 2018

cmonr requested a review from SeppoTakalo September 14, 2018 14:08

cmonr added the needs: review label Sep 14, 2018

0xc0170 requested a review from a team September 17, 2018 11:39

SeppoTakalo force-pushed the master branch from 777711b to 84ba01b Compare September 17, 2018 11:59

SeppoTakalo approved these changes Sep 17, 2018

View reviewed changes

VeijoPesonen reviewed Sep 18, 2018

View reviewed changes

VeijoPesonen approved these changes Sep 18, 2018

View reviewed changes

cmonr added needs: work and removed needs: review labels Sep 20, 2018

SeppoTakalo force-pushed the master branch from 4e614a7 to a1ef7ab Compare September 21, 2018 10:44

SeppoTakalo force-pushed the master branch from a1ef7ab to 027d897 Compare September 21, 2018 10:53

michalpasztamobica and others added 7 commits September 21, 2018 14:23

unittests: improved coverage for UDPSocket and TCPSocket

5a74481

Add functional and line coverage for UDPSocket and TCPSocket. The EventFlagsstub and NetworkStackstub classes are allowed to store multiple return values to allow running internal loops multiple times.

unittests: Check NULL pointer scenario in TCPSocket::recvfrom

e609e2f

unittests: InternetSocket class coverage improved.

d740d69

Added more tests, improved the existing ones. setblocking tests were not checking anything, so they were removed and these functions are called in TCPSocket tests instead.

unittests: Added NetworkInterface unit tests

51dedfd

Most functions are empty or simply return "UNSUPPORTED", but it is still worth covering this functions with unit tests to have better control of unwanted changes.

unittests: Add TCPServer unit tests

aa90f5b

TCPServer class only really implements attach method.

unittests: Add tests for NetworkStack class.

72233b0

Improved the stubs for event queue and nsapi_dns, to allow checking if callback are handled correctly. This involves some memory allocation and deallocation. The NetworkStackWrapper is not covered as it seems to be deprecated code.

Fix TCPSocket::accept() unittest.

fa7547a

accept() is not anymore returning NULL pointer. It was a bug.

SeppoTakalo force-pushed the master branch 2 times, most recently from d9f586d to 995e30d Compare September 21, 2018 11:31

Use SocketAddr that actually has IP bytes in UDP test

b98e6b6

SeppoTakalo force-pushed the master branch from 995e30d to b98e6b6 Compare September 21, 2018 11:35

cmonr added the needs: preceding PR label Sep 21, 2018

0xc0170 approved these changes Sep 21, 2018

View reviewed changes

cmonr removed the needs: preceding PR label Sep 22, 2018

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

cmonr mentioned this pull request Sep 22, 2018

Unit tests: add code coverage filtering #8154

Merged

OPpuolitaival approved these changes Sep 24, 2018

View reviewed changes

0xc0170 requested a review from lorjala September 24, 2018 06:59

lorjala approved these changes Sep 24, 2018

View reviewed changes

cmonr added ready for merge and removed needs: CI labels Sep 26, 2018

0xc0170 merged commit 4957dd5 into ARMmbed:master Sep 26, 2018

0xc0170 removed the ready for merge label Sep 26, 2018

Unit tests for TCPSocket, TCPServer, UDPSocket, NetworkStack, InternetSocket #8138

Unit tests for TCPSocket, TCPServer, UDPSocket, NetworkStack, InternetSocket #8138

Uh oh!

Conversation

michalpasztamobica commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

michalpasztamobica commented Sep 14, 2018

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!

cmonr commented Sep 20, 2018

Uh oh!

SeppoTakalo commented Sep 21, 2018

Uh oh!

0xc0170 commented Sep 21, 2018

Uh oh!

SeppoTakalo commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SeppoTakalo commented Sep 21, 2018

Uh oh!

cmonr commented Sep 22, 2018

Uh oh!

SeppoTakalo commented Sep 22, 2018

Uh oh!

cmonr commented Sep 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Sep 24, 2018

Uh oh!

mbed-ci commented Sep 24, 2018

Triggering tests

Uh oh!

OPpuolitaival commented Sep 24, 2018

Uh oh!

mbed-ci commented Sep 24, 2018

Uh oh!

mbed-ci commented Sep 24, 2018

Uh oh!

SeppoTakalo commented Sep 25, 2018

Uh oh!

cmonr commented Sep 25, 2018

Uh oh!

cmonr commented Sep 25, 2018

Uh oh!

mbed-ci commented Sep 25, 2018

Uh oh!

Uh oh!

michalpasztamobica commented Sep 14, 2018 •

edited

Loading

SeppoTakalo commented Sep 21, 2018 •

edited

Loading

cmonr commented Sep 22, 2018 •

edited

Loading