Opened 7 years ago

Closed 7 years ago

#2467 closed defect (fixed)

Remove raw pointers from DHCP test code

Reported by: stephen Owned by: stephen
Priority: low Milestone: Sprint-DHCP-20130509
Component: dhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Low
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

A lot of DHCP test code relies on allocating objects on the heap and storing the address in a raw pointer, then using delete at the end of the test to destroy it. Occasionally the delete is missed and the valgrind pass fails.

Unless there is a compelling reason otherwise, objects allocated during tests should be pointed to by a smart pointer. This ticket is for a refactoring of the existing tests to do this.

Subtickets

Change History (10)

comment:1 Changed 7 years ago by stephen

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130328

comment:2 Changed 7 years ago by stephen

  • Owner set to stephen
  • Status changed from new to accepted

comment:3 Changed 7 years ago by stephen

  • Owner changed from stephen to UnAssigned
  • Status changed from accepted to reviewing

Pointers removed, now ready for review.

comment:4 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 Changed 7 years ago by stephen

  • Owner changed from stephen to UnAssigned

Oops - had missed the b10-dhcp4/6 tests.

It's really ready for review now.

comment:6 Changed 7 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:7 Changed 7 years ago by tomek

  • Owner changed from tomek to stephen

Reviewing changes made on af37bfe0c400e0dd923f57e9775d67a8f17af802.

Do we want to document methods and classes in test code? Most methods from config_parser_unitest.cc are documented, but all class members are not. You modified srv_. Would it be possible to document it (and the other two members as well)?

dhcp4_srv_unittest.cc
boost::shared_ptr<Pkt4> is used throughout the code. It should be substituted with Pkt4Ptr. As this is strictly not within the scope of this ticket, I'm ok if you choose to not do it. But then please create a separate ticket for it.

dhcp6_srv_unittest.cc
boost::shared_ptr<Pkt6> may be replaced with Pkt6Ptr.

iface_mgr_unittest.cc
There's a (disabled) dhcp6Sniffer pseudo-test. It is not really a test, but a piece of code I used for generating test code based on actual received packets. We should probably extract it into separate file and keep as test writing assistance tool.

In any case, this code uses raw pointers. It should not do that.

Please submit a ticket for IfaceMgr::getIface() update. It should not return raw pointer, but shared_ptr instead.

option4_addrlst_unittest.cc
I'm somewhat familiar with the concept of a complex time, but we do not yet support it. Please remove i from copyright year. :)

option_unittest.cc
Comment for v4_basic test is not valid anymore. v4 is now implemented.

pkt4_unittest.cc
There are several multiplications in Pkt4Test.options that do not have spaces around the * and + operators.

Thank you for doing this work. It must have been less than exciting.

comment:8 Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Changes ready for review.

Please submit a ticket for IfaceMgr::getIface() update. It should not return raw pointer, but shared_ptr instead.

Ticket #2936 created for this.

I'm somewhat familiar with the concept of a complex time, but we do not yet support it.

Well that puts paid to plans of applying the concepts of Minkowski spacetime to the DHCP protocol. The "i" has been removed.

Last edited 7 years ago by stephen (previous) (diff)

comment:9 Changed 7 years ago by tomek

  • Owner changed from tomek to stephen

All my comments are addressed in checkin 55dd08057c52c3f0baf3f1b34f23f87c2ee7ff35, except the one about documenting modified fragments in config_parser_unittest.cc. That was just a minor comment that is not really important, so it is ok to leave it as it is.

Please merge.

comment:10 Changed 7 years ago by stephen

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

Merged in commit 315d2846dec8b2f36df1aa8ed23feddfc621a356

Note: See TracTickets for help on using tickets.