Add MethodsV2 unit testing #1350

tgreenx · 2024-05-22T09:52:57Z

Purpose

This PR adds MethodsV2 unit testing, based on test zone specification from zonemaster/zonemaster#1254. All external methods are covered.

Context

Test zone specification: zonemaster/zonemaster#1254

Changes

Add unit testing for MethodsV2
Minor fix in Zonemaster::Engine::TestMethodsV2

How to test this PR

Unit tests are updated and should pass.

tgreenx · 2024-05-22T09:55:24Z

TODO in this PR:

Discuss the current implementation design
Add data checks
Add documentation

…TestMethodsV2

matsduf · 2024-05-22T11:30:55Z

I disagree that only testing "Get zone NS names and IP addresses" gives a full coverage. The extraction of parent IPs is not tested. Delegation and zone do not need to match. Delegation can be with or without glue or parts of glue.

matsduf · 2024-05-22T11:41:14Z

We could limit the data in the t file to be

Get parent NS IP addresses
Get delegation NS names and IP addresses
Get zone NS names and IP addresses

From "Get delegation NS names and IP addresses" the following two can be extracted:

Get delegation NS names
Get delegation NS IP addresses

From "Get zone NS names and IP addresses" the following two can be extracted:

Get zone NS names
Get zone NS IP addresses

We should verify that MethodsV2 do that extraction correctly, but maybe not for every scenario.

tgreenx · 2024-05-23T14:24:39Z

TODO in this PR:

* [x]  Discuss the current implementation design

* [x]  Add data checks

* [x]  Add documentation

All done in latest commit 2ba293c. I'm marking this PR as ready for review. @matsduf your comments have been addressed.

- Expand unit test coverage to all external Zonemaster::Engine::Test::TestMethodsV2 methods - Add input data checks - Update unit test data - Update documentation - Minor fixes and refactoring

marc-vanderwal

Looks fine, though I have some minor suggestions.

marc-vanderwal · 2024-05-24T08:43:00Z

t/TestUtil.pm

+The arrays of mandatory message tags and forbidden message tags, but not both, could be undefined.
+If the mandatory message tag array is undefined, then it will be generated to contain all message
+tags not included in the forbidden message tag array. The same mechanism is used if the forbidden
+message tag array is undefined.


Is this change supposed to be part of this pull request? I seem to have reviewed a similar passage before and suggesting an improvement. After sleeping on it, I think I’ve come up with something better still.

Suggested change

The arrays of mandatory message tags and forbidden message tags, but not both, could be undefined.

If the mandatory message tag array is undefined, then it will be generated to contain all message

tags not included in the forbidden message tag array. The same mechanism is used if the forbidden

message tag array is undefined.

It is possible to pass C<undef> instead of the mandatory message tag or forbidden message tag arrayref.

In that case, it is equivalent to an arrayref containing C<@$aref_alltags> minus the tags in the other array.

In other words, passing C<undef> as the forbidden message tag arrayref means that any tag not explicitly

allowed must not be emitted, while passing C<undef> as the mandatory message tag arrayref means

that any tag not explicitly forbidden must be emitted.

It is an error if both of the aforementioned arrayrefs are simultaneously undefined or simultaneously empty.

marc-vanderwal · 2024-05-24T08:46:47Z

t/methodsv2.t

+        [ qw( 127.40.3.21 fda1:b2:c3:0:127:40:3:21 127.40.3.22 fda1:b2:c3:0:127:40:3:22 ) ],
+        [ qw( ns1.child.parent.good-1.methodsv2.xa/127.40.4.21 ns1.child.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:4:21 ns2.child.parent.good-1.methodsv2.xa/127.40.4.22 ns2.child.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:4:22 ) ],
+        [ qw( ns1.child.parent.good-1.methodsv2.xa/127.40.4.21 ns1.child.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:4:21 ns2.child.parent.good-1.methodsv2.xa/127.40.4.22 ns2.child.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:4:22 ) ],
+        []


It would be much nicer to read if this data is on multiple lines, like this:

Suggested change

[ qw( 127.40.3.21 fda1:b2:c3:0:127:40:3:21 127.40.3.22 fda1:b2:c3:0:127:40:3:22 ) ],

[ qw( ns1.child.parent.good-1.methodsv2.xa/127.40.4.21 ns1.child.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:4:21 ns2.child.parent.good-1.methodsv2.xa/127.40.4.22 ns2.child.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:4:22 ) ],

[ qw( ns1.child.parent.good-1.methodsv2.xa/127.40.4.21 ns1.child.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:4:21 ns2.child.parent.good-1.methodsv2.xa/127.40.4.22 ns2.child.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:4:22 ) ],

[]

[ qw( 127.40.3.21 fda1:b2:c3:0:127:40:3:21

127.40.3.22 fda1:b2:c3:0:127:40:3:22 ) ],

[ qw( ns1.child.parent.good-1.methodsv2.xa/127.40.4.21

ns1.child.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:4:21

ns2.child.parent.good-1.methodsv2.xa/127.40.4.22

ns2.child.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:4:22 ) ],

[ qw( ns1.child.parent.good-1.methodsv2.xa/127.40.4.21

ns1.child.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:4:21

ns2.child.parent.good-1.methodsv2.xa/127.40.4.22

ns2.child.parent.good-1.methodsv2.xa/fda1:b2:c3:0:127:40:4:22 ) ],

[]

matsduf · 2024-05-26T16:32:29Z

I have realized that I have done less optimal selection of IP addresses for the scenarios. The selection principle is also different for the test scenarios for test cases. I am about to renumber, both in zonemaster/zonemaster#1254 and as a PR to this PR.

matsduf · 2024-05-26T19:22:25Z

For scenarios for test cases it is possible to test and trouble shoot with the help of zonemaster-cli, but that is not possible for the MethodsV2 scenarios. Would it be possible to at a "verbose" mode for the t file in which only one scenario is run and verbose information is outputted, e.g. expected name servers and found name servers, respectively?

matsduf · 2024-05-26T21:17:34Z

I have created tgreenx#2 to update this PR with the new IP addresses from an update of zonemaster/zonemaster#1254. A new data file is also created.

matsduf · 2024-05-27T08:33:40Z

I tested by changing the correct to an incorrect. I tested scenario GOOD-1.

In EXPECTED_PARENT_IP I changed "127.40.1.41" to "127.40.2.41". ("1" to "2".) In EXPECTED_DEL_NS I changed "ns1.child.parent.good-1.methodsv2.xa/127.40.1.51" to "ns1.childs.parent.good-1.methodsv2.xa/127.40.1.51". ("child" to "childs".)

I got the following output:

        #   Failed test 'Nameserver IP '127.40.2.41' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 270.
        # Nameserver IP '127.40.2.41' should have been present, but wasn't
        # Looks like you failed 1 test of 6.

    #   Failed test 'get_parent_ns_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 278.

I expects it to say that the look-up data gives "127.40.1.41" that was not found in the expected data.

        #   Failed test 'Nameserver 'ns1.childs.parent.good-1.methodsv2.xa/127.40.1.51' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 291.
        # Nameserver 'ns1.childs.parent.good-1.methodsv2.xa/127.40.1.51' should have been present, but wasn't
        # Looks like you failed 1 test of 6.

    #   Failed test 'get_del_ns_names_and_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 299.

Same here. "ns1.child.parent.good-1.methodsv2.xa/127.40.1.51" is what we get from the look-up, but we do find it in the expected data.

matsduf · 2024-05-27T08:44:21Z

I removed one IP address from EXPECTED_PARENT_IP and got the following error message:

        #   Failed test at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 273.
        # Number of nameserver IP addresses in both arrays does not match
        # Looks like you failed 1 test of 5.

    #   Failed test 'get_parent_ns_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 278.
    # Looks like you failed 1 test of 7.
t/methodsv2.t .. 4/? 
#   Failed test 'GOOD-1'

It is not very helpful. Are there too many in EXPECTED_PARENT_IP or too few? Which is the problematic IP address?

Then I also changed one address in EXPECTED_PARENT_IP but that information is not provided.

        #   Failed test at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 273.
        # Number of nameserver IP addresses in both arrays does not match
        # Looks like you failed 1 test of 5.

    #   Failed test 'get_parent_ns_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 278.
    # Looks like you failed 1 test of 7.

#   Failed test 'GOOD-1'

matsduf · 2024-05-27T21:56:15Z

Scenario GOOD-6 fails. See #1351.

matsduf · 2024-05-30T08:39:55Z

IB-NOT-IN-ZONE-1 fails and also triggers "Use of uninitialized value".

t/methodsv2.t .. 11/? 
        #   Failed test 'Nameserver 'ns1.child.parent.ib-not-in-zone-1.methodsv2.xa' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 291.
        # Nameserver 'ns1.child.parent.ib-not-in-zone-1.methodsv2.xa' should have been present, but wasn't

        #   Failed test 'Nameserver 'ns2.child.parent.ib-not-in-zone-1.methodsv2.xa' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 291.
        # Nameserver 'ns2.child.parent.ib-not-in-zone-1.methodsv2.xa' should have been present, but wasn't

        #   Failed test at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 294.
        # Number of nameservers in both arrays does not match
        # Looks like you failed 3 tests of 4.

    #   Failed test 'get_zone_ns_names_and_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 299.
Use of uninitialized value $expected_ip in concatenation (.) or string at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 339.

        #   Failed test 'Nameserver IP '' is present'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 339.
Use of uninitialized value $expected_ip in concatenation (.) or string at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 339.
        # Nameserver IP '' should have been present, but wasn't

        #   Failed test at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 342.
        # Number of nameserver IPs in both arrays does not match
        # Looks like you failed 2 tests of 3.

    #   Failed test 'get_zone_ns_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 347.
    # Looks like you failed 2 tests of 7.
t/methodsv2.t .. 14/? 
#   Failed test 'IB-NOT-IN-ZONE-1'

matsduf · 2024-06-03T11:58:16Z

I suggest that the error message is something like:

Unexpected: 127.40.3.21 
Unexpected: fda1:b2:c3:0:127:40:3:21
Missing: 127.40.3.22
Missing: fda1:b2:c3:0:127:40:3:22

"Unexpected" when not in the expected set, but given by the method. "Missing" when in the expected set, but not given by the method.

matsduf · 2024-06-03T14:56:14Z

t/TestUtil.pm

+sub perform_methodsv2_testing {
+    my ( %subtests ) = @_;
+
+    my @untested_scenarios = ();
+
+    for my $scenario ( sort ( keys %subtests ) ) {


I suggest the following to support testing one scenario:

sub perform_methodsv2_testing { my ( $href_subtests, $single_scenario ) = @_; my %subtests = %$href_subtests; my @untested_scenarios = (); for my $scenario ( sort ( keys %subtests ) ) { next if $single_scenario and $scenario ne $single_scenario;

This will require changes to the methodsv2.t too. See that.

The documentation should be updated too.

matsduf · 2024-06-03T14:58:12Z

t/TestUtil.pm

+        if ( $testable != 1 and $testable != 0 ) {
+            croak "Scenario $scenario: Value of testable must be 0 or 1";
+        }
+


Depends on the change above, and will test a single scenario even if not testable. Change to:

if ( $testable != 1 and $testable != 0 ) { croak "Scenario $scenario: Value of testable must be 0 or 1"; } $testable = 1 if $single_scenario and $scenario eq $single_scenario;

matsduf · 2024-06-03T15:00:36Z

t/TestUtil.pm

+                    ok( defined $res, "Result is defined" ) or diag "Unexpected undefined result";
+                    foreach my $expected_ip ( @{ $expected_parent_ip } ) {
+                        ok( grep( /^$expected_ip$/, uniq map { $_->address->short } @{ $res } ), "Nameserver IP '$expected_ip' is present" )
+                            or diag "Nameserver IP '$expected_ip' should have been present, but wasn't";


If change to the following it will be much easier to read:

or diag "Missing: $expected_ip";

However, what is missing from the error messages are the IP addresses found but are not in the expected set.

matsduf · 2024-06-03T15:02:54Z

t/methodsv2.t

+    Zonemaster::Engine::Profile->effective->set( q{no_network}, 1 );
+}
+
+perform_methodsv2_testing( %subtests );


This depends on the changes in t/TestUtil.t. See those.

perform_methodsv2_testing( \%subtests, $ENV{ZONEMASTER_MOTHODSV2_SCENARIO} );

The documentation should also be updated.

tgreenx added FA-MethodV2 Focus Area: Implementing and migrating to MethodNT for test cases. V-Patch Versioning: The change gives an update of patch in version. labels May 22, 2024

tgreenx added this to the v2024.1 milestone May 22, 2024

tgreenx requested a review from matsduf May 22, 2024 09:52

Add unit testing for MethodsV2, and minor fix in Zonemaster::Engine::…

d9f3d0d

…TestMethodsV2

tgreenx force-pushed the methodsv2-unit-tests branch from 3f8dc24 to d9f3d0d Compare May 22, 2024 10:00

tgreenx marked this pull request as ready for review May 23, 2024 14:25

tgreenx requested review from mattias-p and marc-vanderwal May 23, 2024 14:25

Update MethodsV2 unit testing

2ba293c

- Expand unit test coverage to all external Zonemaster::Engine::Test::TestMethodsV2 methods - Add input data checks - Update unit test data - Update documentation - Minor fixes and refactoring

tgreenx force-pushed the methodsv2-unit-tests branch from 639c39e to 2ba293c Compare May 23, 2024 14:42

Add two helper functions in TestUtil

e4828ae

marc-vanderwal reviewed May 24, 2024

View reviewed changes

matsduf mentioned this pull request May 27, 2024

Updates methodsv2 unit tests #1351

Draft

matsduf reviewed Jun 3, 2024

View reviewed changes

tgreenx mentioned this pull request Jun 11, 2024

Create test zones and test data for MethodsV2. #1222

Open

tgreenx linked an issue Jun 11, 2024 that may be closed by this pull request

Create test zones and test data for MethodsV2. #1222

Open

tgreenx removed this from the v2024.1 milestone Jun 11, 2024

tgreenx added this to the v2024.2 milestone Jun 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MethodsV2 unit testing #1350

Add MethodsV2 unit testing #1350

tgreenx commented May 22, 2024 •

edited

Loading

tgreenx commented May 22, 2024 •

edited

Loading

matsduf commented May 22, 2024

matsduf commented May 22, 2024

tgreenx commented May 23, 2024 •

edited

Loading

marc-vanderwal left a comment

marc-vanderwal May 24, 2024

marc-vanderwal May 24, 2024

matsduf commented May 26, 2024

matsduf commented May 26, 2024

matsduf commented May 26, 2024

matsduf commented May 27, 2024 •

edited

Loading

matsduf commented May 27, 2024

matsduf commented May 27, 2024

matsduf commented May 30, 2024

matsduf commented Jun 3, 2024

matsduf Jun 3, 2024

matsduf Jun 3, 2024

matsduf Jun 3, 2024

matsduf Jun 3, 2024

-The arrays of mandatory message tags and forbidden message tags, but not both, could be undefined.
-If the mandatory message tag array is undefined, then it will be generated to contain all message
-tags not included in the forbidden message tag array. The same mechanism is used if the forbidden
-message tag array is undefined.
+It is possible to pass C<undef> instead of the mandatory message tag or forbidden message tag arrayref.
+In that case, it is equivalent to an arrayref containing C<@$aref_alltags> minus the tags in the other array.
+In other words, passing C<undef> as the forbidden message tag arrayref means that any tag not explicitly
+allowed must not be emitted, while passing C<undef> as the mandatory message tag arrayref means
+that any tag not explicitly forbidden must be emitted.
+It is an error if both of the aforementioned arrayrefs are simultaneously undefined or simultaneously empty.

Add MethodsV2 unit testing #1350

Are you sure you want to change the base?

Add MethodsV2 unit testing #1350

Conversation

tgreenx commented May 22, 2024 • edited Loading

Purpose

Context

Changes

How to test this PR

tgreenx commented May 22, 2024 • edited Loading

matsduf commented May 22, 2024

matsduf commented May 22, 2024

tgreenx commented May 23, 2024 • edited Loading

marc-vanderwal left a comment

Choose a reason for hiding this comment

marc-vanderwal May 24, 2024

Choose a reason for hiding this comment

marc-vanderwal May 24, 2024

Choose a reason for hiding this comment

matsduf commented May 26, 2024

matsduf commented May 26, 2024

matsduf commented May 26, 2024

matsduf commented May 27, 2024 • edited Loading

matsduf commented May 27, 2024

matsduf commented May 27, 2024

matsduf commented May 30, 2024

matsduf commented Jun 3, 2024

matsduf Jun 3, 2024

Choose a reason for hiding this comment

matsduf Jun 3, 2024

Choose a reason for hiding this comment

matsduf Jun 3, 2024

Choose a reason for hiding this comment

matsduf Jun 3, 2024

Choose a reason for hiding this comment

tgreenx commented May 22, 2024 •

edited

Loading

tgreenx commented May 22, 2024 •

edited

Loading

tgreenx commented May 23, 2024 •

edited

Loading

matsduf commented May 27, 2024 •

edited

Loading