Code review comment for lp:~newell-jensen/maas/fix-1665839

Revision history for this message
Newell Jensen (newell-jensen) wrote :

> On Tue, Feb 21, 2017 at 07:28:25PM -0000, Newell Jensen wrote:
> > This change is still covered by current unit tests.
>
> I don't get it -- you don't have a case which reproduces the bug, do you?

Before, the discovered_machine was being sieved based off of object equality. Something funky was going on with this equality check as I only saw the bug appear randomly. Maybe something to do with the object's __hash__ method, which Python uses to check object equality.

In any event, now that code is checking for the actual node_id (which I should have been checking for in the first place as this is ultimately what is needed) instead of checking for the new object from the list, I do not have a test to test the original bug.

However, the compose allocation error has been moved so we are testing whether or not the discovered_machine was found which tests whether or not this code fails to find a node_id for the new machine. The test_compose_raises_error_for_no_allocation tests this.

Thanks for the valid review comments. Hope all is going well for you as well Kiko :)

« Back to merge proposal