dhcp6 Solicit_basic test fails

[jinmei]

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.

[tomek]

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.

[stephen]

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;

[tomek]

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.

[stephen]

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.

[tomek]

Good point. Updated both DUID and Solicit_basic testes.

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

[stephen]

Reviewed commit bb73dd2fdeff8da3a3923541495e14e18556b76a

All OK, please merge.

[tomek]

Change pushed to master. Closing ticket.