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

Reduce greentea socket tests failures related to network issues #10330

Merged
merged 2 commits into from Apr 9, 2019

Conversation

Projects
None yet
7 participants
@michalpasztamobica
Copy link
Contributor

commented Apr 5, 2019

Description

Netsocket greentea tests are meant to test Socket API. However they highly depend on the quality of the underlying network connection, especially when it comes to using the echo server.
I tried to identify the most common and fixable failures, which seem to result from the underlying infrastructure rather than the socket operation itself. These failures obscure the overall view of the CI results and might in result hide some actual errors.

The first improvement is in UDPSOCKET_RECV_TIMEOUT. The test was checking that at least 5 out of 10 UDP packets were received from the echo server. I do not see how this is related to socket timeout. We have separate tests UDPSOCKET_ECHO* for UDP sockets to test echo responses and that test can handle WOULD_BLOCK responses in a more reasonable way and will also check if the data in echo responses match.
I suggest we remove the packet success ratio expectation from this test to avoid random failures due to network glitches.

The second potential improvement is in TLSSOCKET_RECV_TIMEOUT. Here we sometimes fail to receive a packet within 20 seconds, especially in the tests using wifi, which we know is rather unreliable. There is a separate mechanism in this test, which guarantees that we will not exceed half of the total time for the test suite, so increasing the sigio timeout is safe and might reduce the number of failures due to network issues.

Pull request type

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

Reviewers

@SeppoTakalo
@VeijoPesonen
@KariHaapalehto
@mtomczykmobica
@tymoteuszblochmobica

@ciarmcom ciarmcom requested review from KariHaapalehto, mtomczykmobica, SeppoTakalo, tymoteuszblochmobica, VeijoPesonen and ARMmbed/mbed-os-maintainers Apr 5, 2019

@0xc0170

0xc0170 approved these changes Apr 8, 2019

@@ -859,9 +859,6 @@ Within each loop, one `recvfrom()` may return the received packet size
When `NSAPI_ERROR_WOULD_BLOCK` is received, check that time consumed is
more that 100 milliseconds but less than 200 milliseconds.

After repeating for 10 times, at least 5 packets must have been
received.

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Apr 8, 2019

Contributor

50 % packet loss is quite a big.
If there is 100 % packet loss, and we accept it, how is this going to test anything?

@SeppoTakalo
Copy link
Contributor

left a comment

I can approve, but if we drop the number of packets away, we might also drop the number of loops to two at least.
Would run much faster and still test what was the idea of the test case.

Show resolved Hide resolved TESTS/netsocket/udp/udpsocket_recv_timeout.cpp Outdated

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:greentea_test_fixes branch from 4bef448 to 872edd4 Apr 8, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Ci started

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Apr 8, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 8, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

CI job restarted: jenkins-ci/greentea-test

K66F issues don't appear to be related to PR.

@cmonr cmonr added ready for merge and removed needs: CI labels Apr 8, 2019

@0xc0170 0xc0170 merged commit af3a765 into ARMmbed:master Apr 9, 2019

21 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/greentea-test Success
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 9864 cycles (-147 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

@0xc0170 0xc0170 removed the ready for merge label Apr 9, 2019

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.