Betterize new unit testing framework

[tgreenx]

Purpose

This PR proposes an update to the new unit testing framework that is being put in place. It aims at improving the content of each test case unit test file (t/Test-*.t) by simplifying them. It does so with the following:

• A sizable amount of duplicate code is moved to a new helper module.
• A hash holding test case scenarios is used (instead of a __DATA__ token at the end of the file)
• All relevant, modifiable content is moved at the start of the file (within ########### banners).

With this PR, current test case unit test files (in the new framework) are reduced by about 30%.

I believe these changes are desirable to ease the migration process of other test case unit test files.

Context

Inspired from #1287

Changes

• Create helper module t/TestUtil.pm
• Create helper function TestUtil::perform_testcase_testing()
• Refactoring

How to test this PR

Tests should pass.

[matsduf]

* A sizable amount of duplicate code is moved to a new helper module.

I think that is a good idea.

* An explicit hash (and keys) holding test case scenarios is used (instead of a `__DATA__` token at the end of the file)

I think the current solution with the __DATA__ token makes it easier to read and write the data section. With the token the data section gets more compact without the loosing readability.

The format in the specification can easily be transformed to the unit test.

What problem do you see by the token solution?

[tgreenx]

I think the current solution with the __DATA__ token makes it easier to read and write the data section. With the token the data section gets more compact without the loosing readability.

The format in the specification can easily be transformed to the unit test.

What problem do you see by the token solution?

Well, firstly, I simply couldn't appropriately feed the __DATA__ token to an external function in a structured way. Besides, I see the following advantages to the proposed change:

• It uses an hash structure with explicit key names, so comment blocks can be removed
• Test scenario names are added as the top-level key
• A testable sub-key name is used to discard (and still display) untestable scenarios
• It reinforces point 3 described in this PR --> 'All relevant, modifiable content is moved at the start of the file (within ########### banners)'
• Increased flexibility and data manipulation in case of additional data fields

The main downside that I see is that when transforming the specification into the unit test, commas need to be removed when added in the code. But personally I didn't find it to be much of a hassle when using a proper text editor. Plus, note that if one or more are included by mistake, this will produce a warning message, such as: Possible attempt to separate words with commas at t/Test-zone09-1.t line 34, so it is easy to spot. From qw():

A common mistake is to try to separate the words with commas or to put comments into a multi-line qw-string. For this reason, the use warnings pragma and the -w switch (that is, the $^W variable) produces warnings if the STRING contains the "," or the "#" character.

[tgreenx]

@matsduf What are your thoughts after my last reply? I would like to have a consensus and have this PR merged before doing more unit test in the new framework.

[marc-vanderwal]

I like the new data structures for describing the expected outcomes of each scenario.

Firstly, they are self-documenting: keys like mandatory and forbidden make it crystal clear to me that these are tags that must, or must not appear when running a test against a given zone.

Secondly, the syntax is flexible and there are no hidden surprises, because Perl’s syntax applies.

Thirdly, these structures can be extended by means of additional keys, to accommodate situations that require it.

I also like the idea of the testable key, which documents scenarios for which we lack the means of testing automatically for the time being.

[matsduf]

First I want to state that I am in favor of finding a format that makes it easy to create and maintain the unit test files, and where a unit module, such as the one that you have created, contains shared code.

Well, firstly, I simply couldn't appropriately feed the __DATA__ token to an external function in a structured way. Besides, I see the following advantages to the proposed change:

If you set a package name in the file, the DATA data will be available as PACKAGE::DATA file handle.

* It uses an hash structure with explicit key names, so comment blocks can be removed

I do not see the direct need of named keys. A small header or a pointer could solve the information need. A readable and compact format is more valuable.

* Test scenario names are added as the top-level key

The scenario name is in the zone name, so that is not needed. The zone name could be the key instead. That would make it shorter. The scenario name is not used for the unit tests.

* A `testable` sub-key name is used to discard (and still display) untestable scenarios

Yes, that is really a good idea.

* It reinforces point 3 described in this PR --> 'All relevant, modifiable content is moved at the start of the file (within ########### banners)'

Well, I do not think that is important. A standardized header first and then specific data at the end works as well.

* Increased flexibility and data manipulation in case of additional data fields

Yes, that could be good, but on the other hand, what we want for the test cases are really fixed format.

The main downside that I see is that when transforming the specification into the unit test, commas need to be removed when added in the code. But personally I didn't find it to be much of a hassle when using a proper text editor.

I still think the __DATA__ makes it easy to create another unit test file. Creating the Perl skeleton requires more work. I just created the unit test files for Consistency05 and Consistency06 (#1303) and for that I could reuse the unit test file for DNSSEC16 and copy the table from the test zone specification and convert it in a few steps. And the section after __DATA__ is easier to read than the %subtests, I think.

Then following would create a more compact format with fewer lines:

my %subtests = (
    'cds-invalid-rrsig.dnssec16.xa' => [
        [ qw(DS16_CDS_INVALID_RRSIG) ],
        [ qw(DS16_CDS_MATCHES_NON_SEP_DNSKEY DS16_CDS_MATCHES_NON_ZONE_DNSKEY DS16_CDS_MATCHES_NO_DNSKEY DS16_CDS_NOT_SIGNED_BY_CDS DS16_CDS_SIGNED_BY_UNK\
NOWN_DNSKEY DS16_CDS_UNSIGNED DS16_CDS_WITHOUT_DNSKEY DS16_DELETE_CDS DS16_DNSKEY_NOT_SIGNED_BY_CDS DS16_MIXED_DELETE_CD) ],
        1
    ],
    'cds-matches-no-dnskey.dnssec16.xa' => [
        [ qw(DS16_CDS_MATCHES_NO_DNSKEY) ],
        [ qw(DS16_CDS_INVALID_RRSIG DS16_CDS_MATCHES_NON_SEP_DNSKEY DS16_CDS_MATCHES_NON_ZONE_DNSKEY DS16_CDS_NOT_SIGNED_BY_CDS DS16_CDS_SIGNED_BY_UNKNOWN\
_DNSKEY DS16_CDS_UNSIGNED DS16_CDS_WITHOUT_DNSKEY DS16_DELETE_CDS DS16_DNSKEY_NOT_SIGNED_BY_CDS DS16_MIXED_DELETE_CDS) ],
        1
    ]
    );

[tgreenx]

@matsduf @marc-vanderwal Following discussions here and elsewhere, I have made the suggested changes as well as adding some value checking of the new hash/array structure. See commit ef82b8a. Please re-review.

[matsduf]

I approve this PR, but I would like to see the following improvements in documentation, which could be in a follow-up PR:

1. In TestUtil.pm the fields of the %subtests should be defined and described. Specifically, it is unclear what the "testable" field does. What happends when true and false, respectively?
2. In the "template" for the t file there should be a pointer to TestUtil.pm for the definition of the %subtests fields.
3. In the "template" for the t file there should be a link to the specification of the scenarios in queestions, i.e. https://github.com/zonemaster/zonemaster/blob/master/docs/public/specifications/test-zones/DNSSEC-TP/dnssec16.md and https://github.com/zonemaster/zonemaster/blob/master/docs/public/specifications/test-zones/Zone-TP/zone09.md, respectively, for the t files in this PR.

[tgreenx]

I approve this PR, but I would like to see the following improvements in documentation, which could be in a follow-up PR:

Good suggestions, I did indeed forget to update the documentation. I addressed your comments in commit e616dfe. Please re-review.