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_ipv6_nib: rtr_ltime test #20372

Merged
merged 3 commits into from
Sep 6, 2024
Merged

Conversation

xnumad
Copy link
Contributor

@xnumad xnumad commented Feb 11, 2024

Contribution description

This PR only provides a test to verify that _handle_rtr_timeout does not have any effect on any RA options:

The Router Lifetime applies only to
the router's usefulness as a default router; it
does not apply to information contained in other
message fields or options
. Options that need time
limits for their information include their own
lifetime fields.

Emphasis added to highlight what part exactly this test covers. (In particular, it does not cover testing that the "Router Lifetime" has any effect to "the router's usefulness as a default router".)

Test is implemented as part of the "base test function", causing 4 tests to fail if not providing an implementation.

Issues/PRs references

Test is fulfilled by #20329 and #20371 (alone each, but they are also not mutually exclusive).
While #20329 itself already provides tests, this PR is an additional test (so no duplicate tests across PRs or sth). (In fact, they do not seem to cover the code of that PR; they still succeed if only keeping the tests from the PR.)

@xnumad xnumad requested a review from miri64 as a code owner February 11, 2024 22:36
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Feb 11, 2024
@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 Feb 12, 2024
@riot-ci
Copy link

riot-ci commented Feb 12, 2024

Murdock results

✔️ PASSED

e767e44 tests/gnrc_ipv6_nib: refactor: extract DAD test

Success Failures Total Runtime
63 0 63 01m:31s

Artifacts

@xnumad
Copy link
Contributor Author

xnumad commented Jul 29, 2024

Rebased, meaning PR can now be merged since test does not fail anymore since the code being tested was fixed (through merge of https://github.com/RIOT-OS/RIOT/pull/20329/files#diff-fba26fd610698922256809a70531c3b18dc624a771563ea8887a08bbf762c457L1470-L1471)

@xnumad
Copy link
Contributor Author

xnumad commented Jul 29, 2024

Force push: Modified commit: Adjusted for changes meanwhile made to master (via bb9ca2e)

Keep testing for options before removing default router
/* no further options */
TEST_ASSERT_NULL(pkt->next->next->next);
gnrc_pktbuf_release(pkt);
if (is_first_check) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you factor this out to a new function, please?
And then not call this inside test_handle_pkt__rtr_adv__options_success().
Effectively it is testing if Duplicate Address Detection is triggered by SLAAC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I thought since SLAAC is triggered by the Prefix Information Option in the Router Advertisement, I could leave its DAD test there. But your suggestion makes test_handle_pkt__rtr_adv__options_success() idempotent, which can be helpful.

Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

make -C tests/net/gnrc_ipv6_nib flash term

OK (53 tests)
{ "threads": [{ "name": "idle", "stack_size": 8192, "stack_used": 436}]}
{ "threads": [{ "name": "main", "stack_size": 12288, "stack_used": 2564}]}

@fabian18 fabian18 added this pull request to the merge queue Aug 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 29, 2024
@benpicco benpicco added this pull request to the merge queue Sep 6, 2024
Merged via the queue into RIOT-OS:master with commit 7dbb298 Sep 6, 2024
27 checks passed
@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants