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.

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

tests/gnrc_dhcpv6_client: add script to check if $IFACE exists #16797

Merged

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Sep 1, 2021

Contribution description

Adds a checker script if the required interface for tests/gnrc_dhcpv6_client exists.

Testing procedure

Run tests/gnrc_dhcpv6_client with and without a TAP interface

sudo dist/tools/tapsetup/tapsetup
make -C tests/gnrc_dhcpv6_client flash -j test-with-config # should succeed
sudo dist/tools/tapsetup/tapsetup -d
make -C tests/gnrc_dhcpv6_client flash -j test-with-config # should fail with appropriate error message

Issues/PRs references

Depends on #16795 for the config check to work and #16796 for the $IFACE export (see #16797 (comment)).

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: waiting for other PR State: The PR requires another PR to be merged first labels Sep 1, 2021
@miri64 miri64 requested a review from kfessel September 1, 2021 15:09
@github-actions github-actions bot added Area: build system Area: Build system Area: tests Area: tests and testing framework labels Sep 1, 2021
@miri64
Copy link
Member Author

miri64 commented Sep 2, 2021

As pointed out by @fjmolinas in #16796 (comment), the export is not necessary. So I removed that dependency with the latest rebase and adopted the Makefile to expose IFACE to the config checker target.

@kfessel
Copy link
Contributor

kfessel commented Sep 3, 2021

PORT may be checked for existence as well - in native case

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

I think this should be rebased and squashed -since its dependencies are merged.
Maybe the wording in the failure message can be changed as well:

@miri64
Copy link
Member Author

miri64 commented Sep 6, 2021

PORT may be checked for existence as well - in native case

It is set by default.

@kfessel
Copy link
Contributor

kfessel commented Sep 6, 2021

PORT may be checked for existence as well - in native case

It is set by default.

i meant check if the interface the is named by PORT is exiting (since it might be a non exiting tap interface with native)

but it is also the case the such a checkscript is not able to test for every misconfiguration (maybe both exist but aren't conected)

@miri64
Copy link
Member Author

miri64 commented Sep 6, 2021

i meant check if the interface the is named by PORT is exiting (since it might be a non exiting tap interface with native)

The native instance will fail to start if it can't be used as a TAP interface or if it isn't initialized. Adding yet another check for that is IMHO really not necessary. If you don't know how to use native with networking, you shouldn't do testing ;-).

@miri64 miri64 force-pushed the tests/enh/gnrc_dhcpv6_client-config-checker branch from ef1eaf9 to 845d589 Compare September 6, 2021 13:18
@github-actions github-actions bot removed the Area: build system Area: Build system label Sep 6, 2021
@miri64
Copy link
Member Author

miri64 commented Sep 6, 2021

Rebased and squashed latest suggestion

@miri64 miri64 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 Sep 6, 2021
@kfessel
Copy link
Contributor

kfessel commented Sep 6, 2021

somehow IFACE sems to not be set

tests/gnrc_dhcpv6_client$ make test-with-config
Device "" does not exist.
You may be able to create "" by using e.g. `dist/tools/tapsetup/tapsetup`.
System configuration for tests is not available, check tests/gnrc_dhcpv6_client/tests-with-config/check-config.sh failed.
tests/gnrc_dhcpv6_client$ IFACE=tapbr0 make test-with-config

finishes successfully

@kfessel kfessel 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 Sep 6, 2021
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

I think a comment would be nice for a future reader of this Makefile

@kfessel
Copy link
Contributor

kfessel commented Sep 7, 2021

I tested:
tapbr0 missing and $ make test-with-config fails with message as expected
tapbr0 setup and $ make test-with-config succeeds as expected
tapbr1 missing and $ IFACE=tapbr1 PORT=tap10 make test-with-config fails with message as expected
tapbr1 setup and $ IFACE=tapbr1 PORT=tap10 make test-with-config succeeds as expected

in between these tests kea-dhcp6 needed to be killed since its config changed

please squash

@miri64 miri64 force-pushed the tests/enh/gnrc_dhcpv6_client-config-checker branch from 9a427a1 to 73d5997 Compare September 7, 2021 11:49
@miri64
Copy link
Member Author

miri64 commented Sep 7, 2021

please squash

Done. I also removed the now orphaned test-with-config/check-config target within the application's Makefile.

@kfessel kfessel added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Sep 7, 2021
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

🎉
🥳
🎉

@kfessel kfessel merged commit f65f3c7 into RIOT-OS:master Sep 8, 2021
@miri64 miri64 deleted the tests/enh/gnrc_dhcpv6_client-config-checker branch September 8, 2021 14:57
@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants