Update unit tests depending on real-world data#1516
Open
marc-vanderwal wants to merge 6 commits intozonemaster:developfrom
Open
Update unit tests depending on real-world data#1516marc-vanderwal wants to merge 6 commits intozonemaster:developfrom
marc-vanderwal wants to merge 6 commits intozonemaster:developfrom
Conversation
The IP address bound to a.root-servers.net is now in over 30 ASNs simultaneously, so let’s use K-root instead, whose address is still in one AS. Until we find a better way.
Parts of this unit tests relies on live data from the Internet that cannot be guaranteed to stay the same. As of now (2026-03-25), the data on www.wiccainfo.se cannot be recorded anymore. These parts are essentially made obsolete by t/recursor-A.t, which exercises the Zonemaster::Engine::Recursor much more thoroughly by means of the test scenario framework. However, we cannot delete the entire file without negatively affecting test coverage in some small parts of the codebase. Best to be careful and not just go around deleting too many things in one go.
Whoever owned 001.tf dropped the domain, but fortunately 001.re exists and has the same problem, namely having all its name servers in the same AS.
Thanks to scanning the entire .fr zone with Zonemaster, I was able to find other domains that had the same problems as those that were previously in the test file, even removing a TODO block in the process.
Two unit tests, t/Test-zone01-{A,B}.t, had incorrect fake delegations
that prevented correct rerecording of the unit tests, given how the
zones are currently set up.
And in t/undelegated.t, an update was necessary for the fake delegations
so that they’d trigger FAKE_DELEGATION_IN_ZONE_NO_IP again.
The reason why t/undelegated.t needed fixing is not quite clear to me
yet. It might be because in the real world, the name servers names of
nic.se are no longer all subdomains of nic.se.
matsduf
added
V-Patch
matsduf
previously approved these changes
Mar 27, 2026
marc-vanderwal
changed the title
At the end of t/nameserver.t, there are attempts to send queries to a fake nameserver whose IPv4 address lies in 192.0.2.0/24 (TEST-NET-1), a range that should not be routable on the public Internet, in order to elicit a guaranteed resolution failure. However, the test is flawed and should only work when it is run under ZONEMASTER_RECORD=1. Here’s why. The test expects a resolution failure to generate two messages, a LOOKUP_ERROR message and a BLACKLISTING message. However, looking at the logic in Zonemaster::Engine::Nameserver, these messages are only generated if the query was actually sent out on the network; in other words, if the Nameserver object is returning a cached resolution failure, these messages will not be generated. Therefore, if running offline (without ZONEMASTER_RECORD=1), we *should not* have these two messages and their presence arguably indicates a bug with the cache. Because of that flawed logic in t/nameserver.t, after rerecording the t/nameserver.data file with: $ ZONEMASTER_RECORD=1 prove -l t/nameserver.t then running the test again in offline mode with: $ prove -l t/nameserver.t the test stops passing. This commit changes the test’s logic so that if we run offline, we ensure that LOOKUP_ERROR and BLACKLISTING are *not* generated. The previous logic is only run if running with ZONEMASTER_RECORD=1.
Contributor
Author
|
I found a flaw in |
tgreenx
approved these changes
Mar 31, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 updates some unit tests so that the associated data files can be rerecorded again.
Some unit tests rely on real-world data across the Internet. There is a facility for storing and replaying network packets so that running the test suite does not generate any requests across the network; this enables running the test suite without depending on Internet access and avoids breakage whenever a network requests gives a different result than what was previously the case.
However, there may be circumstances where rerecording the entirety of the test suite’s data files is necessary. In some cases, this is not possible anymore because of changes that happen beyond our control. This PR tries to increase the likelihood that
ZONEMASTER_RECORD=1 make testpasses without errors.This PR also addresses a logic error in
t/nameserver.tthat fails to take into account that Zonemaster::Engine only generates some messages when the networks was accessed, instead of returning a cached result. For more information, see the correspoding commit’s message.This PR supersedes #1515.
Context
Development of #938.
Changes
t/asn.tt/nameserver.tt/recursor.tt/Test-connectivity03.tt/Test-delegation.tt/Test-zone01-A.tt/Test-zone01-B.tt/undelegated.tHow to test this PR
Run
make testand expect unit tests to pass.Then run
ZONEMASTER_RECORD=1 make testand expect unit tests to pass.Finally, run
make testagain and expect unit tests to pass one more time.