Skip to content

TCP/UDP greentea tests refactoring and cleanup #9376

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 18, 2019
Merged

TCP/UDP greentea tests refactoring and cleanup #9376

merged 1 commit into from
Jan 18, 2019

Conversation

michalpasztamobica
Copy link
Contributor

Description

When adding TLSSocket greentea tests recently in #9283, there was a number of review remarks. Those tests were based on TCPSocket, so most of the review remarks apply there and some to UDPSocket greentea tests, too.

Pull request type

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

Reviewers

@SeppoTakalo
@VeijoPesonen

VeijoPesonen
VeijoPesonen reviewed Jan 15, 2019
View reviewed changes
tools/test_configs/no_network.json Outdated
},
"echo-server-discard-port" : {
"help" : "Discard port of echo server",
"value" : "9"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see value of the "echo-server-port" and the name of the file.

VeijoPesonen
VeijoPesonen reviewed Jan 15, 2019
View reviewed changes
TESTS/netsocket/tcp/tcpsocket_echotest.cpp
static void _sigio_handler(osThreadId id)
{
osSignalSet(id, SIGNAL_SIGIO);
if (event_queue != NULL) {
event_queue->call(tcpsocket_echotest_nonblock_receive);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should here either be an else-clause or the event_queue.call() doesn't need to be wrapped on if-clause

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, never mind... Once you register the SIGIO handler it might be that you don't have a handle to event queue yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the remark. I changed the order of test initialization, so that the event queue gets created before we register the sigio handler.
Nevertheless, to be on the safe side, I left the NULL check and added an else statement that raises an error in case the NULL check fails :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok :) This is not a suggestion to change anything but using TEST_FAIL_MESSAGE indicates there is a problem with the implementation under test. But, actually the issue would be with the test case implementation itself. MBED_ASSERT or MBED_ERROR might be good alternatives here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good point, thanks for that, but I think it makes some sense to stay with TEST_FAIL_MESSAGE.
Firstly, it is not that obvious to me that the error must come from the test code. What if the handler didn't get unregistered correctly or if the EventQueue creation is erroneous?
Also, MBED_ASSERT/ERROR will crash the whole test suite and I find this reaction too serious, given the circumstances.
I hope you're ok with this? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me :)

VeijoPesonen
VeijoPesonen reviewed Jan 15, 2019
View reviewed changes
TESTS/netsocket/tcp/tcpsocket_echotest.cpp
int recvd = sock.recv(&(tcp_global::rx_buffer[bytes2recv_total - bytes2recv]), bytes2recv);
if (recvd == NSAPI_ERROR_WOULD_BLOCK) {
if (tc_exec_time.read() >= time_allotted) {
receive_error = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tx_sem isn't released here. Sender might end up waiting forever as the timeout was removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't really want to release the semaphore in case of WOULD_BLOCK - we want to wait until the data arrives. Therefore, I added the timeout back, but fixed it to the correct units.
I hope this is fine? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, that's fine.

jeromecoutant
jeromecoutant reviewed Jan 15, 2019
View reviewed changes
tools/test_configs/EthernetInterface.json
},
"echo-server-discard-port" : {
"help" : "Discard port of echo server",
"value" : "9"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi
I am using a local test network.
Is this new setting need some new configuration command to my server ?
Thx

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This port number is the same as what was used earlier. The only difference is that instead of it being hardcoded now it can be configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
No, there is no need to change the network/server configuration.
The tests were so far using the same port number, but it was coded explicitly, like so:
tcp_addr.set_port(9);
Now it is defined in the config file, to avoid magic numbers, like so:
tcp_addr.set_port(MBED_CONF_APP_ECHO_SERVER_DISCARD_PORT);

@0xc0170 0xc0170 requested a review from SeppoTakalo January 15, 2019 10:30
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 17, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 18, 2019

Test run: SUCCESS

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

cmonr
cmonr approved these changes Jan 18, 2019
View reviewed changes
Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@cmonr cmonr merged commit 68deb05 into ARMmbed:master Jan 18, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 21, 2019

@michalpasztamobica Can you talk to @OPpuolitaival about this new config addition (our nightly fails as new config addition is not defined there).

@TuomoHautamaki
Copy link

Fix already created and under review

@michalpasztamobica
Copy link
Contributor Author

I've seen the fix and it makes sense. Sorry for breaking the build, I didn't know there are external configs in use in CI, I assumed everyone would use the ones we provide. Next time I will notify @OPpuolitaival in advance.

@michalpasztamobica michalpasztamobica mentioned this pull request Jan 21, 2019
Merged
@michalpasztamobica michalpasztamobica deleted the tcpsocket_greentea branch January 21, 2019 14:35
@OPpuolitaival
Copy link
Contributor

We can try to use configuration more flexible way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeromecoutant jeromecoutant jeromecoutant left review comments

@cmonr cmonr cmonr approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

@VeijoPesonen VeijoPesonen VeijoPesonen approved these changes

Assignees
No one assigned
Labels
release-version: 5.11.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@michalpasztamobica @0xc0170 @mbed-ci @TuomoHautamaki @OPpuolitaival @cmonr @SeppoTakalo @jeromecoutant @VeijoPesonen