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
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.
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.