Rewrite Zone09 fully after specification update - #1543
Conversation
marc-vanderwal
added
V-Patch
|
Is the failing unit test due to error in a hint file for the scenarios? |
|
It’s an obsolete unit test file I forgot to delete. Previously, when different root hints are to be used for a scenario, that scenario had to go in a different |
b86c93f to
b4b29bc
Compare
Rewrite the implementation of Zone09 completely, after the specification was updated. The previous implementation had two main issues: firstly, MX RRsets were not sorted before comparison; secondly, MX RRsets that only differ by TTL values were not deemed equal, although we wanted them to compare equal. Now, only the preference and exchange fields of the RDATA are used for sorting and comparison. Test scenarios were updated as well, so this commit also updates the corresponding unit tests. Said unit tests are ported to the DSL for good measure. When relevant, even the messages’ arguments are tested.
The rewriting of Zone09 also involved a migration from old Methods to MethodsV2, leading to a few more DNS queries made to authoritative servers in parent zones. The corresponding packets do not exist in the corresponding t/Test-zone.data file, so running this unit test fails. It cannot easily be rerecorded either and the .t file barely exercised the code anyway, so we can afford not to run Zone09 in that legacy test.
Use the newly-introduced _is_non_mail_domain() method and rewrite the logic of Zone11 in a way that is equivalent to the current logic, while involving fewer nested ifs.
b4b29bc to
00c3a82
Compare
Is this still a draft PR? |
It’s ready for review, but it shouldn’t be merged yet. I just hope that the specifications and the scenarios are stable enough so that this code is stable too. |
We should have an additional review on both PRs. I will soon review this PRs. |
matsduf
left a comment
matsduf
left a comment
There was a problem hiding this comment.
I have tested the scenarios with zonemaster-cli while updating the test-zones-output.md file. I have also inspected the normal output (non-raw). Everything looks fine and as expected.
The mail exchange domain name has a final dot '.' in the presentation, but in name/IP pairs the final dot is removed. It looks better and it is easier to read if the final dot of a domain name is always removed (unless it is the root node). |
When converting an MX record’s RDATA to string form, the domain name in the exchange field always had its trailing dot, like in the RDATA’s presentation format. This commit omits that trailing dot unless the exchange is the root domain.
Good point. I’ve fixed that. |
|
Yesterday I ran the same |
2 participants
Purpose
This PR is a complete rewrite of the Zone09 test case, after its specification and test scenarios were updated.
The previous implementation had two main issues: firstly, MX RRsets were not sorted before comparison; secondly, MX RRsets that only differ by TTL values were not deemed equal, although we wanted them to compare equal. Now, only the preference and exchange fields of the RDATA are used for sorting and comparison.
Test scenarios were updated as well, so this commit also updates the corresponding unit tests. Said unit tests are ported to the DSL for good measure. When relevant, even the messages’ arguments are tested.
Context
See:
Changes
lib/Zonemaster/Engine/Test/Zone.pmso that lexical subroutines (state sub) can be used inside methodsHow to test this PR
Unit tests should still pass.