Opened 8 years ago

Closed 8 years ago

#1967 closed defect (fixed)

dhcp6 Solicit_basic test fails

Reported by: jinmei Owned by: tomek
Priority: very high Milestone: Sprint-DHCP-20120528
Component: dhcp6 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

See
http://git.bind10.isc.org/~tester/builder//BIND10-lettuce/20120509180001-MacOS/logs/unittests.out

I've tentatively disabled the offending test. Please fix it ASAP.

Subtickets

Change History (9)

comment:1 Changed 8 years ago by tomek

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

comment:2 Changed 8 years ago by tomek

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

Fix pushed. It seems the reason was a simple one. Previous test didn't close sockets, so Basic_Solicit couldn't bind sockets.

Please review trac1967 branch.

comment:3 Changed 8 years ago by tomek

  • Component changed from dhcp to dhcp6

comment:4 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to tomek

Reviewed commit 7df26862869e043efe8b320d9b0404410f123db0.

Test works, this has fixed the problem. However, suggest that rather than:

    case DUID_LL: {
        // not supported yet
        cout << "Test not implemented for DUID-LL yet." << endl;
    }
    case DUID_UUID: // not supported yet
    default:
        cout << "Not supported duid type=" << duid_type << endl;
        ADD_FAILURE();

... combine the output via "cout" with the "ADD_FAILURE()" and "FAIL()" macros, and make a distinction between the known and unknown DUID types, i.e.

    case DUID_LL: // not supported yet
        ADD_FAILURE() << "Test not implemented for DUID-LL yet" << endl;
        break;

    case DUID_UUID: // not supported yet
        ADD_FAILURE() << "Test not implemented for DUID-UUID yet" << endl;
        break;

    default:
        FAIL() << "Not supported: DUID type = " << duid_type << endl;

comment:5 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

Modified as suggested, except:

  • DUID_LL does not cause fail. There is really no way to test if DUID-LL is valid or not. It doesn't have to match current interface are server is expected to keep DUID, despite possible hardware changes.
  • default case does not call FAIL() (fail + assert), but rather ADD_FAILURE() (fail + test continues), because we want to delete srv object, even if test fails.

Please review.

comment:6 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

Looks OK, but your point:

default case does not call FAIL() (fail + assert), but rather ADD_FAILURE() (fail + test continues), because we want to delete srv object, even if test fails.

caused me to check the test in detail. The DUID test function contains an ASSERT_TRUE() call after "srv" has been allocated: if that fails, the test will exit immediately and not clean up "srv". For that reason, it might be better to allocate the Dhcp6Srv object with:

boost::scoped_ptr<Dhcp6Srv> srv;
ASSERT_NO_THROW({
    srv.reset(new Dhcp6Srv(DHCP6_SERVER_PORT + 10000);
});

That way, if any test does cause the function to exit, the Dhcp6Srv object will be deleted.

I also note that the Solicit_basic test includes some ASSERT_XXX() tests; again, if these fail, the "srv" object will not be deleted. The use of scoped_ptr may be appropriate here as well.

comment:7 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

Good point. Updated both DUID and Solicit_basic testes.

Changes pushed to branch. I think this is now ready for merge.

comment:8 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit bb73dd2fdeff8da3a3923541495e14e18556b76a

All OK, please merge.

comment:9 Changed 8 years ago by tomek

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

Change pushed to master. Closing ticket.

Note: See TracTickets for help on using tickets.