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.

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

Fix UDP socket test cases #12267

Merged
merged 1 commit into from
Jan 17, 2020
Merged

Fix UDP socket test cases #12267

merged 1 commit into from
Jan 17, 2020

Conversation

AriParkkila
Copy link

Summary of changes

Fix UDP socket test cases to check more strictly on failure, and lower trace levels to not print errors on successful cases.

Impact of changes

Migration actions required

Documentation

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ciarmcom ciarmcom requested review from a team January 16, 2020 10:00
@ciarmcom
Copy link
Member

@AriParkkila, thank you for your changes.
@ARMmbed/mbed-os-test @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

michalpasztamobica
michalpasztamobica suggested changes Jan 16, 2020
View reviewed changes
Copy link
Contributor

@michalpasztamobica michalpasztamobica left a comment

Choose a reason for hiding this comment

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

Thanks for the clean up. I just have doubts if the extra branch is necessary or not... In case it is, can we use TEST_FAIL_MESSAGE() so the reason for failure is visible without checking source code?

TESTS/netsocket/nidd/niddsocket_echotest.cpp Outdated
@@ -129,6 +129,8 @@ void NIDDSOCKET_ECHOTEST_NONBLOCK()
double loss_ratio = 1 - ((double)packets_recv / (double)packets_sent);
tr_info("Packets sent: %d, packets received %d, loss ratio %.2lf", packets_sent, packets_recv, loss_ratio);
TEST_ASSERT_DOUBLE_WITHIN(TOLERATED_LOSS_RATIO, EXPECTED_LOSS_RATIO, loss_ratio);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the if (packets_sent > 0) was here to avoid division by 0, not to detect that packets were sent or not. If they were not sent we would fail in line 117. So I think this is dead code.
Unless I am not seeing some failure mode, that in fact leads to this branch being a possible option?

TESTS/netsocket/udp/udpsocket_echotest.cpp Outdated
@@ -267,6 +269,8 @@ void UDPSOCKET_ECHOTEST_NONBLOCK_impl(bool use_sendto)
TEST_ASSERT_DOUBLE_WITHIN(TOLERATED_LOSS_RATIO, EXPECTED_LOSS_RATIO, loss_ratio);

#endif
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. Will this ever trigger?

TESTS/network/multihoming/multihoming_udpsocket_echotest.cpp
TEST_ASSERT_DOUBLE_WITHIN(TOLERATED_LOSS_RATIO, EXPECTED_LOSS_RATIO, loss_ratio);

#endif
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

And here as well.

Copy link
Author

Choose a reason for hiding this comment

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

If no packets were sent then packets_sent equals to 0, there is no extra check as in the two cases above?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right for the multihoming case - this makes sense,
In the two other cases these lines will not allow the situation where packets_sent is 0:

        // Make sure that at least one packet of every size was sent.
        TEST_ASSERT_TRUE(packets_sent > packets_sent_prev);

If no packets get sent, this assertion fails and we never reach the if (packets_sent > 0), so perhaps we could actually remove this check (although I believe it's left there in case someone modifies the code and forgets the division by 0 situation or for the static analysis?)

Copy link
Author

Choose a reason for hiding this comment

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

@michalpasztamobica Fixed the two cases you mentioned, please review.

michalpasztamobica
michalpasztamobica reviewed Jan 16, 2020
View reviewed changes
TESTS/netsocket/udp/udpsocket_echotest.cpp Outdated
@@ -146,6 +146,8 @@ void UDPSOCKET_ECHOTEST_impl(bool use_sendto)
double loss_ratio = 1 - ((double)packets_recv / (double)packets_sent);
tr_info("Packets sent: %d, packets received %d, loss ratio %.2lf", packets_sent, packets_recv, loss_ratio);
TEST_ASSERT_DOUBLE_WITHIN(TOLERATED_LOSS_RATIO, EXPECTED_LOSS_RATIO, loss_ratio);
} else {
TEST_FAIL();
Copy link
Contributor

Choose a reason for hiding this comment

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

@AriParkkila Should this be here? Looks like the TEST_ASSERT_TRUE(packets_sent > packets_sent_prev); is just above...

Copy link
Author

Choose a reason for hiding this comment

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

True not needed here, fixed.

0xc0170
0xc0170 requested changes Jan 16, 2020
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Can you amend the info you provided in the first comment about the fix (plus from discussion with @michalpasztamobica about else conditions). It would be helpful.

Otherwise LGTM

Fix UDP socket test cases
031d90a 
Fix UDP socket test cases to check more strictly on failure,
and lower trace levels to not print errors on successful cases.
@AriParkkila
Copy link
Author

@0xc0170 Commit message updated. Should be ready for merge now.

@0xc0170 0xc0170 self-requested a review January 17, 2020 09:46
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 17, 2020

CI restarted

@mbed-ci
Copy link

mbed-ci commented Jan 17, 2020

Test run: SUCCESS

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

@0xc0170 0xc0170 added release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 and removed needs: CI labels Jan 17, 2020
@0xc0170 0xc0170 merged commit 49b158b into ARMmbed:master Jan 17, 2020
@michalpasztamobica michalpasztamobica mentioned this pull request Jan 24, 2020
Merged
jeromecoutant
jeromecoutant reviewed Feb 14, 2020
View reviewed changes
TESTS/network/multihoming/multihoming_udpsocket_echotest.cpp
@@ -84,7 +84,7 @@ void MULTIHOMING_UDPSOCKET_ECHOTEST()
packets_sent++;
}
if (sent != pkt_s) {
printf("[Round#%02d - Sender] error, returned %d\n", s_idx, sent);
tr_warn("[Round#%02d - Sender] error, returned %d\n", s_idx, sent);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi all

Seems #define TRACE_GROUP "GRNT" is missing here ?

        [Error] multihoming_udpsocket_echotest.cpp@87,25: use of undeclared identifier 'TRACE_GROUP'
        [Error] multihoming_udpsocket_echotest.cpp@102,17: use of undeclared identifier 'TRACE_GROUP'
        [Error] multihoming_udpsocket_echotest.cpp@182,25: use of undeclared identifier 'TRACE_GROUP'
        [Error] multihoming_udpsocket_echotest.cpp@201,17: use of undeclared identifier 'TRACE_GROUP'

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

@0xc0170 0xc0170 0xc0170 approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

@michalpasztamobica michalpasztamobica michalpasztamobica approved these changes

Assignees
No one assigned
Labels
release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@AriParkkila @ciarmcom @0xc0170 @mbed-ci @SeppoTakalo @jeromecoutant @michalpasztamobica