Skip to content

Retain the possibility to start a Test Case disabled in the profile #361

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.

Sign up for GitHub

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

Merged
tgreenx merged 6 commits into from
Dec 6, 2023
Merged

Retain the possibility to start a Test Case disabled in the profile #361

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c5e4ca5
Retain the possibility to start a Test Case disabled in the profile
tgreenx Nov 28, 2023
a66d559
Small refactoring in --test
tgreenx Nov 29, 2023
08c2a77
Update after review - refactoring in --test
tgreenx Dec 4, 2023
5c68764
Update after review - more refactoring of --test
tgreenx Dec 5, 2023
583157e
Fix newline for --test in human readable output
tgreenx Dec 5, 2023
d640b9c
Respect profile configuration when there are already test cases of a …
tgreenx Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 93 additions & 19 deletions lib/Zonemaster/CLI.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use Encode;
use Readonly;
use File::Slurp;
use JSON::XS;
use List::Util qw[max];
use List::Util qw[max uniq];
use POSIX qw[setlocale LC_MESSAGES LC_CTYPE];
use Scalar::Util qw[blessed];
use Socket qw[AF_INET AF_INET6];
Expand Down Expand Up @@ -407,6 +407,90 @@ sub run {
Zonemaster::Engine::Profile->effective->merge( $profile );
}

my @testing_suite;
if ( $self->test and @{ $self->test } > 0 ) {
my %existing_tests = Zonemaster::Engine->all_methods;
my @existing_test_modules = keys %existing_tests;
my @existing_test_cases = map { @{ $existing_tests{$_} } } @existing_test_modules;

foreach my $t ( @{ $self->test } ) {
# There should be at most one slash character
if ( $t =~ tr/\/// > 1 ) {
die __( "Error: Invalid input '$t' in --test. There must be at most one slash ('/') character.\n");
}

# The case does not matter
$t = lc( $t );

my ( $module, $method );
# Fully qualified module and test case (e.g. Example/example12), or just a test case (e.g. example12). Note the different capturing order.
if ( ( ($module, $method) = $t =~ m#^ ( [a-z]+ ) / ( [a-z]+[0-9]{2} ) $#ix )
or
( ($method, $module) = $t =~ m#^ ( ( [a-z]+ ) [0-9]{2} ) $#ix ) )
{
# Check that test module exists
if ( grep( /^$module$/, map { lc($_) } @existing_test_modules ) ) {
# Check that test case exists
if ( grep( /^$method$/, @existing_test_cases ) ) {
push @testing_suite, "$module/$method";
}
else {
die __( "Error: Unrecognized test case '$method' in --test. Use --list-tests for a list of valid choices.\n" );
}
}
else {
die __( "Error: Unrecognized test module '$module' in --test. Use --list-tests for a list of valid choices.\n" );
}
}
# Just a module name (e.g. Example) or something invalid.
else {
$t =~ s{/$}{};
# Check that test module exists
if ( grep( /^$t$/, map { lc($_) } @existing_test_modules ) ) {
push @testing_suite, $t;
}
else {
die __( "Error: Invalid input '$t' in --test.\n" );
}
}
}

# Start with all profile-enabled test cases
my @actual_test_cases = @{ Zonemaster::Engine::Profile->effective->get( 'test_cases' ) };

# Derive test module from each profile-enabled test case
my %actual_test_modules;
foreach my $t ( @actual_test_cases ) {
my ( $module ) = $t =~ m#^ ( [a-z]+ ) [0-9]{2} $#ix;
$actual_test_modules{$module} = 1;
}

# Check if more test cases need to be included in the profile
foreach my $t ( @testing_suite ) {
# Either a module/method, or just a module
my ( $module, $method ) = split('/', $t);
if ( $method ) {
# Test case in not already in the profile, we add it explicitly and notify the user
if ( not grep( /^$method$/, @actual_test_cases ) ) {
say $fh_diag __x( "Notice: Engine does not have test case '$method' enabled in the profile. Forcing...");
push @actual_test_cases, $method;
}
}
else {
# No test case from this module is already in the profile, we can add them all
if ( not grep( /^$module$/, keys %actual_test_modules ) ) {
# Get the test module with the right case
( $module ) = grep { lc( $module ) eq lc( $_ ) } @existing_test_modules;
# No need to bother to check for duplicates here
push @actual_test_cases, @{ $existing_tests{$module} };
}
Comment on lines +481 to +486
Copy link
Member

@mattias-p mattias-p Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new feature, right? It's nice though.

Copy link
Contributor

@matsduf matsduf Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not fully consistent, but maybe good enough for now? The most common use case for --test is probably when all test cases are listed in the profile, and for that is seems to work as it should.

The inconsistency is when the profile only lists some test cases.

My profile has only Zone09 listed.

$ cat ~/.zonemaster/profile-zone09-only.json | jq .test_cases
[
  "zone09"
]

When no --test is given the behavior is as expected:

$ zonemaster-cli --profile ~/.zonemaster/profile-zone09-only.json --show-testcase --level info iis.se --raw
Loading profile from /home/matsd/.zonemaster/profile-zone09-only.json.
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v4.7.3
   2.14 INFO     Basic01        B01_PARENT_FOUND  domain=se; ns_ip_list=a.ns.se/192.36.144.107;a.ns.se/2a01:3f0:0:301::53;b.ns.se/192.36.133.107;b.ns.se/2001:67c:254c:301::53;c.ns.se/192.36.135.107;c.ns.se/2001:67c:2554:301::53;f.ns.se/192.71.53.53;f.ns.se/2a01:3f0:0:305::53;g.ns.se/130.239.5.114;g.ns.se/2001:6b0:e:3::1;i.ns.se/194.146.106.22;i.ns.se/2001:67c:1010:5::53;m.ns.se/194.0.11.112;m.ns.se/2001:678:e:112::53;x.ns.se/2001:67c:124c:e000::4;x.ns.se/213.108.25.4;y.ns.se/185.159.197.150;y.ns.se/2620:10a:80aa::150;z.ns.se/185.159.198.150;z.ns.se/2620:10a:80ab::150
   2.14 INFO     Basic01        B01_CHILD_FOUND  domain=iis.se
   2.59 INFO     Basic02        B02_AUTH_RESPONSE_SOA  domain="iis.se"; ns_list=nsa.dnsnode.net/194.58.192.46;nsa.dnsnode.net/2a01:3f1:46::53;nsp.dnsnode.net/194.58.198.32;nsp.dnsnode.net/2a01:3f1:3032::53;nsu.dnsnode.net/185.42.137.98;nsu.dnsnode.net/2a01:3f0:400::32
   2.59 INFO     Unspecified    HAS_NAMESERVER_NO_WWW_A_TEST  zname="iis.se"
   2.68 INFO     Zone09         Z09_MX_DATA  mailtarget_list=mx1.iis.se.;mx2.iis.se.; ns_ip_list=194.58.198.32;185.42.137.98;2a01:3f1:3032::53;2a01:3f1:46::53;194.58.192.46;2a01:3f0:400::32

If --test Address is selcted, then all Address test cases are run and unexpectedly no warning that they are not listed in the profile:

$  zonemaster-cli --profile ~/.zonemaster/profile-zone09-only.json --show-testcase --level info iis.se --test Address --raw
Loading profile from /home/matsd/.zonemaster/profile-zone09-only.json.
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v4.7.3
   0.51 INFO     Address01      NO_IP_PRIVATE_NETWORK  
   2.23 INFO     Address02      NAMESERVERS_IP_WITH_REVERSE  
   2.24 INFO     Address03      NAMESERVER_IP_PTR_MATCH

If --test Zone01 is selected only that test case is run with a warning that it is not listed in the profile:

$  zonemaster-cli --profile ~/.zonemaster/profile-zone09-only.json --show-testcase --level info iis.se --raw --test Zone01
Loading profile from /home/matsd/.zonemaster/profile-zone09-only.json.
Notice: Engine does not have test case 'zone01' enabled in the profile. Forcing...
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v4.7.3
   0.59 INFO     Zone01         Z01_MNAME_NOT_IN_NS_LIST  nsname=ns.nic.se

If --test Zone is selected unexpectedly only Zone09 is run:

$ zonemaster-cli --profile ~/.zonemaster/profile-zone09-only.json --show-testcase --level info iis.se --raw --test Zone
Loading profile from /home/matsd/.zonemaster/profile-zone09-only.json.
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v4.7.3
   0.58 INFO     Zone09         Z09_MX_DATA  mailtarget_list=mx2.iis.se.;mx1.iis.se.; ns_ip_list=185.42.137.98;2a01:3f1:3032::53;2a01:3f0:400::32;2a01:3f1:46::53;194.58.198.32;194.58.192.46

Copy link
Contributor

@matsduf matsduf Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ready to approve if the behavior above is accepted.

Copy link
Contributor Author

@tgreenx tgreenx Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not fully consistent, but maybe good enough for now? The most common use case for --test is probably when all test cases are listed in the profile, and for that is seems to work as it should.

The inconsistency is when the profile only lists some test cases.

All of the scenarios that you mention are expected behaviors. The warning was supposed to appear only when wanting to run a single test case that is not in the profile (i.e. using Zonemaster::Engine::test_method()). Running a test module (i.e. using Zonemaster::Engine::test_module()) will either only run the test cases of that module if any are specified in the profile, else all of them.

This is a new feature, right? It's nice though.

It is not, this is to keep the behavior of current develop (i.e. see your comment).

Copy link
Contributor

@matsduf matsduf Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running a test module (i.e. using Zonemaster::Engine::test_module()) will either only run the test cases of that module if any are specified in the profile, else all of them.

Well, I find it strange that all Addresses are run when none of them are included in the profile, but only one Zone test case because that single test case is included in the profile. That is inconsistent.

The behavior is not bad enough to block it, but I think it should be straighten out in next release.

Copy link
Contributor Author

@tgreenx tgreenx Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is debatable. We can look at it for next release yes.

}
}

# Configure Engine to include all of the required test cases in the profile
Zonemaster::Engine::Profile->effective->set( 'test_cases', [ uniq sort @actual_test_cases ] );
}

# These two must come after any profile from command line has been loaded
# to make any IPv4/IPv6 option override the profile setting.
if ( defined ($self->ipv4) ) {
Expand All @@ -416,7 +500,6 @@ sub run {
Zonemaster::Engine::Profile->effective->set( q{net.ipv6}, 0+$self->ipv6 );
}


if ( $self->dump_profile ) {
do_dump_profile();
}
Expand Down Expand Up @@ -548,7 +631,7 @@ sub run {
}
);

if ( $self->profile ) {
if ( $self->profile or $self->test ) {
# Separate initialization from main output in human readable output mode
print "\n" if $fh_diag eq *STDOUT;
}
Expand Down Expand Up @@ -631,31 +714,22 @@ sub run {
# Actually run tests!
eval {
if ( $self->test and @{ $self->test } > 0 ) {
my $zone = Zonemaster::Engine->zone( $domain );
foreach my $t ( @{ $self->test } ) {
# The case does not matter
$t = lc( $t );
# Fully qualified module and test case (e.g. Example/example12)
if (my ($module, $method) = $t =~ m#^ ( [a-z]+ ) / ( [a-z]+[0-9]{2} ) $#ix) {
Zonemaster::Engine->test_method( $module, $method, $zone );
foreach my $t ( @testing_suite ) {
# Either a module/method, or just a module
my ( $module, $method ) = split('/', $t);
if ( $method ) {
Zonemaster::Engine->test_method( $module, $method, $domain );
}
# Just a test case (e.g. example12). Note the different capturing order.
elsif (($method, $module) = $t =~ m#^ ( ( [a-z]+ ) [0-9]{2} ) $#ix) {
Zonemaster::Engine->test_method( $module, $method, $zone );
}
# Just a module name (e.g. Example) or something invalid.
# TODO: in case of invalid input, print a proper error message
# suggesting to use --list-tests for valid choices.
else {
$t =~ s{/$}{};
Zonemaster::Engine->test_module( $t, $domain );
Zonemaster::Engine->test_module( $module, $domain );
}
}
}
else {
Zonemaster::Engine->test_zone( $domain );
}
};

if ( not $self->raw and not $self->json ) {
if ( not $printed_something ) {
say __( "Looks OK." );
Expand Down