-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add MethodsV2 unit testing #1350
Conversation
TODO in this PR:
|
3f8dc24
to
d9f3d0d
Compare
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. |
We could limit the data in the
From "Get delegation NS names and IP addresses" the following two can be extracted:
From "Get zone NS names and IP addresses" the following two can be extracted:
We should verify that MethodsV2 do that extraction correctly, but maybe not for every scenario. |
639c39e
to
2ba293c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine, though I have some minor suggestions.
t/TestUtil.pm
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
t/methodsv2.t
Outdated
[ 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 ) ], | ||
[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be much nicer to read if this data is on multiple lines, like this:
[ 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 ) ], | |
[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, updated.
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. |
For scenarios for test cases it is possible to test and trouble shoot with the help of |
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. |
I tested by changing the correct to an incorrect. I tested scenario In I got the following output:
I expects it to say that the look-up data gives "127.40.1.41" that was not found in the expected data.
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. |
I removed one IP address from
It is not very helpful. Are there too many in Then I also changed one address in
|
Scenario GOOD-6 fails. See #1351. |
IB-NOT-IN-ZONE-1 fails and also triggers "Use of uninitialized value".
|
I suggest that the error message is something like:
"Unexpected" when not in the expected set, but given by the method. "Missing" when in the expected set, but not given by the method. |
sub perform_methodsv2_testing { | ||
my ( %subtests ) = @_; | ||
|
||
my @untested_scenarios = (); | ||
|
||
for my $scenario ( sort ( keys %subtests ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if ( $testable != 1 and $testable != 0 ) { | ||
croak "Scenario $scenario: Value of testable must be 0 or 1"; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Zonemaster::Engine::Profile->effective->set( q{no_network}, 1 ); | ||
} | ||
|
||
perform_methodsv2_testing( %subtests ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@matsduf I added a fix (48ff5dc) to a crash in method See zone
Before fix:
After fix:
|
Your request for a scenario when "an out-of-bailiwick nameserver had no IP address" could be interpreted as either of the following two:
For alternative 1 there is already scenario GOOD-2. For alternative 2 there are no scenarios, but I will add such scenarios. "Has no IP address" can be either NODATA or NXDOMAIN and I will create both. If the zone has two NS it could be on one or both. There are many combinations! |
@@ -69,7 +69,7 @@ This Method will obtain the IP addresses of the Out-Of-Bailiwick name servers fo | |||
|
|||
Takes a Zonemaster::Engine::Zone object (C<$zone>) and an arrayref of Zonemaster::Engine::Nameserver objects (C<$ns_names_ref>). | |||
|
|||
Returns an arrayref of Zonemaster::Engine::Nameserver objects. | |||
Returns an arrayref of Zonemaster::Engine::Nameserver and/or Zonemaster::Engine::DNSName objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two suggestions:
- Wrap module names in
L<…>
so that they become hyperlinks when viewing the documentation online (e.g. on Metacpan); - Make it clearer when the function returns a Zonemaster::Engine::Nameserver object and when it returns a Zonemaster::Engine::DNSName.
Returns an arrayref of Zonemaster::Engine::Nameserver and/or Zonemaster::Engine::DNSName objects. | |
Returns an arrayref of L<Zonemaster::Engine::Nameserver> objects for each name server name that was successfully resolved to an IP address, and L<Zonemaster::Engine::DNSName> objects for each name server name that could not be resolved to an IP address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestions, updated.
Returns an arrayref of Zonemaster::Engine::Nameserver objects, or `undef` if no parent zone was found. | ||
Returns an arrayref of Zonemaster::Engine::Nameserver and/or Zonemaster::Engine::DNSName objects, or `undef` if no parent zone was found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same suggestion as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Returns an arrayref of Zonemaster::Engine::Nameserver objects, or `undef` if no parent zone was found. | ||
Returns an arrayref of Zonemaster::Engine::Nameserver and/or Zonemaster::Engine::DNSName objects, or `undef` if no parent zone was found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same suggestion as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
@tgreenx, there is a textual conflict in |
- 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
4a09f61
to
92e7df4
Compare
One scenario (GOOD-UNDEL-1) has been disabled while it awaits further investigation.
Another PR (#1351) builds on top of this PR, and the work for MethodsV2 unit testing will be continued there. Merging as is. |
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
Zonemaster::Engine::TestMethodsV2
How to test this PR
Unit tests are updated and should pass.