Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
Nov 27, 2024

Conversation

matsduf
Copy link
Contributor

@matsduf matsduf commented Nov 2, 2024

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 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
Copy link
Contributor Author

matsduf commented Nov 13, 2024

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

Copy link
Contributor

@marc-vanderwal marc-vanderwal left a comment

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.

t/Test-delegation02.t Outdated Show resolved Hide resolved
@matsduf matsduf force-pushed the new-unittests-delegation01 branch from e8f3990 to e88bef4 Compare November 22, 2024 09:40
@matsduf
Copy link
Contributor Author

matsduf commented Nov 22, 2024

@marc-vanderwal, please re-review.

marc-vanderwal
marc-vanderwal previously approved these changes Nov 25, 2024
@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
@matsduf matsduf dismissed marc-vanderwal’s stale review November 25, 2024 10:47

The merge-base changed after approval.

@matsduf
Copy link
Contributor Author

matsduf commented Nov 25, 2024

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

image

@marc-vanderwal
Copy link
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
@matsduf matsduf dismissed marc-vanderwal’s stale review November 25, 2024 12:47

The merge-base changed after approval.

@matsduf
Copy link
Contributor Author

matsduf commented Nov 25, 2024

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
@matsduf matsduf dismissed tgreenx’s stale review November 26, 2024 13:57

The merge-base changed after approval.

@tgreenx
Copy link
Contributor

tgreenx commented Nov 26, 2024

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
Copy link
Contributor Author

matsduf commented Nov 27, 2024

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
3 checks passed
@matsduf matsduf deleted the new-unittests-delegation01 branch November 29, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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