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

Add TLSSocket greentea tests. #9283

Merged
merged 2 commits into from Jan 30, 2019

Conversation

@michalpasztamobica
Copy link
Contributor

michalpasztamobica commented Jan 8, 2019

Description

TLSSocket tests based on existing TCPSocket greentea tests and TLSSocket icetea tests.
The tests are only passing with "rtos.main-thread-stack-size": 8192 (6144 if we take out the SIMULTANEOUS test).
HTTPS_GET test is commented as it fails, perhaps due to connectivity reasons - I don't want to break the CI.

Pull request type

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

Reviewers

@SeppoTakalo
@KariHaapalehto
@VeijoPesonen
@mtomczykmobica

@ciarmcom ciarmcom requested review from KariHaapalehto , mtomczykmobica , SeppoTakalo , VeijoPesonen and ARMmbed/mbed-os-maintainers Jan 8, 2019

@ciarmcom

This comment has been minimized.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_greentea branch Jan 8, 2019

@mtomczykmobica
Copy link
Contributor

mtomczykmobica left a comment

Looks good.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_greentea branch Jan 8, 2019

@SeppoTakalo
Copy link
Contributor

SeppoTakalo left a comment

Just a minor changes proposed.

Otherwise looks good.

TESTS/netsocket/tls/tlssocket_echotest_burst.cpp Outdated
}

if (bt_left != 0) {
drop_bad_packets(*sock, 0);

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 8, 2019

Contributor

This can only happen if recv() returns error and if it does, we should not try to read again.

I prefer this drop_bad_packets() to be removed. Just mark the test failed (with error message, if possible).

Show resolved Hide resolved TESTS/netsocket/tls/main.cpp
TESTS/netsocket/tls/main.cpp Outdated
{
SocketAddress tls_addr;

get_interface()->gethostbyname(MBED_CONF_APP_ECHO_SERVER_ADDR, &tls_addr);

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 8, 2019

Contributor

Probably a minor thing, but now that we use NetworkInterface::get_default_instance() can we refactor all these get_interface() call to NetworkInterface::get_default_instance()->

I feel like get_interface() is also a residue from time when default network interface did not exist

TESTS/netsocket/tls/main.cpp Outdated
if (err != NSAPI_ERROR_OK) {
printf("Error from sock.connect: %d\n", err);
return err;
}

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 8, 2019

Contributor

This function is copy&paste from previous, only change is the port address.
Can the port address be a parameter to this and just use the same function?

TESTS/netsocket/tls/main.cpp Outdated
Case("TLSSOCKET_OPEN_TWICE", TLSSOCKET_OPEN_TWICE),
Case("TLSSOCKET_OPEN_LIMIT", TLSSOCKET_OPEN_LIMIT),
Case("TLSSOCKET_OPEN_DESTRUCT", TLSSOCKET_OPEN_DESTRUCT),
// Case("TLSSOCKET_HTTPS_GET", TLSSOCKET_HTTPS_GET), // Fails!

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 8, 2019

Contributor

// Fails! does not sound very good on public example.

Maybe "work-in-progress, unstable" or something like that.

Show resolved Hide resolved TESTS/netsocket/tls/tlssocket_echotest.cpp
TESTS/netsocket/tls/tlssocket_echotest.cpp Outdated
tls_global::TLS_OS_STACK_SIZE,
stack_mem,
"receiver");
TEST_ASSERT_EQUAL(osOK, thread->start(callback(tlssocket_echotest_nonblock_receiver, &pkt_s)));

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 8, 2019

Contributor

New thread per packet?

Sounds a bit overkill...

We could push event to global event loop every time we receive sigio(), and that event could the read until WOULD_BLOCK is received.. but if all data is received, it could set some global event flag to indicate that all data is read.

This comment has been minimized.

@michalpasztamobica

michalpasztamobica Jan 8, 2019

Author Contributor

Just to make sure we have a common understanding of what is going on in this test...

Indeed, a thread is created for every new packet size. But there is always just one receiver thread running at a time (tx_sem.wait will block the main thread's for loop until receiver thread exits). The receiver thread already contains a loop, which is controller with SIGIO (as per the previous comment :) ) and returns once all data is received.
So the only change would be to trigger the receiver's loop using a global event loop instead of triggering it using thread creation - is that right? Or is there some further optimisation which I failed to notice?

TESTS/netsocket/README.md Outdated

$ sudo nmap -sT -p7,9,13,37 echo.mbedcloudtesting.com
$ sudo nmap -sT -p7,9,13,37,2007,2009,2013 echo.mbedcloudtesting.com

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 8, 2019

Contributor

I can see that there was a error on previous documentation.
This was supposed to do UDP scan, and therefore required sudo

Can you change it to do
sudo nmap -sU instead?

TESTS/netsocket/tls/tlssocket_endpoint_close.cpp Outdated
* limitations under the License.
*/

#include "tls_tests.h""

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 8, 2019

Contributor

Douple "" at the end of line

Show resolved Hide resolved TESTS/netsocket/tls/tlssocket_endpoint_close.cpp

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jan 8, 2019

Show resolved Hide resolved TESTS/netsocket/tls/main.cpp
TESTS/netsocket/tls/tlssocket_bind_unopened.cpp Outdated

TLSSocket *sock = new TLSSocket;
if (!sock) {
TEST_FAIL();

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Jan 8, 2019

Contributor

TEST_FAIL won't abort execution so adding return-statement would be good here.

This comment has been minimized.

@michalpasztamobica

michalpasztamobica Jan 8, 2019

Author Contributor

Good point, but I removed the bind tests entirely.
However, these same comments apply also to tcp tests.

TESTS/netsocket/tls/tlssocket_no_cert.cpp Outdated
{
TLSSocket sock;
TEST_ASSERT_EQUAL(NSAPI_ERROR_OK, sock.open(get_interface()));
TEST_ASSERT_EQUAL(NSAPI_ERROR_AUTH_FAILURE, sock.connect(MBED_CONF_APP_ECHO_SERVER_ADDR, 2007));

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Jan 8, 2019

Contributor

Would be good to take the port number from app config json-file as this is non-standard port

Show resolved Hide resolved TESTS/netsocket/tls/tlssocket_send_unconnected.cpp

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_greentea branch Jan 8, 2019

@cmonr cmonr added needs: review and removed needs: work labels Jan 8, 2019

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_greentea branch Jan 9, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

SeppoTakalo commented Jan 9, 2019

@ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-core Hi guys, do you know that is there any way we could force main stack size to be raised to 8 kB just for these TLSSocket tests?

Currently that is roughly the amount of stack that TLS handshake requires. Therefore we need to resolve the stack issue before merging?

For example, can I manually create a thread that has 8kB of stack, and launch Greentea from there?

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

SeppoTakalo commented Jan 9, 2019

@michalpasztamobica Can you try for example following:


static int run_test(voi) {
    return !Harness::run(specification);
}

static char stack_mem[8192];
int main()
{
    Thread *th = new Thread(osPriorityNormal,
                        8192,
                         stack_mem,
                         "tls_gt_thread");
    th->start(run_test);
    return th->join();
}

If that works, we could perhaps use that to force bigger stack for TLS thread.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_greentea branch Jan 9, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Jan 9, 2019

@SeppoTakalo , your idea worked. Thanks!
I also added the echo-server-port-tls and echo-server-discard-port-tls definitions in tools/test_configs/*.json files.
If this is approved we can merge safely.

@SeppoTakalo
Copy link
Contributor

SeppoTakalo left a comment

Now looks OK

@OPpuolitaival
Copy link
Contributor

OPpuolitaival left a comment

Looks good

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Jan 11, 2019

@SeppoTakalo , @VeijoPesonen , I updated the README.md file with the TLSSocket test documentation and also amended some mistakes I spotted. I did not open a new PR, as the changes were actually relevant to this PR and it still wasn't merged.
Please, review just that one file once more.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Jan 22, 2019

Fixed the missing whitespace which Travis pointed out.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_greentea branch Jan 22, 2019

Add TLSSocket greentea tests.
TLSSocket tests based on existing TCPSocket greentea tests and TLSSocket icetea tests.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_greentea branch Jan 22, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Jan 22, 2019

Travis was giving some odd errors, so I rebased to latest master to check I am missing some CI fix...

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

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 22, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Jan 22, 2019

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_dynamic-memory-usage
  • jenkins-ci/mbed-os-ci_greentea-test

@cmonr cmonr added needs: work and removed needs: CI labels Jan 22, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 23, 2019

@michalpasztamobica test failures are related

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Jan 23, 2019

The reason is a known issue in IAR: #8306
I admit, I did not test locally with all compilers. IAR compiler doesn't take the configured heap size into consideration and we have seen in icetea tests that opening two TLSSockets consumes too much memory to fit into the IAR's predefined area size.

@SeppoTakalo , in icetea we left the test failing in order not to hide the bug, but I think for greentea we have to exclude the test, otherwise we will break the public CI. Do you agree?

I wrapped the TLSSOCKET_SIMULTANEOUS test with a preprocessor guard:

#ifndef __IAR_SYSTEMS_ICC__
    Case("TLSSOCKET_SIMULTANEOUS", TLSSOCKET_SIMULTANEOUS)
#endif

I checked that it doesn't hurt other compilers and IAR simply runs one test less.

Publish TLSSocket test documentation
The whole README.md had to be updated to match the internal Confluence documentation, which can now be locked. In the process I also updated any spotted mistakes in tests documentation, removed the obsolete TCPServer tests documentation and added a test which was missing from UDPSocket's main.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_greentea branch to af65522 Jan 23, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Jan 29, 2019

@0xc0170 , can we give this another try? Seppo approved my solution and it worked fine locally.

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Jan 29, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 29, 2019

Ci started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Jan 29, 2019

Test run: FAILED

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

Failed test jobs:

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

This comment has been minimized.

Copy link
Contributor

alekla01 commented Jan 29, 2019

jenkins-ci/greentea-test restarted

@0xc0170 0xc0170 merged commit 7d036b5 into ARMmbed:master Jan 30, 2019

24 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-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests 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 9884 cycles (+572 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

@michalpasztamobica michalpasztamobica deleted the michalpasztamobica:tlssocket_greentea branch Jan 30, 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.