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

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

Merged
merged 8 commits into from Sep 26, 2018

Conversation

Projects
None yet
8 participants
@michalpasztamobica
Contributor

michalpasztamobica commented Sep 14, 2018

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

This comment has been minimized.

Contributor

michalpasztamobica commented Sep 14, 2018

@SeppoTakalo please review

@cmonr cmonr requested a review from SeppoTakalo Sep 14, 2018

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

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-ipcore Sep 17, 2018

@SeppoTakalo SeppoTakalo force-pushed the michalpasztamobica:master branch from 777711b to 84ba01b Sep 17, 2018

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

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Sep 18, 2018

Contributor

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

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Sep 21, 2018

Contributor

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.

This comment has been minimized.

@0xc0170

0xc0170 Sep 21, 2018

Member

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

This comment has been minimized.

@michalpasztamobica

michalpasztamobica Oct 1, 2018

Contributor

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.

@@ -19,10 +19,17 @@
void *equeue_alloc(equeue_t *q, size_t size)
{
return (void *)malloc(size);

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Sep 18, 2018

Contributor

Unnecessary cast.

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 20, 2018

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

Also, this needs a rebase.

@SeppoTakalo SeppoTakalo force-pushed the michalpasztamobica:master branch from 4e614a7 to a1ef7ab Sep 21, 2018

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 21, 2018

@cmonr Rebased and fixed the commented issues.

@SeppoTakalo SeppoTakalo force-pushed the michalpasztamobica:master branch from a1ef7ab to 027d897 Sep 21, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 21, 2018

jenkins-ci/unittests — Test job: unstable

@OPpuolitaival Please review

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 21, 2018

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.

michalpasztamobica and others added some commits Sep 12, 2018

unittests: improved coverage for UDPSocket and TCPSocket
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: InternetSocket class coverage improved.
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
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
TCPServer class only really implements attach method.
unittests: Add tests for NetworkStack class.
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.
accept() is not anymore returning NULL pointer. It was a bug.

@SeppoTakalo SeppoTakalo force-pushed the michalpasztamobica:master branch 2 times, most recently from d9f586d to 995e30d Sep 21, 2018

@SeppoTakalo SeppoTakalo force-pushed the michalpasztamobica:master branch from 995e30d to b98e6b6 Sep 21, 2018

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 21, 2018

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

This comment has been minimized.

Contributor

cmonr commented Sep 22, 2018

Progress when able! #8182 is now in.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 22, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 22, 2018

@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

This comment has been minimized.

Member

0xc0170 commented Sep 24, 2018

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

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 24, 2018

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

This comment has been minimized.

Contributor

OPpuolitaival commented Sep 24, 2018

@lorjala please review

@0xc0170 0xc0170 requested a review from lorjala Sep 24, 2018

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 25, 2018

@0xc0170 Needs re-testing.
Unrelated failure.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 25, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 25, 2018

/morph test

@mbed-ci

This comment has been minimized.

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

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

15 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
jenkins-ci/unittests Test job: successful
Details
travis-ci/astyle Passed, 620 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9928 cycles (+772 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