Opened 4 years ago

Closed 4 years ago

#4500 closed defect (complete)

The lease6_rebind hook is missing unit tests and missing from the Hooks developer's guide

Reported by: tmark Owned by: fdupont
Priority: medium Milestone: Kea1.1
Component: hooks Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 2
Total Hours: 2 Internal?: no

Description

The lease6_rebind hook point is incomplete in terms of unit testing and it is not mentioned in the Hooks Developer's guide.

Subtickets

Change History (8)

comment:1 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.1

Per team meeting 12 May, accept 1.1. Estimate = .5d

comment:2 Changed 4 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to assigned

comment:3 Changed 4 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from assigned to reviewing

Done: rebind is very like renew with a few differences:

  • rebind forbids server-id (vs renew requires it)
  • renewed -> rebound
  • renewal -> rebinding

So I copied renew unit tests and hook documentation and applied these differences.

Ready for review? I don't believe a ChangeLog is required. BTW is there in the doc a packet flow description with all the hooks?

comment:4 follow-up: Changed 4 years ago by tomek

  • Add Hours to Ticket changed from 0 to 2
  • Owner changed from UnAssigned to fdupont
  • Total Hours changed from 0 to 2

I did review the changes. Thanks for writing those unit-tests. I have only two minor comments:

dhcp6_hooks.dox
rebindal => rebind

hooks_unittest.cc
lease6_rebind_update_callout gets lease6 parameter and stores it into
callback_lease6_ . If the parameter is null, the code would segfault.
Maybe you could add an assert there?

Once you address them, feel free to merge. I don't need to see this ticket again.

Code compiled and unit-tests passed on Ubuntu 15.10 x64.

There are no user-visible changes, so it's ok to not have ChangeLog? entry.

comment:5 in reply to: ↑ 4 Changed 4 years ago by fdupont

Replying to tomek:

I did review the changes. Thanks for writing those unit-tests. I have only two minor comments:

dhcp6_hooks.dox
rebindal => rebind

=> in fact it is "rebinding". Done.

hooks_unittest.cc
lease6_rebind_update_callout gets lease6 parameter and stores it into
callback_lease6_ . If the parameter is null, the code would segfault.
Maybe you could add an assert there?

=> I agree. I added asserts to similar cases too.

comment:6 Changed 4 years ago by fdupont

Changed the asserts in expects with a comment so if one day it crashes at this place it will be easy to debug...

comment:7 Changed 4 years ago by fdupont

I put this here to not lost it but it should be explained for instance the developer guide: when an ASSERT is in a fixture method (vs a TEST body and the method does not return void you can get void value not ignored as it ought to be errors.

comment:8 Changed 4 years ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged. Closing.

Note: See TracTickets for help on using tickets.