Updates TestUtil.pm for unit tests by matsduf · Pull Request #1340 · zonemaster/zonemaster-engine

matsduf · 2024-05-06T14:53:41Z

Purpose

This PR introduces two changes to function in TestUtil.pm:

Support of undelegated data (both NS and DS) for unit tests.
Makes the function more flexible to avoid long lists of tags when not needed.
Do not use diag/fail when there is an error in the t file. Use croak() instead.
More verifications of data.

The unit test files using TestUtil.pm have been updated to match the new format.

Five new scenarios have been added to Consistency05 and two to Consistency06. Both updates are to unit tests that uses the new function in TestUtil.pm that supports undelegated data.

The following new scenario fail:

MULT-SOA-MNAMES-NO-DEL-UNDEL-2 (Consistency06)

Context

The update is needed for zonemaster/zonemaster#1255.

How to test this PR

Review TestUtil.pm. If the unit test files using TestUtil.pm then they should be fine.

…o be more flexible

mattias-p · 2024-05-06T16:40:13Z

+(in all uppercase), and their corresponding values are an array of:
+
+=over
+
+=item *
+a boolean (testable)
+
+=item *
+a string (zone name)
+
+=item *
+an array of strings (all test case message tags)
+
+=item *
+an array of strings (mandatory message tags)
+
+=item *
+an array of strings (forbidden message tags)
+
+=item *
+an array of name server expressions for undelegated name servers
+
+=item *
+an array of DS expressions for "undelegated" DS
+
+=back


These are a lot of positional arguments. I suggest replacing the arrayref of positional arguments with a hashref of named arguments.

I think that I understand what you mean, but that will make it more complex to create e.g. t/Test-nameserver15.t and harder to read it. Please note that "all test case message tags" will just be a variable, and that the two last arguments will not be used in most cases.

I'm not sure how you're measuring complexity in the call site. The calling code would be easier to read and less error prone because you wouldn't run the risk of mixing up the arguments. There would be more code inside perform_testcase_testing() but this is a quite good investment.

mattias-p · 2024-05-06T17:04:43Z

+        if ( ref( $testable ) ne '' ) {
+            diag("Scenario $scenario: Type of testable must not be a reference");
+            fail("Testable is of the correct type");
+            next;
+        }


This is problematic for a couple of reasons, but the biggest one is that the diag() and fail() methods are uses here for complaining about the test script. They should only be used for complaining about the object under test. When complaining about the test script it's better to use croak().

There are more of these below.

I do realize you followed the established pattern, but it would be a really good to break this pattern.

This is not new code. It is the same code as in the current version, but moved up. Could you suggest the replacement code?

Just replace the diag, fail and next with a croak.

mattias-p · 2024-05-06T17:06:56Z

        }

-        if ( ref( $mandatory_message_tags ) ne 'ARRAY' ) {
+        if ( ref( $all_test_case_tags ) ne 'ARRAY' ) {


If we care to verify that it's an array, shouldn't we also check the type of the array elements?

There are more of these below.

This is not new code. It is the same code as in the current version, but maybe moved. What you suggest is that all elements in the array should be matching the tag format? I guess that could be done. Should that also use croak() instead of diag/fail? If so, what is the benefit of croak?

I don’t think it’s useful to check the type of the array elements: because the array is usually prepared using qw(), it is reasonable to assume that each element can be treated as a string. I’m not saying this kind of thorough checking shouldn’t be done; only that I don’t mind if it isn’t done.

However, I do agree that we should use croak instead of diag and fail here. We don’t want to raise an issue with the domain under test here, but with the code that tests the code. In that case, aborting early gives a stronger signal that the problem is with the testing code.

Co-authored-by: Mattias Päivärinta <mattias@paivarinta.se>

* Replaces diag/fail with croak(). * Additional verification of test data from t file. * Moved "all tags" to be a common parameter, not scenario specific.

matsduf · 2024-05-07T13:20:46Z

@mattias-p, please re-review.

matsduf · 2024-05-13T12:44:21Z

@mattias-p, could your re-review?

tgreenx · 2024-05-14T09:01:21Z

    'NO-RESPONSE-MX-QUERY' => [
-        q(no-response-mx-query.zone09.xa),
+        1,
+        q(no-response-mx-query.zone09.xa),#


Why is there a trailing # here? (many other occurrences below)

I tagged scenarios where MANDATORY and FORBIDDEN did not cover all tags. I'll make it explicit.

Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>

matsduf · 2024-05-14T12:29:12Z

@tgreenx and @mattias-p, please re-review.

matsduf · 2024-05-21T07:49:49Z

@mattias-p, @tgreenx and @marc-vanderwal, can you approve this? I would like to merge it. It updates TestUtils.pm and the unit tests that use it. They pass as expected.

marc-vanderwal · 2024-05-21T12:34:30Z

I’ve had a look. I see no issues that haven’t already been brought up by @tgreenx.

Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>

matsduf · 2024-05-21T13:22:23Z

@marc-vanderwal and @tgreenx, please re-review after my latest updates.

marc-vanderwal

Looks fine. I do think that croak is better than diag/fail when there is a problem with the data in the test script. It’s better to have a specific failure mode for problems with the testing code, that can’t be confused with the outcome of a failing unit test suite for reasons unrelated to the testing code itself.

marc-vanderwal · 2024-05-21T13:46:19Z

        }

-        if ( ref( $mandatory_message_tags ) ne 'ARRAY' ) {
+        if ( ref( $all_test_case_tags ) ne 'ARRAY' ) {


I don’t think it’s useful to check the type of the array elements: because the array is usually prepared using qw(), it is reasonable to assume that each element can be treated as a string. I’m not saying this kind of thorough checking shouldn’t be done; only that I don’t mind if it isn’t done.

However, I do agree that we should use croak instead of diag and fail here. We don’t want to raise an issue with the domain under test here, but with the code that tests the code. In that case, aborting early gives a stronger signal that the problem is with the testing code.

marc-vanderwal · 2024-05-21T13:49:09Z

+        }
+
+        if ( ! defined( $mandatory_message_tags ) and !defined( $forbidden_message_tags ) ) {
+            diag("Scenario $scenario: Not both array of mandatory tags and array of forbidden tags can be undefined");


Suggested change

diag("Scenario $scenario: Not both array of mandatory tags and array of forbidden tags can be undefined");

diag("Scenario $scenario: Arrays of mandatory tags and forbidden tags cannot be both undefined");

Your proposal must be based on old code. The current code uses "croak" for such tests.

matsduf · 2024-05-22T07:52:17Z

@marc-vanderwal and @tgreenx, please re-review after my latest updates. All tests pass. TestUtil now treats empty-empty as an error. Can this be merged now?

tgreenx

@marc-vanderwal and @tgreenx, please re-review after my latest updates. All tests pass. TestUtil now treats empty-empty as an error. Can this be merged now?

Almost! A few nits left:

Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>

matsduf · 2024-05-22T09:02:37Z

I have commited the proposal from @tgreenx and I have updated the documentation so that it states that empty-empty is not permitted. @marc-vanderwal and @tgreenx, please re-review.

Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>

matsduf · 2024-05-22T09:04:43Z

@marc-vanderwal and @tgreenx, please re-review.

matsduf added 2 commits May 6, 2024 16:43

Updates perform_testcase_testing() to support undelegated data plus t…

6c19de9

…o be more flexible

Adaption to updated TestUtil

cbb0e5f

matsduf added this to the v2024.1 milestone May 6, 2024

matsduf requested review from marc-vanderwal, mattias-p and tgreenx May 6, 2024 14:53

matsduf added 3 commits May 6, 2024 15:11

Corrects code errors

8748e64

Corrects errors in the code

f4e6fdd

Corrects errors in one unit test

3dc5802

mattias-p reviewed May 6, 2024

View reviewed changes

matsduf and others added 7 commits May 6, 2024 23:31

Update t/TestUtil.pm

2454b53

Co-authored-by: Mattias Päivärinta <mattias@paivarinta.se>

Editorial update from review comment

b029102

Adds some verification of "all tags" array

c19ead3

Clarification

c6668a0

Adds documentation of the mechanism of undefined argument

d38dcf8

Updates TestUtil.pm

c330a1d

* Replaces diag/fail with croak(). * Additional verification of test data from t file. * Moved "all tags" to be a common parameter, not scenario specific.

Adapts to updated TestUtils

9bc3e12

matsduf requested a review from mattias-p May 7, 2024 13:20

Corrects NS name pattern

dd0630d

matsduf mentioned this pull request May 12, 2024

Adds unit tests for Basic01 #1345

Merged

matsduf added 2 commits May 12, 2024 21:49

Adjusts error reporting

3987bd6

Adapts to updated TestUtils

4263eba

tgreenx reviewed May 14, 2024

View reviewed changes

matsduf and others added 2 commits May 14, 2024 13:59

Apply suggestions from code review

f46b5c6

Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>

Updates based on review comments

87dbe6c

matsduf requested a review from tgreenx May 14, 2024 12:28

tgreenx reviewed May 14, 2024

View reviewed changes

Comment thread t/TestUtil.pm Outdated

matsduf mentioned this pull request May 18, 2024

Adds test scenarios for Consistency05 and Consisteny06 zonemaster/zonemaster#1266

Merged

matsduf added the V-Patch Versioning: The change gives an update of patch in version. label May 18, 2024

matsduf requested a review from tgreenx May 18, 2024 20:45

Editorial updates

354471a

tgreenx reviewed May 21, 2024

View reviewed changes

Comment thread t/Test-zone11.t Outdated

Comment thread t/TestUtil.pm Outdated

Comment thread t/TestUtil.pm Outdated

Comment thread t/TestUtil.pm

matsduf and others added 2 commits May 21, 2024 15:09

Apply suggestions from code review

ae82da2

Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>

Editorial updates

345c038

matsduf requested a review from tgreenx May 21, 2024 13:22

marc-vanderwal reviewed May 21, 2024

View reviewed changes

matsduf requested a review from marc-vanderwal May 21, 2024 14:59

matsduf added 2 commits May 21, 2024 16:03

Adds correct handling of empty tag arrays

ed80af8

Corrects and change logic: both arrays empty not permitted

f566274

tgreenx reviewed May 22, 2024

View reviewed changes

Comment thread t/TestUtil.pm

Comment thread t/TestUtil.pm Outdated

matsduf and others added 2 commits May 22, 2024 10:56

Apply suggestions from code review

d538a14

Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>

Editorial update

7d11932

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

marc-vanderwal reviewed May 22, 2024

View reviewed changes

Comment thread t/TestUtil.pm Outdated

matsduf requested a review from marc-vanderwal May 22, 2024 09:03

Apply suggestions from code review

0bc6dcd

Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>

marc-vanderwal approved these changes May 22, 2024

View reviewed changes

tgreenx approved these changes May 22, 2024

View reviewed changes

tgreenx mentioned this pull request May 22, 2024

Add MethodsV2 unit testing #1350

Merged

matsduf merged commit aeb9b63 into zonemaster:develop May 22, 2024

matsduf deleted the update-unit-test-testutil branch May 22, 2024 09:58

	diag("Scenario $scenario: Not both array of mandatory tags and array of forbidden tags can be undefined");
	diag("Scenario $scenario: Arrays of mandatory tags and forbidden tags cannot be both undefined");

Uh oh!

Conversation

matsduf commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Context

How to test this PR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matsduf May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matsduf commented May 7, 2024

Uh oh!

matsduf commented May 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matsduf commented May 14, 2024

Uh oh!

Uh oh!

matsduf commented May 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marc-vanderwal commented May 21, 2024

Uh oh!

matsduf commented May 21, 2024

Uh oh!

marc-vanderwal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matsduf commented May 22, 2024

Uh oh!

tgreenx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matsduf commented May 22, 2024

Uh oh!

Uh oh!

matsduf commented May 22, 2024

Uh oh!

Reviewers

matsduf commented May 6, 2024 •

edited

Loading

matsduf May 6, 2024 •

edited

Loading