Opened 7 years ago

Closed 7 years ago

#2721 closed task (fixed)

Write unit test for Ticket 2719

Reported by: jwright Owned by: tmark
Priority: medium Milestone: Sprint-DHCP-20130411
Component: Unclassified Version:
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: 0
Total Hours: 0 Internal?: no

Description

Need to have a unit tests written for Ticket #2719 (http://bind10.isc.org/ticket/2719).

Subtickets

Attachments (1)

dhcp6_unittests.log (7.6 KB) - added by tmark 7 years ago.
unit test run output

Download all attachments as: .zip

Change History (12)

comment:1 Changed 7 years ago by jwright

  • Milestone changed from New Tasks to Sprint-DHCP-20130214

comment:2 Changed 7 years ago by tmark

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

Changed 7 years ago by tmark

unit test run output

comment:3 Changed 7 years ago by tmark

The existing tests use the Dhcp6SvrTest test_fixture class. This class provides
functionality for conducting tests based upon a canned, pre-configured v6 subnet.
Since the new tests needed to be performed against an "empty" configuration,
I split out the common functionality into a base class text fixture, NakedDhcp6SvrTest,
and altered Dhcp6SvrTest to derive from it. The new base class has no
pre-configuration. What remains in Dhcp6SvrTest is geared solely towards tests against
a valid, canned, configuration.

The new test_fixture NakedDhcp6SrvTest is used for the following new tests:

  1. SolicitNoSubnet? checks response to a SOLICIT w/o subnet defined
  2. RequestNoSubnet? checks response to a REQUEST w/o subnet defined
  3. RenewNoSubnet? checks response to a RENEW w/o subnet defined
  4. ReleaseNoSubnet? checks response to a RELEASE w/o subnet defined

comment:4 Changed 7 years ago by tmark

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

comment:5 follow-up: Changed 7 years ago by marcin

  • Owner changed from UnAssigned to tmark

checkNakResponse()

Please add a short description of the function

The list of function arguments in the second line should be aligned with arguments in the first line like this

    void checkNakResponse(const Pkt6Ptr& rsp, uint8_t expected_message_type,
                          uint32_t expected_transid, uint16_t expected_status_code) {

''tmp'' is not the best name for the object holding an option. I can see that ''tmp'' is used everywhere in this file but it is wrong.
Assume, that in the following code:

// Check that IA_NA was returned 
OptionPtr tmp = rsp->getOption(D6O_IA_NA);
ASSERT_TRUE(tmp);

the getOption() function returns NULL pointer. The ASSERT will report an error, visible in a log file. This log will tell me that some temporary value is NULL. If you called this variable ''option_ia_na'' I would know from the log that it is IA option which is NULL - that gives me some more information.

checkIA_NA:
This line the code:

boost::shared_ptr<Option6IA> ia = boost::dynamic_pointer_cast<Option6IA>(tmp);

may result in ''ia'' being NULL pointer. Please consider adding ''ASSERT_TRUE(ia)'' before using it further in the code.

SolicitNoSubnet: I suggest that the function description is rephrased: ''Test verifies that incoming SOLICIT can be handled properly when there are no subnets configured''. The word ''even'' suggests that test may be also doing other checks and the no-subnet check is only special case. This refers for other tests as well.

RequestNoSubnet: The test description seems to be copied from the previous test. Please replace SOLICIT with REQUEST.

ChangeLog'''

  • Spurious '']'' in ''func''.
  • There is a white-space aty the end of the second line in the entry.
  • (See Trac #2719): perhaps it would be better to mention that unit-tests being added have been missed during implementation of #2719.
  • There should be a blank line between your entry and the previous entry
  • s/v6/IPv6 as the latter is the actual protocol name

comment:6 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by tmark

Replying to marcin:

checkNakResponse()

Please add a short description of the function

Done.

The list of function arguments in the second line should be aligned with arguments in the first line like this

    void checkNakResponse(const Pkt6Ptr& rsp, uint8_t expected_message_type,
                          uint32_t expected_transid, uint16_t expected_status_code) {

Done.

''tmp'' is not the best name for the object holding an option. I can see that ''tmp'' is used everywhere in this file but it is wrong.
Assume, that in the following code:

// Check that IA_NA was returned 
OptionPtr tmp = rsp->getOption(D6O_IA_NA);
ASSERT_TRUE(tmp);

the getOption() function returns NULL pointer. The ASSERT will report an error, visible in a log file. This log will tell me that some temporary value is NULL. If you called this variable ''option_ia_na'' I would know from the log that it is IA option which is NULL - that gives me some more information.

checkIA_NA:
This line the code:

boost::shared_ptr<Option6IA> ia = boost::dynamic_pointer_cast<Option6IA>(tmp);

may result in ''ia'' being NULL pointer. Please consider adding ''ASSERT_TRUE(ia)'' before using it further in the code.

This line is already followed by a call to ASSERT_TRU(ia).

SolicitNoSubnet: I suggest that the function description is rephrased: ''Test verifies that incoming SOLICIT can be handled properly when there are no subnets configured''. The word ''even'' suggests that test may be also doing other checks and the no-subnet check is only special case. This refers for other tests as well.

RequestNoSubnet: The test description seems to be copied from the previous test. Please replace SOLICIT with REQUEST.

Done.

ChangeLog'''

  • Spurious '']'' in ''func''.
  • There is a white-space aty the end of the second line in the entry.
  • (See Trac #2719): perhaps it would be better to mention that unit-tests being added have been missed during implementation of #2719.
  • There should be a blank line between your entry and the previous entry
  • s/v6/IPv6 as the latter is the actual protocol name

I reverted the original commit to ChangeLog? as it was made prematurely. The following is submitted for review as the new ChangeLog? entry:

5XX. [func] tmark

b10-dhcp6: Added unit tests for handling requests when no
v6 subnets are configured/defined. Testing these conditions
was overlooked during implementation of Trac #2719.
(Trac #2721, git 03b6bd8461d7aee84ac34da1c737fed4a3c16c22)
(Trac #2721, git c533897b58690ab30d12dd9837de04e08d840ee3)

comment:7 Changed 7 years ago by tmark

  • Owner changed from tmark to marcin

comment:8 in reply to: ↑ 6 Changed 7 years ago by marcin

  • Owner changed from marcin to tmark

Replying to tmark:

checkIA_NA:
This line the code:

boost::shared_ptr<Option6IA> ia = boost::dynamic_pointer_cast<Option6IA>(tmp);

may result in ''ia'' being NULL pointer. Please consider adding ''ASSERT_TRUE(ia)'' before using it further in the code.

This line is already followed by a call to ASSERT_TRU(ia).

Not it is not.

5XX. [func] tmark

b10-dhcp6: Added unit tests for handling requests when no
v6 subnets are configured/defined. Testing these conditions
was overlooked during implementation of Trac #2719.
(Trac #2721, git 03b6bd8461d7aee84ac34da1c737fed4a3c16c22)
(Trac #2721, git c533897b58690ab30d12dd9837de04e08d840ee3)

Why specify two commits for #2721?

comment:9 Changed 7 years ago by tmark

  • Owner changed from tmark to marcin

checkIA_NA method was modified with a an test and an ADD_FAILURE, as ASSERT may not be used in non-void functions.

ChangeLog? entry will have only the git commit # used for the merge (TBD).

comment:10 Changed 7 years ago by marcin

  • Owner changed from marcin to tmark

All ok. Please merge.

comment:11 Changed 7 years ago by tmark

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

Changes merged.

Note: See TracTickets for help on using tickets.