Skip to content

Remove Basic00 Test Case#1226

Merged
tgreenx merged 2 commits into
zonemaster:developfrom
tgreenx:remove-basic00
Dec 4, 2023
Merged

Remove Basic00 Test Case#1226
tgreenx merged 2 commits into
zonemaster:developfrom
tgreenx:remove-basic00

Conversation

@tgreenx

@tgreenx tgreenx commented Nov 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Purpose

This PR removes the Basic00 Test Case as it has been replaced by RequirementsAndNormalizationOfDomainNames (see #942).

Note that the remaining mention of Basic00 in the following files will get updated at release:

  • docs/public/specifications/tests/README.md
  • docs/public/specifications/tests/ImplementedTestCases.md
  • docs/public/specifications/tests/Basic-TP/README.md
  • docs/public/specifications/tests/TestMessages.md

Context

It was first removed by #942, then restored by #1123 because of missing implementation and usage. That is now done (see implementation in Engine, and usage in CLI and Backend)

Changes

  • Remove docs/public/specifications/tests/Basic-TP/basic00.md
  • Update docs/internal/test-requirements/TestRequirements.md

How to test this PR

There should not be relevant leftover files that mention Basic00 (besides the ones mentioned in the "Purpose" section above):

$ git log -1 --oneline
a7dfab04 (HEAD -> remove-basic00, origin/remove-basic00) Remove Basic00 Test Case

$ ack -i basic00
[ ... ]

@tgreenx tgreenx added the A-Documentation Area: Documentation only. label Nov 29, 2023
@tgreenx tgreenx added this to the v2023.2 milestone Nov 29, 2023
@mattias-p mattias-p mentioned this pull request Nov 29, 2023
Merged
mattias-p
mattias-p previously approved these changes Nov 29, 2023
View reviewed changes

@mattias-p mattias-p left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I had a look and I also found basic00 mentioned in docs/internal/templates/specifications/tests/TestCaseIdentifierSpecification.md. That should probably be removed as well but that doesn't seem critical.

@tgreenx

tgreenx commented Nov 29, 2023

Copy link
Copy Markdown
Contributor Author

LGTM. I had a look and I also found basic00 mentioned in docs/internal/templates/specifications/tests/TestCaseIdentifierSpecification.md. That should probably be removed as well but that doesn't seem critical.

Right, I somehow missed it, it is now removed in 745eef0.

@tgreenx tgreenx requested a review from mattias-p November 29, 2023 17:27
mattias-p
mattias-p previously approved these changes Nov 30, 2023
View reviewed changes
matsduf
matsduf reviewed Nov 30, 2023
View reviewed changes
Comment thread docs/internal/test-requirements/TestRequirements.md Outdated
Comment on lines +27 to +31
R00400| The zone name should consists of valid IDN or non-IDN ASCII labels (names). | |[RequirementsAndNormalizationOfDomainNames]
R00500| IDN labels (names) should be valid. |[RFC5890] |[RequirementsAndNormalizationOfDomainNames]
R00600| Non-IDN ASCII labels (names) should be valid. |[RFC1123] [RFC2782] |[RequirementsAndNormalizationOfDomainNames]
R00700| A DNS zone should have a parent zone from which it is delegated. | |[RequirementsAndNormalizationOfDomainNames]
R00800| A DNS zone should have at least one accessible name server that hosts it. | |[RequirementsAndNormalizationOfDomainNames]

@matsduf matsduf Nov 30, 2023

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.

The header of column four is "Test case" but the requirements document is not a test case. Maybe "Implementation specification" instead?

"RequirementsAndNormalizationOfDomainNames" is quite long and hard to parse. I suggest that e.g. the following is added to the text above the table:

In column four on links to test cases and to "Requirements and normalization of domain names in input" (in the column called "Normalization") are found.

@tgreenx tgreenx Nov 30, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, see cbe3ff4.

tgreenx added a commit to tgreenx/zonemaster that referenced this pull request Nov 30, 2023
@tgreenx
Update after review - editorial updates
cbe3ff4 
zonemaster#1226 (comment)
@tgreenx tgreenx requested review from matsduf and mattias-p November 30, 2023 15:04
mattias-p
mattias-p reviewed Nov 30, 2023
View reviewed changes
Comment thread docs/internal/test-requirements/TestRequirements.md Outdated
matsduf
matsduf reviewed Dec 1, 2023
View reviewed changes

@matsduf matsduf 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.

Fine besides the broken link.

@tgreenx
Remove Basic00 Test Case
1e88426 
It has been replaced by a new generic specification: docs/public/specifications/tests/RequirementsAndNormalizationOfDomainNames.md
@tgreenx
Update after review - editorial updates
45496a7 
zonemaster#1226 (comment)
@tgreenx

tgreenx commented Dec 4, 2023

Copy link
Copy Markdown
Contributor Author

Fine besides the broken link.

Fixed, please re-review.

@tgreenx tgreenx requested review from matsduf and mattias-p December 4, 2023 10:42
@tgreenx tgreenx merged commit 9dd9160 into zonemaster:develop Dec 4, 2023
@tgreenx tgreenx deleted the remove-basic00 branch December 4, 2023 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matsduf matsduf matsduf approved these changes
@hannaeko hannaeko Awaiting requested review from hannaeko
@marc-vanderwal marc-vanderwal Awaiting requested review from marc-vanderwal
@mattias-p mattias-p Awaiting requested review from mattias-p

Assignees

No one assigned

Labels

A-Documentation Area: Documentation only.

Projects

None yet

Milestone

v2023.2

Development

Successfully merging this pull request may close these issues.

3 participants

@tgreenx @mattias-p @matsduf