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

gnrc_tcp: refactor tests #16461

Merged
merged 1 commit into from
Jul 13, 2021
Merged

gnrc_tcp: refactor tests #16461

merged 1 commit into from
Jul 13, 2021

Conversation

brummer-simon
Copy link
Member

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
@brummer-simon brummer-simon force-pushed the gnrc_tcp-refactor_tests branch from 3095bc1 to 0886675 Compare July 2, 2021 08:02
@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
@brummer-simon brummer-simon force-pushed the gnrc_tcp-refactor_tests branch from 0886675 to 5b26589 Compare July 8, 2021 09:18
@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?

@brummer-simon brummer-simon force-pushed the gnrc_tcp-refactor_tests branch from 5b26589 to 876c24d Compare July 8, 2021 12:22
@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

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 CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 8, 2021
@benpicco
Copy link
Contributor

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