Code review comment for lp:~lamont/maas/bug-1677741

Revision history for this message
LaMont Jones (lamont) wrote :

> This code is very difficult to review since its complexity has gone through
> the roof.
>
> I think I said this the last this was changed, but I really think we should
> break this down into multiple queries (via views, perhaps) so that individual
> pieces can be tested easier.
>
> For now, I attempted to review by comparing the added/changed tests to the
> added/changed code. I think I would like to see more specific tests to capture
> each edge case, such as:
>
> test__default_domain_is_extra_special_ipv4
> test__default_domain_is_extra_special_ipv6
These are both covered via:
    test_user_reserved_addresses_included_in_get_hostname_ip_mapping
    test_user_reserved_addresses_included_in_correct_domains
    test_user_reserved_IP_on_node_or_default_domain_not_both
I have updated them to be more clear about what they are expecting to see.

> test__domains_are_special
This is covered (among others) via:
    test_user_reserved_addresses_included_in_correct_domains

> test__attached_addresses_only_map_back_to_node
This is covered via:
    test_get_hostname_ip_mapping_handles_dnsrr_node_collision

> That way it's clearer to future maintainers how each edge case and branch
> called out in the SQL maps to a requirement for a specific behavior.

« Back to merge proposal