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

Ability to run single testcase via test_zone() #1312

Merged
merged 10 commits into from
May 3, 2024

Conversation

mattias-p
Copy link
Member

@mattias-p mattias-p commented Dec 8, 2023

Purpose

Improve the semantics of the test_cases profile property to be more useful and less surprising. This is helpful for

  1. people wondering why a test case won't run even though they specified it in their profile, and
  2. applications providing flexible ways for users to specify what test cases to run.

Context

Prerequisite for zonemaster/zonemaster-cli#359.

Changes

This PR the behavior of Zonemaster::Engine->test_zone() (and its plumbing). It is updated in some important ways:

  1. The model for dependencies between test cases is improved. Let's say test case Y has a starting condition that test case X has given a certain signal, but X is disabled in the profile. Before this PR there was no way to run Y without also running X. After this PR disabling X automatically gives the starting signal for Y.
  2. Basic01 and Basic02 now respect the test_cases profile property.
  3. The Basic test module used to have a special status and was given special treatment in various places. One particular thing the special privilege was used for was to signal that a zone is so broken that no further testing should be done. After this PR there is no discrimination between modules. Any module could in principle signal fatal brokenness — though in practice Basic is the only model that ever does.
  4. In some cases when a test case is skipped because its starting condition fails, a special message is emitted. This PR makes it so these messages are only emitted if the skipped test case is enabled in the profile.

This PR also includes a bug fix:

  1. A stray TEST_CASE_END message that was emitted from Zonemaster::Engine::Test::DNSSEC->all() is removed.

How to test this PR

Automated tests:

  • A unit test is included ensuring that when you 1) enable any single test case in the profile, and 2) run all enabled test cases, the enabled test case is run exactly once and no other test cases are run.

Manual tests:

  • Run zonemaster-cli --test dnssec and verify that it doesn't emit stray TEST_CASE_END messages.

Tests that ought to be automated as part of this PR but that I'm lacking test data for:

  • Verify that enabled test cases can disable dependent test cases. Here I've compiled a list of all signals involved in dependencies between test cases. Each entry consists of three sets. The first one is a set of signaling test cases, the second is a set of test cases with a starting condition, the third is a set of messages involved in the signal. I don't have test zones for any of these, but if you have some I'll add unit tests for them.
    1. ({basic01}, {basic02, basic03}, {B01_CHILD_FOUND})
    2. ({basic02}, {basic03}, {B02_AUTH_RESPONSE_SOA})
    3. ({basic02}, all test cases in all test modules other than Basic, {B02_NO_DELEGATION, B02_AUTH_RESPONSE_SOA, a somewhat external "is-undelegated" property})
    4. ({address02}, {address03}, {NAMESERVERS_IP_WITH_REVERSE})
    5. ({dnssec07}, all other test cases in the DNSSEC test module, {NEITHER_DNSKEY_NOR_DS})
    6. ({dnssec07}, {dnssec06}, {DNSKEY_BUT_NOT_DS, DNSKEY_AND_DS})
    7. ({syntax01}, {syntax04, syntax05, syntax06, syntax07, syntax08}, {ONLY_ALLOWED_CHARS})
    8. ({syntax05}, {syntax06, syntax07}, {NO_RESPONSE_SOA_QUERY})
    9. ({zone08}, {zone09}, {NO_RESPONSE_MX_QUERY})
    10. ({zone02, zone03, zone04, zone05, zone06, zone07}, {zone10, zone11}, {NO_RESPONSE_SOA_QUERY})
  • Verify that the special skipping messages are emitted if and only if their associated test case is both skipped and enabled in the profile. Here I've compiled a list of these special messages. Each entry consists of a pair of a test case and a special message. I don't have test zones for any of these, but if you have some I'll add unit tests for them.
    1. (basic03, HAS_NAMESERVER_NO_WWW_A_TEST)
    2. (dnssec06, ADDITIONAL_DNSKEY_SKIPPED)

@mattias-p
Copy link
Member Author

@marc-vanderwal, @matsduf, @tgreenx, @hannaeko Would you guys have time to have a look at this? If this PR is approved I could build on top of it for the custom test modules feature design.

@marc-vanderwal
Copy link
Contributor

@marc-vanderwal, @matsduf, @tgreenx, @hannaeko Would you guys have time to have a look at this? If this PR is approved I could build on top of it for the custom test modules feature design.

I won’t have time today, but I can take a more thorough look at it tomorrow.

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.

Well, apart from that comment, it looks good to me.

@mattias-p
Copy link
Member Author

I've rebased onto develop. Please review again.

@matsduf
Copy link
Contributor

matsduf commented Apr 8, 2024

A lot of changes in MANIFEST, but any real change?

@matsduf
Copy link
Contributor

matsduf commented Apr 8, 2024

I do not understand what this PR wants to change. Could you clarify the purpose?

@mattias-p
Copy link
Member Author

A lot of changes in MANIFEST, but any real change?

I probably ran make manifest.

I do not understand what this PR wants to change. Could you clarify the purpose?

I'm not sure what you're looking for. Maybe you could rephrase your question in the language of the Purpose and Changes sections of the PR description?

@matsduf
Copy link
Contributor

matsduf commented Apr 10, 2024

Could you clarify the purpose of this PR? Could you also clarify what is changed by this PR?

@mattias-p
Copy link
Member Author

@matsduf We talked about this offline.
@marc-vanderwal I've dealt with your comment and rebased onto develop since your last review.
Could you have another look at this?

marc-vanderwal
marc-vanderwal previously approved these changes May 2, 2024
@matsduf
Copy link
Contributor

matsduf commented May 2, 2024

This PR the behavior of Zonemaster::Engine->test_zone() (and its plumbing). It is updated in some important ways:

  1. The model for dependencies between test cases is improved. Let's say test case Y has a starting condition that test case X has given a certain signal, but X is disabled in the profile. Before this PR there was no way to run Y without also running X. After this PR disabling X automatically gives the starting signal for Y.

Is the model defined anywhere?

@marc-vanderwal
Copy link
Contributor

If by that you mean that there is a data structure, such as a graph, that defines those dependency relationships, then no; this model is defined in code by a series of if statements.

I’d say that the code could be refactored so that each test case is a method with appropriate subroutine attributes. We could get rid of some repetitive boilerplate and encode dependencies between subroutines that way. The downside is that this technique is pretty advanced Perl and requires some brutal metaprogramming.

@matsduf
Copy link
Contributor

matsduf commented May 2, 2024

I expected a model, "this is how dependencies should be implemented", but is sound like there is just the implementations.

I think the dependency, if there should be any, should be defined in the specifications.

matsduf
matsduf previously approved these changes May 2, 2024
lib/Zonemaster/Engine.pm Outdated Show resolved Hide resolved
lib/Zonemaster/Engine/Test.pm Outdated Show resolved Hide resolved
@mattias-p mattias-p dismissed stale reviews from matsduf and marc-vanderwal via ed15f4d May 3, 2024 09:09
mattias-p and others added 2 commits May 3, 2024 11:09
Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>
Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>
@mattias-p mattias-p merged commit f6d0ef2 into zonemaster:develop May 3, 2024
3 checks passed
@matsduf matsduf added V-Minor Versioning: The change gives an update of minor in version. V-Major Versioning: The change gives an update of major in version. and removed V-Minor Versioning: The change gives an update of minor in version. labels May 3, 2024
@marc-vanderwal
Copy link
Contributor

I was able to test most of the changes introduced by this PR. Although I could not be exhaustive with my testing, I am confident enough that there are no regressions.

Manual tests:

  • Run zonemaster-cli --test dnssec and verify that it doesn't emit stray TEST_CASE_END messages.

Success: I ran zonemaster-cli --level DEBUG --test dnssec --no-ipv6 afnic.fr | grep -E 'TEST_CASE_(START|END)'. Each TEST_CASE_END message matches a single TEST_CASE_START message.

Tests that ought to be automated as part of this PR but that I'm lacking test data for:

  • Verify that enabled test cases can disable dependent test cases. Here I've compiled a list of all signals involved in dependencies between test cases. Each entry consists of three sets. The first one is a set of signaling test cases, the second is a set of test cases with a starting condition, the third is a set of messages involved in the signal. I don't have test zones for any of these, but if you have some I'll add unit tests for them.

    1. ({basic01}, {basic02, basic03}, {B01_CHILD_FOUND})

Partially tested. Testing with afnic.fr: Basic01 emits B01_CHILD_FOUND and Basic02 runs (Basic03 is then inhibited). Testing with not-a-zone.afnic.fr: Basic01 emits B01_NO_CHILD and neither Basic02 nor Basic03 run.

    1. ({basic02}, {basic03}, {B02_AUTH_RESPONSE_SOA})

Couldn’t test exhaustively, but with a domain that doesn’t cause B02_AUTH_RESPONSE_SOA to be emitted, such as cocotier.re, Basic03 does not seem to run.

    1. ({basic02}, all test cases in all test modules other than Basic, {B02_NO_DELEGATION, B02_AUTH_RESPONSE_SOA, a somewhat external "is-undelegated" property})

With a broken domain such as cocotier.re, which doesn’t elicit B02_AUTH_RESPONSE_SOA, none of the tests in other modules are performed.

    1. ({address02}, {address03}, {NAMESERVERS_IP_WITH_REVERSE})

Successfully tested: with afnic.fr, which elicits NAMESERVERS_IP_WITH_REVERSE, causing Address03 to run; with chien-perdu-76.fr, which doesn’t because one of its two name servers has no reverse name for its IP, Address03 is skipped.

    1. ({dnssec07}, all other test cases in the DNSSEC test module, {NEITHER_DNSKEY_NOR_DS})

Successfully tested with chien-perdu-76.fr, which elicits NEITHER_DNSKEY_NOR_DS, and afnic.fr, which does not. In the former case, only DNSSEC07 is run; in the latter, more test cases in the DNSSEC module are run.

    1. ({dnssec07}, {dnssec06}, {DNSKEY_BUT_NOT_DS, DNSKEY_AND_DS})

Successfully tested: with afnic.fr, which elicits DNSKEY_AND_DS, DNSSEC06 is run; with chien-perdu-76.fr, which does not elicit DNSKEY_AND_DS, DNSSEC06 is skipped; with 3mimmobilier.fr, which elicits DNSKEY_BUT_NOT_DS, DNSSEC06 is run.

    1. ({syntax01}, {syntax04, syntax05, syntax06, syntax07, syntax08}, {ONLY_ALLOWED_CHARS})

Cannot test: none of the available user interfaces allow a test to start when a domain name contains invalid characters.

    1. ({syntax05}, {syntax06, syntax07}, {NO_RESPONSE_SOA_QUERY})
    1. ({zone08}, {zone09}, {NO_RESPONSE_MX_QUERY})
    1. ({zone02, zone03, zone04, zone05, zone06, zone07}, {zone10, zone11}, {NO_RESPONSE_SOA_QUERY})

Cannot test these three: I was unable to find a zone that elicited NO_RESPONSE_SOA_QUERY or NO_RESPONSE_MX_QUERY. Testing these will require IBDNS.

  • Verify that the special skipping messages are emitted if and only if their associated test case is both skipped and enabled in the profile. Here I've compiled a list of these special messages. Each entry consists of a pair of a test case and a special message. I don't have test zones for any of these, but if you have some I'll add unit tests for them.

    1. (basic03, HAS_NAMESERVER_NO_WWW_A_TEST)

Successfully tested with afnic.fr and a profile disabling Basic03.

    1. (dnssec06, ADDITIONAL_DNSKEY_SKIPPED)

Cannot test: I was unable to find a zone that elicited ADDITIONAL_DNSKEY_SKIPPED.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ReleaseTested Status: The PR has been successfully tested in release testing V-Major Versioning: The change gives an update of major in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants