Skip to content

New unit tests for Delegation01, -02 and -03, and legacy tests are removed#1395

Merged
matsduf merged 15 commits into
zonemaster:developfrom
matsduf:new-unittests-delegation01
Nov 27, 2024
Merged

New unit tests for Delegation01, -02 and -03, and legacy tests are removed#1395
matsduf merged 15 commits into
zonemaster:developfrom
matsduf:new-unittests-delegation01

Conversation

@matsduf

@matsduf matsduf commented Nov 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Purpose

This PR implements the scenarios and test zones for Delegation01, Delegation02 and Delegation03 defined in zonemaster/zonemaster#1305.

This PR is built on top of commit b276cf8 in PR #1393 to get updated version of t/TestUtil.pm.

Old legacy unit tests files are removed. The zones behind most of them will soon be removed.

In unit test file t/Test-delegation.t legacy tests and commented code have been removed. The file, however, cannot be rerecorded due to changes in zones not controlled by Zonemaster.

Some scenarios for Delegation01 cannot be included due to bug or bugs in the implementation (not in the scope of this PR):

t/Test-delegation01.t .. 1/? Untested scenarios:
	Scenario ENOUGH-DEL-NOT-CHILD cannot be tested.
	Scenario IPV4-AND-DEL-OK-NO-IPV6-CHILD cannot be tested.
	Scenario IPV6-AND-DEL-OK-NO-IPV4-CHILD cannot be tested.
	Scenario MISMATCH-DELEGATION-CHILD-1 cannot be tested.
	Scenario MISMATCH-DELEGATION-CHILD-2 cannot be tested.

Some scenarios for Delegation02 cannot be included due to bug or bugs in the implementation (not in scope of this PR):

t/Test-delegation02.t .. 1/? Untested scenarios:
	Scenario CHILD-NON-DISTINCT cannot be tested.
	Scenario DEL-NON-DISTINCT cannot be tested.

Please note that scenario CHILD-NON-DISTINCT and CHILD-NON-DISTINCT-UND are the same except for that the latter is undelegated. And the latter passes. The same for DEL-NON-DISTINCT and DEL-NON-DISTINCT-UND. This shows that the code does not handle normal and undelegated tests in the sama way. The zone content "poisons" the delegation.

No issues with Delegation03.

Context

zonemaster/zonemaster#1305.

This PR is built on top of #1398. Commit e9060d1 does not belong to this PR.

Changes

...

How to test this PR

@matsduf matsduf added this to the v2024.2 milestone Nov 2, 2024
@matsduf matsduf marked this pull request as draft November 2, 2024 16:03
@matsduf matsduf changed the title New unittests for Delegation01, legacy tests removed New unit tests for Delegation01, legacy tests removed Nov 4, 2024
@matsduf matsduf force-pushed the new-unittests-delegation01 branch from 5039ed5 to 076c30c Compare November 4, 2024 14:04
@matsduf matsduf mentioned this pull request Nov 4, 2024
Merged
@matsduf matsduf changed the title New unit tests for Delegation01, legacy tests removed New unit tests for Delegation01, -02 and -03, and legacy tests are removed Nov 6, 2024
@matsduf matsduf marked this pull request as ready for review November 6, 2024 15:45
@matsduf matsduf added the V-Patch Versioning: The change gives an update of patch in version. label Nov 8, 2024
@matsduf matsduf modified the milestones: v2024.2, v2025.1 Nov 12, 2024
@matsduf matsduf force-pushed the new-unittests-delegation01 branch from 9e7362c to 2433036 Compare November 13, 2024 16:47
@matsduf

matsduf commented Nov 13, 2024

Copy link
Copy Markdown
Contributor Author

@mattias-p, @marc-vanderwal, @tolvmannen, @tgreenx -- please review.

marc-vanderwal
marc-vanderwal reviewed Nov 21, 2024
View reviewed changes
Comment thread t/Test-delegation01.t Outdated
marc-vanderwal
marc-vanderwal reviewed Nov 21, 2024
View reviewed changes

@marc-vanderwal marc-vanderwal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks fine. Just some really minor nits.

Comment thread t/Test-delegation02.t Outdated
@matsduf matsduf force-pushed the new-unittests-delegation01 branch from e8f3990 to e88bef4 Compare November 22, 2024 09:40
@matsduf

matsduf commented Nov 22, 2024

Copy link
Copy Markdown
Contributor Author

@marc-vanderwal, please re-review.

marc-vanderwal
marc-vanderwal previously approved these changes Nov 25, 2024
View reviewed changes
@matsduf matsduf dismissed marc-vanderwal’s stale review November 25, 2024 09:06

The merge-base changed after approval.

marc-vanderwal
marc-vanderwal previously approved these changes Nov 25, 2024
View reviewed changes
@matsduf matsduf dismissed marc-vanderwal’s stale review November 25, 2024 10:47

The merge-base changed after approval.

@matsduf

matsduf commented Nov 25, 2024

Copy link
Copy Markdown
Contributor Author

@marc-vanderwal, I do not know what "The merge-base changed after approval" really means. I did not do anything.

image

@marc-vanderwal

marc-vanderwal commented Nov 25, 2024

Copy link
Copy Markdown
Contributor

Isn’t it because PRs are being merged in the develop branch and that this PR is being rebased continuously on the latest develop? I’m not sure either. Weird.

marc-vanderwal
marc-vanderwal previously approved these changes Nov 25, 2024
View reviewed changes
@matsduf matsduf dismissed marc-vanderwal’s stale review November 25, 2024 12:47

The merge-base changed after approval.

@matsduf

matsduf commented Nov 25, 2024

Copy link
Copy Markdown
Contributor Author

Isn’t it because PRs are being merged in the develop branch and that this PR is being rebased continuously on the latest develop? I’m not sure either. Weird.

It has only been rebased once, and that was on top of ommit e9060d1. I have never seen this behavior before.

tgreenx
tgreenx previously approved these changes Nov 26, 2024
View reviewed changes
@matsduf matsduf dismissed tgreenx’s stale review November 26, 2024 13:57

The merge-base changed after approval.

@tgreenx

tgreenx commented Nov 26, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Isn’t it because PRs are being merged in the develop branch and that this PR is being rebased continuously on the latest develop? I’m not sure either. Weird.

It has only been rebased once, and that was on top of ommit e9060d1. I have never seen this behavior before.

Looks like a known bug in Github: https://github.com/orgs/community/discussions/58535?sort=top

Re-selecting the base branch seems to make it go away.

@matsduf

matsduf commented Nov 27, 2024

Copy link
Copy Markdown
Contributor Author

After the approval by @marc-vanderwal there as been no change, just execution of the Github bug.

@matsduf matsduf merged commit 67959ea into zonemaster:develop Nov 27, 2024
@matsduf matsduf deleted the new-unittests-delegation01 branch November 29, 2024 12:33
@tgreenx tgreenx mentioned this pull request May 6, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tgreenx tgreenx tgreenx approved these changes
@mattias-p mattias-p Awaiting requested review from mattias-p
@tolvmannen tolvmannen Awaiting requested review from tolvmannen
@MichaelTimbert MichaelTimbert Awaiting requested review from MichaelTimbert
@marc-vanderwal marc-vanderwal Awaiting requested review from marc-vanderwal

Assignees

No one assigned

Labels

V-Patch Versioning: The change gives an update of patch in version.

Projects

None yet

Milestone

v2024.2

Development

Successfully merging this pull request may close these issues.

3 participants

@matsduf @marc-vanderwal @tgreenx