From c5e4ca5a4f278e7e2460369d7dfc8c53fa1d77aa Mon Sep 17 00:00:00 2001 From: "[Thomas Green]" Date: Tue, 28 Nov 2023 17:34:46 +0100 Subject: [PATCH 1/6] Retain the possibility to start a Test Case disabled in the profile https://github.com/zonemaster/zonemaster-engine/pull/1294 removes the possibility for Zonemaster-Engine to start a Test Case which is explicitly disabled in its profile. This commit retains the possibility by letting Zonemaster-CLI directly modify the profile, and notifying the user with a notice message. --- lib/Zonemaster/CLI.pm | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/Zonemaster/CLI.pm b/lib/Zonemaster/CLI.pm index 1969401..3398109 100755 --- a/lib/Zonemaster/CLI.pm +++ b/lib/Zonemaster/CLI.pm @@ -416,7 +416,6 @@ sub run { Zonemaster::Engine::Profile->effective->set( q{net.ipv6}, 0+$self->ipv6 ); } - if ( $self->dump_profile ) { do_dump_profile(); } @@ -637,10 +636,20 @@ sub run { $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) { + if ( not grep( /^$method$/, @{ Zonemaster::Engine::Profile->effective->get( 'test_cases' ) } ) ) { + say $fh_diag __x( "Notice: Engine does not have test case '$method' enabled in the profile. Forcing..."); + Zonemaster::Engine::Profile->effective->set( 'test_cases', [ "$method" ] ); + } + Zonemaster::Engine->test_method( $module, $method, $zone ); } # Just a test case (e.g. example12). Note the different capturing order. elsif (($method, $module) = $t =~ m#^ ( ( [a-z]+ ) [0-9]{2} ) $#ix) { + if ( not grep( /^$method$/, @{ Zonemaster::Engine::Profile->effective->get( 'test_cases' ) } ) ) { + say $fh_diag __x( "Notice: Engine does not have test case '$method' enabled in the profile. Forcing..."); + Zonemaster::Engine::Profile->effective->set( 'test_cases', [ "$method" ] ); + } + Zonemaster::Engine->test_method( $module, $method, $zone ); } # Just a module name (e.g. Example) or something invalid. From a66d5592b01b85833e718a96190c6b98fb305f6b Mon Sep 17 00:00:00 2001 From: "[Thomas Green]" Date: Wed, 29 Nov 2023 11:24:56 +0100 Subject: [PATCH 2/6] Small refactoring in --test --- lib/Zonemaster/CLI.pm | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/lib/Zonemaster/CLI.pm b/lib/Zonemaster/CLI.pm index 3398109..86fee2a 100755 --- a/lib/Zonemaster/CLI.pm +++ b/lib/Zonemaster/CLI.pm @@ -630,27 +630,21 @@ 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) { + # Fully qualified module and test case (e.g. Example/example12), or just a test case (e.g. example12). Note the different capturing order. + my ( $module, $method ); + if ( ( ($module, $method) = $t =~ m#^ ( [a-z]+ ) / ( [a-z]+[0-9]{2} ) $#ix ) + or + ( ($method, $module) = $t =~ m#^ ( ( [a-z]+ ) [0-9]{2} ) $#ix ) ) + { if ( not grep( /^$method$/, @{ Zonemaster::Engine::Profile->effective->get( 'test_cases' ) } ) ) { say $fh_diag __x( "Notice: Engine does not have test case '$method' enabled in the profile. Forcing..."); Zonemaster::Engine::Profile->effective->set( 'test_cases', [ "$method" ] ); } - Zonemaster::Engine->test_method( $module, $method, $zone ); - } - # Just a test case (e.g. example12). Note the different capturing order. - elsif (($method, $module) = $t =~ m#^ ( ( [a-z]+ ) [0-9]{2} ) $#ix) { - if ( not grep( /^$method$/, @{ Zonemaster::Engine::Profile->effective->get( 'test_cases' ) } ) ) { - say $fh_diag __x( "Notice: Engine does not have test case '$method' enabled in the profile. Forcing..."); - Zonemaster::Engine::Profile->effective->set( 'test_cases', [ "$method" ] ); - } - - Zonemaster::Engine->test_method( $module, $method, $zone ); + Zonemaster::Engine->test_method( $module, $method, $domain ); } # Just a module name (e.g. Example) or something invalid. # TODO: in case of invalid input, print a proper error message From 08c2a77d574ff1ab5c8850f5f1eb6357d2e406b7 Mon Sep 17 00:00:00 2001 From: "[Thomas Green]" Date: Mon, 4 Dec 2023 13:30:01 +0100 Subject: [PATCH 3/6] Update after review - refactoring in --test Move input checks earlier in the code Add input check for multiple slash characters Refactoring --- lib/Zonemaster/CLI.pm | 60 +++++++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/lib/Zonemaster/CLI.pm b/lib/Zonemaster/CLI.pm index 86fee2a..a12b6a5 100755 --- a/lib/Zonemaster/CLI.pm +++ b/lib/Zonemaster/CLI.pm @@ -407,6 +407,39 @@ sub run { Zonemaster::Engine::Profile->effective->merge( $profile ); } + my @testing_suite; + if ( $self->test and @{ $self->test } > 0 ) { + foreach my $t ( @{ $self->test } ) { + # There should be at most one slash character + if ( $t =~ tr/\/// > 1 ) { + die __( "Error: --test must have at most one slash ('/') character: $t"); + } + + # 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 ) ) + { + push @testing_suite, "$module/$method"; + if ( not grep( /^$method$/, @{ Zonemaster::Engine::Profile->effective->get( 'test_cases' ) } ) ) { + # Actual forcing will be done later in the code, i.e. just before calling Zonemaster::Engine->test_method() + say $fh_diag __x( "Notice: Engine does not have test case '$method' enabled in the profile. Forcing..."); + } + } + # 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{/$}{}; + push @testing_suite, $t; + } + } + } + # 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) ) { @@ -630,28 +663,16 @@ sub run { # Actually run tests! eval { if ( $self->test and @{ $self->test } > 0 ) { - foreach my $t ( @{ $self->test } ) { - # The case does not matter - $t = lc( $t ); - # Fully qualified module and test case (e.g. Example/example12), or just a test case (e.g. example12). Note the different capturing order. - my ( $module, $method ); - if ( ( ($module, $method) = $t =~ m#^ ( [a-z]+ ) / ( [a-z]+[0-9]{2} ) $#ix ) - or - ( ($method, $module) = $t =~ m#^ ( ( [a-z]+ ) [0-9]{2} ) $#ix ) ) - { - if ( not grep( /^$method$/, @{ Zonemaster::Engine::Profile->effective->get( 'test_cases' ) } ) ) { - say $fh_diag __x( "Notice: Engine does not have test case '$method' enabled in the profile. Forcing..."); - Zonemaster::Engine::Profile->effective->set( 'test_cases', [ "$method" ] ); - } - + foreach my $t ( @testing_suite ) { + # Either a module/method, or just a module + my ( $module, $method ) = split('/', $t); + if ( $method ) { + # Force Engine to include the requested test case in its profile + Zonemaster::Engine::Profile->effective->set( 'test_cases', [ "$method" ] ); Zonemaster::Engine->test_method( $module, $method, $domain ); } - # 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 ); } } } @@ -659,6 +680,7 @@ sub run { Zonemaster::Engine->test_zone( $domain ); } }; + if ( not $self->raw and not $self->json ) { if ( not $printed_something ) { say __( "Looks OK." ); From 5c687644660db9e374b937a1ab242f79d13ed990 Mon Sep 17 00:00:00 2001 From: "[Thomas Green]" Date: Tue, 5 Dec 2023 14:37:44 +0100 Subject: [PATCH 4/6] Update after review - more refactoring of --test Add input validation, based on the content of 'Zonemaster::Engine->all_methods' Construct the 'test_cases' profile property from the combination of all --test switches Refactoring --- lib/Zonemaster/CLI.pm | 56 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 10 deletions(-) diff --git a/lib/Zonemaster/CLI.pm b/lib/Zonemaster/CLI.pm index a12b6a5..c2f9d1e 100755 --- a/lib/Zonemaster/CLI.pm +++ b/lib/Zonemaster/CLI.pm @@ -409,10 +409,14 @@ sub run { 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: --test must have at most one slash ('/') character: $t"); + die __( "Error: Invalid input '$t' in --test. There must be at most one slash ('/') character.\n"); } # The case does not matter @@ -424,20 +428,54 @@ sub run { or ( ($method, $module) = $t =~ m#^ ( ( [a-z]+ ) [0-9]{2} ) $#ix ) ) { - push @testing_suite, "$module/$method"; - if ( not grep( /^$method$/, @{ Zonemaster::Engine::Profile->effective->get( 'test_cases' ) } ) ) { - # Actual forcing will be done later in the code, i.e. just before calling Zonemaster::Engine->test_method() - say $fh_diag __x( "Notice: Engine does not have test case '$method' enabled in the profile. Forcing..."); + # Check that test module exists + if ( grep( /^$module$/, map { lc($_) } @existing_test_modules ) ) { + # Check that test case exists + if ( grep( /^$method$/, @existing_test_cases ) ) { + if ( not grep( /^$method$/, @{ Zonemaster::Engine::Profile->effective->get( 'test_cases' ) } ) ) { + # Actual forcing will be done later in the code + say $fh_diag __x( "Notice: Engine does not have test case '$method' enabled in the profile. Forcing..."); + } + + 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. - # TODO: in case of invalid input, print a proper error message - # suggesting to use --list-tests for valid choices. else { $t =~ s{/$}{}; - push @testing_suite, $t; + # 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" ); + } + } + } + + my @actual_test_cases; + foreach my $t ( @testing_suite ) { + # Either a module/method, or just a module + my ( $module, $method ) = split('/', $t); + if ( $method ) { + push @actual_test_cases, $method; + } + else { + # Get the test module with the right case + ( $module ) = grep { lc( $module ) eq lc( $_ ) } @existing_test_modules; + push @actual_test_cases, @{ $existing_tests{$module} }; } } + + # Force Engine to include all of the requested test cases in the profile + Zonemaster::Engine::Profile->effective->set( 'test_cases', [ @actual_test_cases ] ); } # These two must come after any profile from command line has been loaded @@ -667,8 +705,6 @@ sub run { # Either a module/method, or just a module my ( $module, $method ) = split('/', $t); if ( $method ) { - # Force Engine to include the requested test case in its profile - Zonemaster::Engine::Profile->effective->set( 'test_cases', [ "$method" ] ); Zonemaster::Engine->test_method( $module, $method, $domain ); } else { From 583157ec5c0b4c586c611daa9189802fd02307b2 Mon Sep 17 00:00:00 2001 From: "[Thomas Green]" Date: Tue, 5 Dec 2023 15:01:06 +0100 Subject: [PATCH 5/6] Fix newline for --test in human readable output --- lib/Zonemaster/CLI.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Zonemaster/CLI.pm b/lib/Zonemaster/CLI.pm index c2f9d1e..bfee26b 100755 --- a/lib/Zonemaster/CLI.pm +++ b/lib/Zonemaster/CLI.pm @@ -618,7 +618,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; } From d640b9cde80b55c3ac8b500ecec8b00b0f3634ff Mon Sep 17 00:00:00 2001 From: "[Thomas Green]" Date: Tue, 5 Dec 2023 17:55:21 +0100 Subject: [PATCH 6/6] Respect profile configuration when there are already test cases of a requested module in the profile --- lib/Zonemaster/CLI.pm | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/lib/Zonemaster/CLI.pm b/lib/Zonemaster/CLI.pm index bfee26b..e153199 100755 --- a/lib/Zonemaster/CLI.pm +++ b/lib/Zonemaster/CLI.pm @@ -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]; @@ -432,11 +432,6 @@ sub run { if ( grep( /^$module$/, map { lc($_) } @existing_test_modules ) ) { # Check that test case exists if ( grep( /^$method$/, @existing_test_cases ) ) { - if ( not grep( /^$method$/, @{ Zonemaster::Engine::Profile->effective->get( 'test_cases' ) } ) ) { - # Actual forcing will be done later in the code - say $fh_diag __x( "Notice: Engine does not have test case '$method' enabled in the profile. Forcing..."); - } - push @testing_suite, "$module/$method"; } else { @@ -460,22 +455,40 @@ sub run { } } - my @actual_test_cases; + # 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 ) { - push @actual_test_cases, $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 { - # Get the test module with the right case - ( $module ) = grep { lc( $module ) eq lc( $_ ) } @existing_test_modules; - push @actual_test_cases, @{ $existing_tests{$module} }; + # 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} }; + } } } - # Force Engine to include all of the requested test cases in the profile - Zonemaster::Engine::Profile->effective->set( 'test_cases', [ @actual_test_cases ] ); + # 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