Skip to content

gnrc_tcp: refactor tests#16461

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
brummer-simon:gnrc_tcp-refactor_tests
Jul 13, 2021
Merged

gnrc_tcp: refactor tests#16461
miri64 merged 1 commit intoRIOT-OS:masterfrom
brummer-simon:gnrc_tcp-refactor_tests

Conversation

@brummer-simon
Copy link
Member

@brummer-simon brummer-simon commented May 8, 2021

Contribution description

This PR refactors the test suite of GNRC_TCP. In consolidates all test scripts in a into a single script
and adds helper classes wrapping common functions used in GNRC_TCP tests.
This PR simplifies the process of adding additional tests greatly.
All existing tests were moved into the new test script.

Testing procedure

To test this PR follow the instructions under tests/gnrc_tcp/Readme.md

Issues/PRs references

Depends on PR #16459

@benpicco benpicco added Area: network Area: Networking State: waiting for other PR State: The PR requires another PR to be merged first Type: new feature The issue requests / The PR implemements a new feature for RIOT labels May 8, 2021
@brummer-simon brummer-simon changed the title GNRC TCP refactor tests gnrc_tcp: refactor tests May 8, 2021
@jeandudey jeandudey added this to the Release 2021.07 milestone May 12, 2021
This was referenced May 22, 2021
Merged
Merged
@github-actions github-actions bot added Area: doc Area: Documentation Area: sys Area: System Area: tests Area: tests and testing framework labels Jul 2, 2021
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 8, 2021
@benpicco
Copy link
Contributor

benpicco commented Jul 8, 2021 •
edited
Loading

feel free to rebase

Does this only move code around?

@github-actions github-actions bot removed Area: network Area: Networking Area: sys Area: System labels Jul 8, 2021
@brummer-simon
Copy link
Member Author

brummer-simon commented Jul 8, 2021 •
edited
Loading

feel free to rebase

Does this only move code around?

Mostly. It adds a few python helper classes simplifying the process of writing tests. All existing tests are merged into a single file and these tests are rewritten based on the helper classes. So the tests are moved and partly rewritten with the new helper classes.

All in all the tests test the same functionality as before.

@miri64
Copy link
Member

miri64 commented Jul 8, 2021

It adds a few python helper classes simplifying the process of writing tests.

How would you feel about porting the tests to riotctrl instead? That is the "new" way for implementing tests. Some of the commands implemented by shell_commands are provided in riotctrl_shell, application-specific ShellInteractions usually are implemented within the applications python test part (basically where you have the helpers now).

@brummer-simon
Copy link
Member Author

brummer-simon commented Jul 8, 2021 •
edited
Loading

It adds a few python helper classes simplifying the process of writing tests.

How would you feel about porting the tests to riotctrl instead? That is the "new" way for implementing tests. Some of the commands implemented by shell_commands are provided in riotctrl_shell, application-specific ShellInteractions usually are implemented within the applications python test part (basically where you have the helpers now).

Makes sence from my point of view. Although this PR and the two PR building onto of this PR introduce new tests based on the old scheme. I would prefer that we review and merge the open followup PRs and port the tests in a single PR after the successfull sock integration. @miri64 What do you think about this approach?

@miri64
Copy link
Member

miri64 commented Jul 8, 2021

@miri64 What do you think about this approach?

Mh... to be honest, I am not sure if it would be less work for you to rework the tests once instead of twice ;-)

@brummer-simon
Copy link
Member Author

brummer-simon commented Jul 8, 2021 •
edited
Loading

@miri64 What do you think about this approach?

Mh... to be honest, I am not sure if it would be less work for you to rework the tests once instead of twice ;-)

Well the PRs already ready for review including the tests. So the work is already done. Rewriting the test now would mean that I change large pieces of code in PRs that basically require only reviews.

EDIT: Here for reference:
#16493
#16494

@miri64
Copy link
Member

miri64 commented Jul 8, 2021

It's your code, so if you think rewriting twice in this case is less work, now that the first rework is basically done, I don't mind.

@brummer-simon
Copy link
Member Author

brummer-simon commented Jul 8, 2021

It's your code, so if you think rewriting twice in this case is less work, now that the first rework is basically done, I don't mind.

Lets do it this way then :)

@benpicco benpicco added the CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs label Jul 8, 2021
@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 8, 2021
@benpicco
Copy link
Contributor

benpicco commented Jul 10, 2021

Tests are still passing on

native

- "test_connection_lifecycle_as_client": SUCCESS
- "test_connection_lifecycle_as_server": SUCCESS
- "test_send_data_from_riot_to_host": SUCCESS
- "test_send_data_from_host_to_riot": SUCCESS
- "test_gnrc_tcp_garbage_packets_short_payload": SUCCESS
- "test_gnrc_tcp_garbage_packets_short_header": SUCCESS
- "test_gnrc_tcp_garbage_packets_ack_instead_of_sym": SUCCESS
- "test_gnrc_tcp_garbage_packets_option_parsing": SUCCESS
- "test_gnrc_tcp_recv_behavior_on_closed_connection": SUCCESS
- "test_gnrc_tcp_ep_from_str": SUCCESS
- "test_gnrc_tcp_ep_from_str_returns_EINVAL": SUCCESS
- "test_gnrc_tcp_accept_returns_EAGAIN": SUCCESS
- "test_gnrc_tcp_accept_returns_ETIMEDOUT": SUCCESS
- "test_gnrc_tcp_accept_returns_ENOMEM": SUCCESS
    Running listen/accept iteration 0
    Running listen/accept iteration 1
    Running listen/accept iteration 2
    Running listen/accept iteration 3
    Running listen/accept iteration 4
    Running listen/accept iteration 5
    Running listen/accept iteration 6
    Running listen/accept iteration 7
    Running listen/accept iteration 8
    Running listen/accept iteration 9
- "test_connection_listen_accept_cycle": SUCCESS
01-run.py: success

and

same54-xpro

- "test_connection_lifecycle_as_client": SUCCESS
- "test_connection_lifecycle_as_server": SUCCESS
- "test_send_data_from_riot_to_host": SUCCESS
- "test_send_data_from_host_to_riot": SUCCESS
- "test_gnrc_tcp_garbage_packets_short_payload": SUCCESS
- "test_gnrc_tcp_garbage_packets_short_header": SUCCESS
- "test_gnrc_tcp_garbage_packets_ack_instead_of_sym": SUCCESS
- "test_gnrc_tcp_garbage_packets_option_parsing": SUCCESS
- "test_gnrc_tcp_recv_behavior_on_closed_connection": SUCCESS
- "test_gnrc_tcp_ep_from_str": SUCCESS
- "test_gnrc_tcp_ep_from_str_returns_EINVAL": SUCCESS
- "test_gnrc_tcp_accept_returns_EAGAIN": SUCCESS
- "test_gnrc_tcp_accept_returns_ETIMEDOUT": SUCCESS
- "test_gnrc_tcp_accept_returns_ENOMEM": SUCCESS
    Running listen/accept iteration 0
    Running listen/accept iteration 1
    Running listen/accept iteration 2
    Running listen/accept iteration 3
    Running listen/accept iteration 4
    Running listen/accept iteration 5
    Running listen/accept iteration 6
    Running listen/accept iteration 7
    Running listen/accept iteration 8
    Running listen/accept iteration 9
- "test_connection_listen_accept_cycle": SUCCESS
01-run.py: success

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 13, 2021
@miri64 miri64 merged commit bf0bbbf into RIOT-OS:master Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@miri64 miri64 Awaiting requested review from miri64

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

Labels

Area: doc Area: Documentation Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2021.10

Development

Successfully merging this pull request may close these issues.

5 participants

@brummer-simon @benpicco @miri64 @jeandudey @MrKevinWeiss