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

tests/gnrc_tcp_*: remove default PORT for native #10948

Merged
merged 2 commits into from
Feb 5, 2019
Merged

tests/gnrc_tcp_*: remove default PORT for native #10948

merged 2 commits into from
Feb 5, 2019

Conversation

PeterKietzmann
Copy link
Member

Contribution description

This PR removes the default PORT?= which was set to run the TCP client/server test applications on two native instances. A problem occurs when calling BOARD=xyz make flash term (with xyz not native) because in that case, PORT is set to tap by defualt.

Testing procedure

Build/flash/term a board with one of the applications tests/gnrc_tcp_* with and without this PR.

Issues/PRs references

#10947

@PeterKietzmann PeterKietzmann added Area: network Area: Networking Area: tests Area: tests and testing framework Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 5, 2019
@miri64
Copy link
Member

miri64 commented Feb 5, 2019

The point of those is, I believe that those two apps belong to each other. IMHO we need to rework these tests in general (however, having the first bisect in #10947 having a Python script interacting via a TCP socket with the test is probably not a good solution either)

smlng
smlng suggested changes Feb 5, 2019
View reviewed changes
tests/gnrc_tcp_client/README.md Outdated
Build and run test:
make clean all term
Build and run test (don't forget to use different tap device than server):
make clean all term PORT=tap1
Copy link
Member

Choose a reason for hiding this comment

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

such a line should be added to the servers readme, too ... or not?

Copy link
Member

Choose a reason for hiding this comment

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

PORT defaults to tap0 so it is not strictly necessary.

Copy link
Member

Choose a reason for hiding this comment

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

alternatively, as some might overlook this or do not know that tap0 is default, we could add:

ifeq (native,$(BOARD))
PORT ?= tap1
endif

in the Makefile instead

Copy link
Member

@miri64 miri64 Feb 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'd prefer that solution as well.

@PeterKietzmann
Copy link
Member Author

Fine with me. Changed, squashed, pushed...

@smlng smlng added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Feb 5, 2019
@smlng
Copy link
Member

smlng commented Feb 5, 2019

@PeterKietzmann I assume (and trust) that you've tested this 😄

@PeterKietzmann
Copy link
Member Author

@smlng you should never do that :-P! However, I was able to start both programs via make all term. The server application automatically chose tap0, the client chose tap1.

@miri64
Copy link
Member

miri64 commented Feb 5, 2019

Confirmed.

@miri64 miri64 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Feb 5, 2019
@miri64 miri64 merged commit a73deac into RIOT-OS:master Feb 5, 2019
@PeterKietzmann PeterKietzmann deleted the pr_tests_tcp_del_tap branch March 4, 2019 09:27
@danpetry danpetry added this to the Release 2019.04 milestone Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 left review comments

@smlng smlng smlng approved these changes

@brummer-simon brummer-simon brummer-simon approved these changes

Assignees
No one assigned
Labels
Area: network Area: Networking 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 Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines
Projects
None yet
Milestone
Release 2019.04
Development

Successfully merging this pull request may close these issues.
5 participants
@PeterKietzmann @miri64 @smlng @brummer-simon @danpetry