Update implementation of Zone01 - #1028 #1035
Conversation
|
The process we have followed so far is to have the specification completed before implementation starts. In that we lower that risk of have work to be redone. I suggest that the implementation is paused until the specification is done. |
|
I agree with @matsduf. We should follow the same process we had before. Specifications first, then implementation. |
tgreenx
added
the
A-TestCase
tgreenx
changed the title
|
Unit tests are not updated yet and thus will fail. More soon. |
tgreenx
requested review from
marc-vanderwal,
hannaeko,
mattias-p and
a user
|
Ready for review |
|
@matsduf please re-review |
|
@matsduf please re-approve. I just added a reminder for untested message tags in unitary tests. |
lib/Zonemaster/Engine/Test/Zone.pm
Outdated
| Z01_MNAME_NOT_MASTER => { | ||
| ns_list => join( q{;}, sort map { $_ . '/' . %{ $mname_not_master{$_} } } keys %mname_not_master ), | ||
| soaserial => max( map { $mname_not_master{$_} } keys %mname_not_master ), | ||
| soaserial_list => join( q{;}, @serial_ns ) |
There was a problem hiding this comment.
@serial_ns -> uniq @serial_ns, maybe?
There was a problem hiding this comment.
I agree. Either that or turn "serial_ns" into a hash as I suggested above. Compare with %mname_ns.
lib/Zonemaster/Engine/Test/Zone.pm
Outdated
| $continue = 0; | ||
| last; |
There was a problem hiding this comment.
I’ve had a hard time understanding this bit. Here’s what I suggest:
- Give the
foreach my $mname_ipa label:
MNAME_IP: foreach my $mname_ip ( keys %{ $mname_ns{$mname} } ){- Use
nextwith a LABEL instead oflast:
next MNAME_IP;- Get rid of the
$continuevariable.
Doing so will, IMHO, make the code state its intent more clearly.
I’ve implemented this change myself locally to ensure that this suggestion is sound, and unit tests still pass on my machine.
There was a problem hiding this comment.
I agree. There are three foreach-loops and it would maybe be easier of the enclosed ones are labeled.
There was a problem hiding this comment.
Besides one minor comment and the two comments by @marc-vanderwal it looks fine.
lib/Zonemaster/Engine/Test/Zone.pm
Outdated
| if ( not %mname_ns ){ | ||
| return ( @results, info( TEST_CASE_END => { testcase => (split /::/, (caller(0))[3])[-1] } ) ); | ||
| } |
There was a problem hiding this comment.
This follows the specification, but if this is removed then the same thing will happen after foreach my $mname ( keys %mname_ns ){ ... } and if ( $found_serial ){ ... } since the two statements will be false.
marc-vanderwal
added
the
S-ReleaseTested
|
Unit tests pass. Also, I was able to elicit the following messages which are not covered yet by unit tests:
However a more complex setup, which I do not yet have at my disposal, would be needed to check the following messages:
|
matsduf
added
the
V-Minor
Purpose
This PR proposes an update to the implementation of Zone01 as described in zonemaster/zonemaster#1028.
The corresponding update to the specification can be found in zonemaster/zonemaster#1032.
Context
Addresses zonemaster/zonemaster#1028.
How to test this PR
Tests should pass