Skip to content

Conversation

@tgreenx
Copy link
Contributor

@tgreenx tgreenx commented May 14, 2025

Purpose

This PR proposes an update to the implementation of Address01 following a specification update.

Context

Based on:

Changes

  • lib/Zonemaster/Engine/Constants.pm
  • lib/Zonemaster/Engine/Test/Address.pm
  • share/profile.{json, yaml}
  • t/Test-address.t : updated and refactored
  • t/Test-address01.t : added

How to test this PR

Unit tests are added/updated and should pass.

@tgreenx tgreenx added this to the v2025.1 milestone May 14, 2025
@tgreenx tgreenx added A-TestCase Area: Test case specification or implementation of test case V-Patch Versioning: The change gives an update of patch in version. labels May 14, 2025
@tgreenx tgreenx changed the title Update Address01 implementation and unit tests Update Address01 implementation May 14, 2025
@tgreenx tgreenx force-pushed the update-address01 branch 2 times, most recently from 0ae205d to ec92cd4 Compare May 20, 2025 13:28
@matsduf matsduf modified the milestones: v2025.1, v2025.2 Jun 4, 2025
@matsduf matsduf added the RC-Fixes Release category: Fixes. label Aug 10, 2025
@tgreenx tgreenx added RC-Features Release category: Features. and removed RC-Fixes Release category: Fixes. labels Oct 30, 2025
@tgreenx tgreenx requested review from matsduf and tolvmannen October 30, 2025 17:39
Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is still in draft state.

@tgreenx tgreenx force-pushed the update-address01 branch 3 times, most recently from 97196c3 to 1df45d3 Compare November 6, 2025 16:30
@tgreenx tgreenx marked this pull request as ready for review November 6, 2025 16:30
@tgreenx
Copy link
Contributor Author

tgreenx commented Nov 6, 2025

@tolvmannen @matsduf This PR is now ready for review. Note that unit tests are updated and should all pass, which is the case for specific test scenarios of Address01 located in t/Test-address01.t, but for t/Test-address.t one test fails which is due to the same issue in other PRs (i.e. zut-root.rd.nic.fr is currently offline). It should be resolved early next week.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered using the format introduced by #1467 instead?

Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but all tests should pass before merge. Either if the problematic server comes up or if the failing tests are marked as TODO. In the latter case the tests could possibly replaced by scenario based tests. The scenarios for this test case pass.

Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two scenarios fail when testing with zonemaster-cli:

| Scenario name         | Mandatory message tag                                                         | Forbidden message tags |                                         
|:----------------------|:------------------------------------------------------------------------------|:-----------------------|                                         
| GOOD-1                | A01_GLOBALLY_REACHABLE_ADDR                                                   | 2)                     |                                         

* (2) All tags except for those specified as “Mandatory message tags”

```                                                                                                                                                                        
$ zonemaster-cli --show-testcase --level INFO --test address01 --hints ../../COMMON/hintfile --raw good-1.address01.xa                                                     
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v8.0.0                                                                                                             
   0.03 CRITICAL Address01      A01_NO_NAME_SERVERS_FOUND                                                                                                                  
   0.03 INFO     Address01      A01_GLOBALLY_REACHABLE_ADDR                                                                                                                
```                                                                                                                                                                        
--> Not OK, *A01_NO_NAME_SERVERS_FOUND* unexpected.

and

| Scenario name         | Mandatory message tag                                                         | Forbidden message tags |                                         
|:----------------------|:------------------------------------------------------------------------------|:-----------------------|                                         
| NO-NAME-SERVERS       | A01_NO_NAME_SERVERS_FOUND   | 2)                     |                                                                                           

* (2) All tags except for those specified as “Mandatory message tags”

```                                                                                                                                                                        
$ zonemaster-cli --show-testcase --level INFO --test address01 --hints ../../COMMON/hintfile --raw no-name-servers.address01.xa                                            
   0.00 INFO     Unspecified    GLOBAL_VERSION  version=v8.0.0                                                                                                             
   0.04 CRITICAL Address01      A01_NO_NAME_SERVERS_FOUND                                                                                                                  
   0.04 INFO     Address01      A01_GLOBALLY_REACHABLE_ADDR                                                                                                                
```                                                                                                                                                                        
--> Not OK, *A01_GLOBALLY_REACHABLE_ADDR* unexpected.

@matsduf
Copy link
Contributor

matsduf commented Nov 10, 2025

I have also a change request on the specification to make it easier to follow the behavior of the code, see my comments at zonemaster/zonemaster#1284.

@tgreenx tgreenx requested a review from matsduf November 13, 2025 11:23
@tgreenx
Copy link
Contributor Author

tgreenx commented Nov 13, 2025

@matsduf @tolvmannen please re-review

Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks fine now. Only recreation of the data file is needed.

@matsduf matsduf dismissed their stale review November 13, 2025 21:44

No real changes are needed.

@tgreenx
Copy link
Contributor Author

tgreenx commented Nov 14, 2025

Only recreation of the data file is needed.

@matsduf @tolvmannen Done. Also updated message ids to "address(es)". Please re-review.

@tgreenx tgreenx requested a review from matsduf November 14, 2025 08:51
@tgreenx tgreenx merged commit cbf9b78 into zonemaster:develop Nov 14, 2025
3 checks passed
@tgreenx tgreenx deleted the update-address01 branch November 14, 2025 10:01
@marc-vanderwal
Copy link
Contributor

I successfully tested this PR for the 2025.2 release.

On my development setup, unit tests pass and t/test-Address01.t, which is introduced by this PR, is being executed. No errors occurred.

Some basic testing with internal domains which have private IPv4 addresses cause messages such as “IP address(es) intended for local use on network or service provider level” to appear in the output. On the other hand, other zones give “Globally reachable IP address(es)”.

@marc-vanderwal marc-vanderwal added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Dec 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-TestCase Area: Test case specification or implementation of test case RC-Features Release category: Features. S-ReleaseTested Status: The PR has been successfully tested in release testing V-Patch Versioning: The change gives an update of patch in version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants