Merge develop branch into master (zonemaster-engine) #1483
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merge master into develop (Zonemaster-Engine)
correct. Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>
Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>
The scenario named GOOD-2 erroneously referred to a test zone named good-1.basic02.xa, instead of good-2.basic02.xa. Because of that, scenario GOOD-2 was not properly tested. Simply fixing the typo was not enough. No data was recorded about good-2.basic02.xa at all, so the companion .data file also needed to be regenerated.
Fix typo in unit test of BASIC02
Scenario MULT-SOA-MNAMES-NO-DEL-UNDEL-2 was marked as not testable, but it turned out that it was due to errors both in the test scenario implementation (i.e. the zone files and CoreDNS configuration: see zonemaster/zonemaster#1414) and a small typo in the .t file. In the .t file, scenario MULT-SOA-MNAMES-NO-DEL-UNDEL-2 erroneously listed ns3.mult-soa-mnames-no-del-undel-2.consistency06.xb twice in its undelegated data. The second instance should be ns4, not ns3. After fixing these problems, we can enable the scenario in Zonemaster-Engine’s test suite and rerecord the test data.
The get_parent_ns_ips() method in MethodsV2 takes a class (string) and a Zonemaster::Engine::Zone object. It is memoized thanks to the facility provided by the Memoize module. However, it turns out that the cache key computed by Memoize is incorrect. This could lead to suboptimal performance and in some cases completely wrong results. In particular, it could cause the t/methodsv2.t unit test to fail intermittently – which can be a real nuisance with continuous integration. By default, Memoize takes the string representations of each item in @_ joined by the ASCII 0x1c (FS, Field Separator) character. The $zone argument is a blessed reference to Zonemaster::Engine::Zone, and the package does not provide a stringification operator override; therefore a nonsensical string like Zonemaster::Engine::Zone=HASH(0x62c9f89bc410) is used as a cache key instead of the zone name. This commit provides a custom normalizer to the call to memoize() so that the zone name is used as part of the cache key. It seems to fix the problem (I tested by running t/methodsv2.t 50 times in a row) and, as a bonus, also accelerates the unit test suite somewhat. Maybe real-world domain tests might also benefit from it?
…sistency06-data Test-consistency06.t: enable scenario MULT-SOA-MNAMES-NO-DEL-UNDEL-2
Fix error in memoization for MethodsV2 method Get-Parent-NS-IP
There is a proto-DSL already, in t/TestUtil.pm, that helps us test Zonemaster test cases with respect to predefined scenarios. But we need to go further. Scenarios are defined in terms of a zone name (which names a zone that is configured in a predetermined way), some possible extra configuration for the test case (such as “fake NS” and “fake DS” records), and a list of messages that we expect Zonemaster to emit when testing the zone name. Currently, when using t/TestUtil.pm, we list scenarios in a array of arrays. It has been working fine for now, but it’s lacking a feature: testing message arguments in addition to message tags. How do we specify predicates that message *arguments* should satisfy? This is where the current method shows its limit. The DSL is meant to be easy to read, not only for those who routinely write such unit tests, but also for more casual readers. Its system of keywords leaves room for extensibility when the need for new features arises. It provides shorthands that aid writing unit tests that look similar, without compromising readability. And last but not least, the syntax allows for specifying properties that message arguments are to satisfy when looking for messages of a certain tag (which isn’t fully implemented yet). Care has been taken to retain maximum compatibility with the features of TestUtil.pm’s perform_testcase_testing() function in most areas. In addition for its improved facilities for testing message arguments, the DSL introduces another extension: a distinction between “todo” tests (i.e. not testable because of a bug in the implementation, but otherwise the scenario is fine) and “not_testable” tests (i.e. not testable because the scenario is not implemented or cannot be implemented with current means).
Functions like Test::More’s ok() automatically generate diagnostic messages pointing to the caller’s location when a test fails. It works fine in simple cases where Test::More is used directly in a .t file, but it stops working here because of the DSL’s design. As a result, test failures were always traced to some line in either t/TestUtil/DSL.pm or t/TestUtil/DSL/Compiler.pm, which makes no sense. In order to remedy this, we need to augment the AST with context information about the place where scenario blocks and expect/forbid keywords are used. Then, at execution time, we employ some rather inelegant hacks in order to print the appropriate context information in lieu of Test::More’s automatically generated context information should a test fail.
Some refactoring was needed in order to make sure that todo tests were handled appropriately. The previous implementation did not call todo_start() at the right moment. Because of that, a test marked todo which did pass was not reported at the end of the test harness’s output.
An issue, zonemaster/zonemaster#1401, proposes to “[u]pdate a test scenario with requirements for message arguments”. While as of writing this commit for the first time (2025-07-30), no scenario has been updated yet, the tooling is implemented. Message arguments can be tested as it is specified in the DSL’s documentation. If such a test fails, we try our best to print as helpful a message as possible: the test prints what exactly it was looking for and which messages it has examined. This debugging output could be improved by explaining why a given message has failed a match, but the current level of detail is hopefully plenty enough for debugging purposes. Below is a sample TAP output detailing a failure of such a test. Long lines are folded in order not to exceed 78 columns (the offendings lines end with a backslash): t/Test-basic02.t .. 10/? # Failed test 'Messages of tag 'B02_AUTH_RESPONSE_SOA' exist with\ # specified arguments' # at t/Test-basic02.t line 112. # Looked for a message whose tag is 'B02_AUTH_RESPONSE_SOA' # where 'domain' equals 'mixed-1.basic02.xa' # and 'ns_list' equals 'ns1.mixed-1.basic02.xa/127.12.2.31;\ # ns1.mixed-1.basic02.xa/fda1:b2:c3:0:127:12:2:31' # and contains no argument other than those listed above # Here are all messages that unsuccessfully matched: # B02_AUTH_RESPONSE_SOA domain="mixed-1.basic02.xa"; \ # ns_list=ns1.mixed-1.basic02.xa/127.12.2.31;\ # ns1.mixed-1.basic02.xa/fda1:b2:c3:0:127:12:2:31 # Failed scenario 'MIXED-1' # at t/Test-basic02.t line 117. # Looks like you failed 1 test of 26. t/Test-basic02.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/26 subtests
To avoid surprises in the semantics of the DSL, the scenarios should be run in the order they were defined in the .t file. Doing this might let us simplify the implementation later.
Refactor the way subtsets are selected or disabled using environment variables. We also enforce the existence of scenario names listed in those environment variables, which we previously didn’t.
Re-engineer the AST data structure generated by TestUtil::DSL so that multiple root_hints are allowed. Subsequent root_hints entirely override previous ones.
In order to avoid surprises for the end user, it’s better to respect the order in which the expect and forbid keywords were defined in the scenarios too. It simplifies the DSL’s implementation as well.
Add a keyword to tell Zonemaster::Engine::Resolver to clear the cache between tests. That can be useful in some corner cases where caching of resolution failures interferes with the correct operation of the unit test, which is the case of t/Test-connectivity04.t, among others.
This one is interesting because it has a scenario, PTR-IS-ILLEGAL-CNAME, which expects one tag, forbids one tag and leaves two others hanging.
Rewriting this test file to use the DSL helped me find a typo in the original t/Test-basic02.t file that caused the GOOD-2 scenario to point to good-1.basic02.xa instead of good-2.basic02.xa (see PR #1465). It’s a good demonstration of why it can be important to design data structures and formats in such a way that internal repetition is avoided unless absolutely necessary: when done right, it makes subtle errors like these more obvious.
In this specific case, the unit test was broken in two files because caching of resolution failures (“blacklisting”) caused issues with one of the scenarios. Thanks to the DSL’s “clear_cache” keyword, however, we can put the affected scenario in the same file.
This one is interesting because it has a few scenarios that are marked as not testable. We do the same here.
Another rewrite of a unit test that led me to uncover a typo in the original unit test. Actually, this time, the specification was incorrect (see zonemaster/zonemaster#1414).
This one is fairly straightforward, but many scenarios are marked as not testable.
Minor typos and french translation modifications
Somehow, introducing Get-Parent-NS-Names-and-IPs, then redefining Get-Parent-NS-IPs in terms of Get-Parent-NS-Names-and-IPs, has had side effects on some unit tests that perform tests against live zones: suddenly, they tried to perform queries to a variety of name servers (among which M-root). I haven’t fully investigated the reason why the tests failed, but it might be because the order of the servers being queried has changed. I also suspect some of the name servers have changed IP addresses since recording. The offending .t files are t/Test.t, t/Test-dnssec.t and t/Test-connectivity.t. Rerecording the corresponding data files is enough to fix everything, except for t/Test-dnssec.data where it was necessary to splice part of the newly recorded data into the existing data so that everything continues to work. It is rather inelegant, but it gets the job done until the affected tests are migrated to the new scenario-based framework.
…mes-and-ips Implement new MethodV2: Get-Parent-NS-Names-and-IPs
RD flag, protocol and TC upgrade policy for a given query should be determined by code, not config.
Ignore and deprecate some profile properties
Update Zone11 implementation
Update DNSSEC05 implementation
Update Address01 implementation
Update DNSSEC01 implementation
Update DNSSEC07 implementation
Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>
Update DNSSEC05 message IDs
Update DNSSEC07 message IDs
Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>
Update French translation
Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>
Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>
Preparation for release v2025.2 (Zonemaster-Engine)
matsduf
added a commit
that referenced
this pull request
Dec 19, 2025
Merge pull request #1483 from zonemaster/develop
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
This PR is step https://github.com/zonemaster/zonemaster/blob/develop/docs/internal/maintenance/ReleaseProcess-release.md#15-merge-develop-branch-into-master
How to test this PR
"No reviewer or approval is required for this update."