[matsduf]

Purpose

This PR corrects some errors for Consistency05 implementation:

• CS05_OOD_ADDR_MISMATCH had wrong msgid
• Wrong argument name was used

Context

Issue #1559.

How to test this PR

Correct messages (msgids) should be displayed.

[marc-vanderwal]

I preferred using __ over __x if the msgid has no arguments to interpolate. Other than that, it seems that __x was used correctly.

This PR could also have been an opportunity to add to the scenario .t file the appropriate tests for the message arguments, like t/test-Zone09.t does.

[matsduf]

@marc-vanderwal!

I preferred using __ over __x if the msgid has no arguments to interpolate. Other than that, it seems that __x was used correctly.

I checked if that could be the case, but __ is (was) not consistently used for msgid without argument. Do you want me to revert that change?

This PR could also have been an opportunity to add to the scenario .t file the appropriate tests for the message arguments, like t/test-Zone09.t does.

Well, then it is more than a bug fix. I could be in the same PR or in a new one. I will check how to do it.

Else, did you see any problem with the PR?

[marc-vanderwal]

Writing a failing unit test that reproduces the bug is the best way to make sure that we’ve actually fixed the bug and don’t reintroduce it later on.

diff --git a/t/Test-consistency05.t b/t/Test-consistency05.t
index a640e824..cac4c226 100644
--- a/t/Test-consistency05.t
+++ b/t/Test-consistency05.t
@@ -155,7 +155,13 @@ scenario 'NO-NS-ADDR-CHILD-2' => sub {
 scenario 'OOD-ADDR-MISMATCH' => sub {
     zone 'child.{SCENARIO}.{TESTCASE}.xa';

-    expect 'CS05_OOD_ADDR_MISMATCH';
+    expect 'CS05_OOD_ADDR_MISMATCH' => {
+        nsname => 'ns1.sibbling.ood-addr-mismatch.consistency05.xa',
+        ns_ip_list_glue => '127.14.5.39;'
+                         . 'fda1:b2:c3:0:127:14:5:39',
+        ns_ip_list_lookup => '127.14.5.33;'
+                           . 'fda1:b2:c3:0:127:14:5:33',
+    };
     forbid_others;
 };

(I’m not entirely sure that the arguments’ values I’m expecting in the unit test are correct. Feel free to fix them for me.)

When I check out your branch and apply my changes to have the failing unit test, I realize that there is another problem with the same code.

    #   Failed test 'Messages of tag 'CS05_OOD_ADDR_MISMATCH' exist with specified arguments'
    #   at t/Test-consistency05.t line 158.
    # Looked for a message whose tag is 'CS05_OOD_ADDR_MISMATCH'
    #   where 'ns_ip_list_glue' equals '127.14.5.39;fda1:b2:c3:0:127:14:5:39'
    #     and 'ns_ip_list_lookup' equals '127.14.5.33;fda1:b2:c3:0:127:14:5:33'
    #     and 'nsname' equals 'ns1.sibbling.ood-addr-mismatch.consistency05.xa'
    #     and contains no argument other than those listed above
    # Here are all messages that unsuccessfully matched:
    #   CS05_OOD_ADDR_MISMATCH ns_ip_list_glue=3;3; ns_ip_list_lookup=3;3; nsname="ns1.sibbling.ood-addr-mismatch.consistency05.xa"

Notice the arguments of the last line.

Let’s not change what isn’t strictly necessary to change to fix the bug.

[marc-vanderwal]

Okay, basically you also need this:

diff --git a/lib/Zonemaster/Engine/Test/Consistency.pm b/lib/Zonemaster/Engine/Test/Consistency.pm
index 90ca034d..5699b1de 100644
--- a/lib/Zonemaster/Engine/Test/Consistency.pm
+++ b/lib/Zonemaster/Engine/Test/Consistency.pm
@@ -1057,8 +1057,8 @@ sub consistency05 {
             if ( not $set->equals( $lookup ) ) {
                 push @results, _emit_log( CS05_OOD_ADDR_MISMATCH => {
                     nsname => $n,
-                    ns_ip_list_glue => join( ";", map { $_->address() } $set->sorted_items() ),
-                    ns_ip_list_lookup => join( ";", map { $_->address() } $lookup->sorted_items() )
+                    ns_ip_list_glue => join( ";", map { $_->address()->short() } $set->sorted_items() ),
+                    ns_ip_list_lookup => join( ";", map { $_->address()->short() } $lookup->sorted_items() )
                 } );
             }
         }

Then the failing test I mentioned in my previous comment passes.