Skip to content
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

Updates methodsv2 unit tests #1351

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

matsduf
Copy link
Contributor

@matsduf matsduf commented May 27, 2024

Purpose

This PR provides unit tests based on test scenarios for MethodsV2, as defined in zonemaster/zonemaster#1254. It also updates t/TestUtil.pm.

  • Updates of t/TestUtil.pm:

    • Explicit error messages expected list does not match extracted by code
    • Support for distrinktion between empty list and undefined list
    • Added feature "single scenario"
    • Added feature "disable scenarios" for one run
    • Corrected bug that created undefined IP address when IP addresses were extracted from name/IP expressions an expression was name only
    • Replaced built in IP address verification against from Zonemaster library
  • Updates of t/methodsv2.t:

    • Adds more scenarios/unit tests
    • Updates unit tests
    • Adds support for running a single scenario or temporarily disable a scenario or list of scenarios
  • Updated t/methodsv2.data

When run with the code in #1373 installed most scenarios/unit tests pass, but the following will fail:

	Scenario CHILD-NS-CNAME-2 has been disabled from testing.
	Scenario CHLD-FOUND-INCONSIST-10 has been disabled from testing.
	Scenario CHLD-FOUND-INCONSIST-6 has been disabled from testing.
	Scenario CHLD-FOUND-INCONSIST-7 has been disabled from testing.
	Scenario CHLD-FOUND-INCONSIST-8 has been disabled from testing.
	Scenario CHLD-FOUND-INCONSIST-9 has been disabled from testing.
	Scenario DELEG-OOB-W-ERROR-1 has been disabled from testing.
	Scenario DELEG-OOB-W-ERROR-2 has been disabled from testing.
	Scenario DELEG-OOB-W-ERROR-3 has been disabled from testing.
	Scenario DELEG-OOB-W-ERROR-4 has been disabled from testing.

How to test

All unit tests should be enabled and all should pass. When #1373 has been updated and merged this PR will be rebased on develop branch and all should pass.

@matsduf matsduf marked this pull request as draft May 27, 2024 21:55
@tgreenx tgreenx added the FA-MethodV2 Focus Area: Implementing and migrating to MethodNT for test cases. label Jun 11, 2024
@tgreenx tgreenx added this to the v2024.2 milestone Jun 11, 2024
@matsduf matsduf force-pushed the updates-methodsv2-unit-tests branch 2 times, most recently from 58a034e to 7b12a5c Compare July 9, 2024 21:37
@matsduf matsduf requested a review from tgreenx July 9, 2024 21:39
@matsduf matsduf force-pushed the updates-methodsv2-unit-tests branch 2 times, most recently from 1747b77 to fb3ca78 Compare July 9, 2024 22:22
Copy link
Contributor

@tgreenx tgreenx left a comment

Choose a reason for hiding this comment

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

LGTM so far

t/TestUtil.pm Outdated Show resolved Hide resolved
t/methodsv2.t Outdated Show resolved Hide resolved
Copy link
Contributor

@tgreenx tgreenx left a 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 fix to handle undefined values in the input data hash :

diff --git a/t/TestUtil.pm b/t/TestUtil.pm
index 2f627823..28941126 100644
--- a/t/TestUtil.pm
+++ b/t/TestUtil.pm
@@ -334,9 +334,9 @@ sub perform_methodsv2_testing {

             # Methods: get_del_ns_names() and get_zone_ns_names()
             @method_names = qw( get_del_ns_names get_zone_ns_names );
-            my @expected_del_ns_names = uniq map { (split( m(/), $_ ))[0] } @{ $expected_del_ns };
-            my @expected_zone_ns_names = uniq map { (split( m(/), $_ ))[0] } @{ $expected_zone_ns };
-            my @expected_ns_names = ( \@expected_del_ns_names, \@expected_zone_ns_names );
+            my $expected_del_ns_names = defined $expected_del_ns ? [ uniq map { (split( m(/), $_ ))[0] } @{ $expected_del_ns } ] : undef;
+            my $expected_zone_ns_names = defined $expected_zone_ns ? [ uniq map { (split( m(/), $_ ))[0] } @{ $expected_zone_ns } ] : undef;
+            my @expected_ns_names = ( $expected_del_ns_names, $expected_zone_ns_names );
             foreach my $i ( 0..$#method_names ) {
                 my $method = $method_names[$i];
                 subtest $method => sub {
@@ -362,9 +362,9 @@ sub perform_methodsv2_testing {

             # Methods: get_del_ns_ips() and get_zone_ns_ips()
             @method_names = qw( get_del_ns_ips get_zone_ns_ips );
-            my @expected_del_ns_ips = grep defined, uniq map { (split( m(/), $_ ))[1] } @{ $expected_del_ns };
-            my @expected_zone_ns_ips = grep defined, uniq map { (split( m(/), $_ ))[1] } @{ $expected_zone_ns };
-            my @expected_ns_ips = ( \@expected_del_ns_ips, \@expected_zone_ns_ips );
+            my $expected_del_ns_ips = defined $expected_del_ns ? [ uniq map { (split( m(/), $_ ))[1] } @{ $expected_del_ns } ] : undef;
+            my $expected_zone_ns_ips = defined $expected_zone_ns ? [ uniq map { (split( m(/), $_ ))[1] } @{ $expected_zone_ns } ] : undef;
+            my @expected_ns_ips = ( $expected_del_ns_ips, $expected_zone_ns_ips ); 

Moreover I suggest that some new checks be added around line 269 to forbid the presence of any undefined value in the arrays of the input data hash.

@matsduf
Copy link
Contributor Author

matsduf commented Jul 11, 2024

I suggest the following fix to handle undefined values in the input data hash :

I implemented the proposal and ran one unit test under the latest code of #1373 and get the following output (still with defined array in methods.t):

        #   Failed test 'Result is defined'
        #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 297.
        # Unexpected undefined result
        # Looks like you failed 1 test of 1.

    #   Failed test 'get_parent_ns_ips'
    #   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 307.
    # Looks like you failed 1 test of 1.
t/methodsv2.t .. 4/? 
#   Failed test 'NO-CHILD-2'
#   at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 395.
Can't use an undefined value as an ARRAY reference at /home/ubuntu/git/zonemaster-engine/t/TestUtil.pm line 302.

It is hard to understand such a message. It should be clearer. It is not obvious what it means.

If I add undef in methods.t there is no error.

All tests successful.

@matsduf
Copy link
Contributor Author

matsduf commented Jul 11, 2024

Moreover I suggest that some new checks be added around line 269 to forbid the presence of any undefined value in the arrays of the input data hash.

I do not know what you suggest.

@matsduf matsduf force-pushed the updates-methodsv2-unit-tests branch from 83211a7 to 3ba6d01 Compare July 12, 2024 19:57
@matsduf matsduf marked this pull request as ready for review July 12, 2024 20:08
Copy link
Contributor

@marc-vanderwal marc-vanderwal left a comment

Choose a reason for hiding this comment

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

Looks good to me, save for a few typos.

t/TestUtil.pm Outdated Show resolved Hide resolved
t/TestUtil.pm Outdated Show resolved Hide resolved
t/TestUtil.pm Outdated Show resolved Hide resolved
matsduf and others added 5 commits July 16, 2024 18:50
* Explicit error messages expected list does not match extracted by code
* Support for distrinktion between empty list and undefined list
* Added feature "single scenario"
* Added feature "disable scenarios" for one run
* Corrected bug that created undefined IP address when IP addresses were
  extracted from name/IP expressions an expression was name only
* Replaced built in IP address verification against from Zonemaster library
* Adds more scenarios/unit tests
* Updates unit tests
* Adds support for running a single scenario or
  temporarily disable a scenario or list of scenarios
Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>
- Fix support of name servers without IP addresses in input data hash
- Update unit tests
- Update unit test data
- Fix support of name servers without IP address
- Add check for existence of single scenario
- Update unit test and unit test data
Copy link
Contributor

@tgreenx tgreenx left a comment

Choose a reason for hiding this comment

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

@matsduf I took the liberty of adding a few commits in this PR. Now, together with the latest changes in #1373, all unit tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FA-MethodV2 Focus Area: Implementing and migrating to MethodNT for test cases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants