Refactoring of function Zonemaster::Engine::Test::run_one()

[tgreenx]

Purpose

This PR proposes an update of function Zonemaster::Engine::Test::run_one(). See the Changes section below.

Context

Fixes #1278 and #1284

Changes

• Harmonize input arguments (with respect to Zonemaster::Engine::Test::run_all_for() and Zonemaster::Engine::Test::run_module())
• Remove message tag TEST_ARGS
• Add conditional check for Test Case - trying to run a Test Case disabled by the profile will now return an error.
  --> note that this was a feature only used in CLI and is now moved there: Retain the possibility to start a Test Case disabled in the profile zonemaster-cli#361
• Add basic{01,02} to the default profile

How to test this PR

Tests should pass.

Also, attempting to run a Test Case disabled by the current profile will now return an error instead of running the test:

$ perl -MZonemaster::Engine -E 'Zonemaster::Engine::Profile->effective->set( 'test_cases', [ "address01" ] ); my @res = Zonemaster::Engine->test_method("Address", "address01", "zonemaster.net"); say $_ for @res; say "nothing" unless @res'

System:Unspecified:START_TIME string=2023-12-04 16:18:16 +0100; time_t=1701703096
System:Unspecified:TEST_TARGET module=Address; testcase=address01; zone=zonemaster.net
Address:Address01:TEST_CASE_START testcase=Address01
Address:Address01:NO_IP_PRIVATE_NETWORK
Address:Address01:TEST_CASE_END testcase=Address01

$ perl -MZonemaster::Engine -E 'Zonemaster::Engine::Profile->effective->set( 'test_cases', [ "address01" ] ); my @res = Zonemaster::Engine->test_method("Address", "address02", "zonemaster.net"); say $_ for @res; say "nothing" unless @res'

nothing

[matsduf]

I disagree with the change.

I do not understand why it should be impossible to run a specific test if it is not listed in the current profile. What problem does that solve?

[ghost]

I disagree with the change.

To better understand your position, do you disagree with all the changes in this PR or only with the part that is described as "Add conditional check for Test Case - trying to run a Test Case disabled by the profile will now return an error." in the PR's description?

[tgreenx]

I disagree with the change.

I do not understand why it should be impossible to run a specific test if it is not listed in the current profile. What problem does that solve?

I responded in #1284 (comment). We can continue the discussion in the issue if you want. Other opinions are welcome - to me it is logical to make that change, the configuration (profile) should take precedence in all cases.

[matsduf]

I disagree with the change.

To better understand your position, do you disagree with all the changes in this PR or only with the part that is described as "Add conditional check for Test Case - trying to run a Test Case disabled by the profile will now return an error." in the PR's description?

Yes, the change that I am against is

• Add conditional check for Test Case - trying to run a Test Case disabled by the profile will now return an error.

I have seen no explanation of the problem that it would solve so I do not understand why the flexibility should be removed.

The following sounds fine:

• Harmonize input arguments (with respect to Zonemaster::Engine::Test::run_all_for() and
  Zonemaster::Engine::Test::run_module())

The following I do not know what it changes. I probably affects the output, but I do not understand how. Could be fine.

• Remove message tag TEST_ARGS

[matsduf]

I responded in #1284 (comment). We can continue the discussion in the issue if you want. Other opinions are welcome - to me it is logical to make that change, the configuration (profile) should take precedence in all cases.

What problem do you see by allowing the user to run an unlisted test case? Why is this a change to the better?

[ghost]

What problem do you see by allowing the user to run an unlisted test case? Why is this a change to the better?

On one hand we can see it as a feature. On the other hand, if you deploy Zonemaster with a custom profile, it allows to bypass it and run a specific test.
Say you want to have Zonemaster installed that runs only one testcase and use the following configuration:

$ cat /etc/zonemaster/profile.zone01.json
{ "test_cases": [ "zone01" ] }

$ cat /etc/zonemaster/cli.args
--profile /etc/zonemaster/profile.zone01.json

Then you would expect your user to only be able to run testcase ZONE01. However with the current implementation you can bypass this limitation with the --test option, for instance --test zone/zone02.
If I understand correctly, this PR would solve this and only allow to run testcases defined in the used profile. I think this proposal makes sense.

[matsduf]

$ cat /etc/zonemaster/profile.zone01.json
{ "test_cases": [ "zone01" ] }

$ cat /etc/zonemaster/cli.args
--profile /etc/zonemaster/profile.zone01.json

Then you would expect your user to only be able to run testcase ZONE01. However with the current implementation you can bypass this limitation with the --test option, for instance --test zone/zone02. If I understand correctly, this PR would solve this and only allow to run testcases defined in the used profile. I think this proposal makes sense.

The limitation is not harder than you can create your own profile without any superuser privileges:

$ cat  ~/.zonemaster/profile.zone0102.json
{ "test_cases": [ "zone01", "zone02" ] }
 
$ cat ~/.zonemaster/cli.args
--profile ~/.zonemaster/profile.zone0102.json

I see no reason to remove the flexibility that we have today. You can test a test case not included in the profile without having to include it.

[ghost]

The limitation is not harder than you can create your own profile without any superuser privileges:

I agree. And I think this would be a good practice to do so (and not rely on a potential hidden feature).

You can test a test case not included in the profile without having to include it.

I think that for the time being I disagree with your point of view.
(Also by default, Zonemaster uses its internal profile which has all testcases defined. So the use case we're discussing about only happens on setups with an explicitly changed profile.)

[matsduf]

The limitation is not harder than you can create your own profile without any superuser privileges:

I agree. And I think this would be a good practice to do so (and not rely on a potential hidden feature).

Firstly, it means that your argument above is not valid. Secondly, it is not a hidden feature.

I still do not see what problem it would solve to introduce to limit the flexibility.

[tgreenx]

What problem do you see by allowing the user to run an unlisted test case? Why is this a change to the better?

When a user changes the profile in Engine it does so with a specific intent. In this case, such an intent is to explicitly disable a Test Case from being run by Engine. From my perspective, an interface (such as Zonemaster-CLI) to Engine should not be permitted to bypass the default configuration, at least not in nominal conditions. It is surprising to me that the same user, using Zonemaster-CLI, can run a test module which will not run one or several specific test cases (as expected), and then consequently be able to run such a disabled test case, all using the same interface argument (i.e. --test).

It also complexifies the documentation. So the main problems I see here are consistency and predictability.

[mattias-p]

This is the current documentation:

test_cases

An arrayref of names of implemented test cases as listed in the test case specifications. Default is an arrayref listing all the test cases.

Specifies which test cases to consider when a test module is asked to run of all of its test cases.

Test cases not included here can still be run individually.

The test cases basic00, basic01 and basic02 are always considered no matter if they're excluded from this property. This is because part of their function is to verify that the given domain name can be tested at all.

If we change the behavior we also need to update the documentation. Since this behavior is stated so explicitly I'd say it's a breaking change.

I don't think the current behavior has a lot of merit other than the fact that it's documented. I don't believe anyone consciously designed it that way. It's just a weird design. When we wrote the documentation for the profile properties our intention was just to document whatever the current behavior happened to be, and then possibly improve it later once the interface was well understood.

I think the change suggested in this PR is an improvement. It's a simple clean model that a test case is either available in a profile or not. I guess it would be helpful to have different error messages when attempting to run a completely unrecognized test case and when attempting to run a test case that's disabled in the effective profile.

While we're on the topic of changing the semantics of test_cases I'd like to suggest an additional change. Instead of letting Zonemaster Engine implicitly add basic00, basic01 and basic02 to test_cases, we should require that they are explicitly included. This raises the bar slightly when specifying a profile, but it makes things much clearer for whoever reads it.

[marc-vanderwal]

Looks fine. One might as well update or improve the documentation a bit for the affected code.

[tgreenx]

Yes, the change that I am against is

    Add conditional check for Test Case - trying to run a Test Case disabled by the profile will now return an error.

Decided to move the functionality to CLI, this PR is postponed while the change in CLI is proposed.

[tgreenx]

If I create a profile with Zone09 only and run that with zonemaster-cli --profile PROFILE then besides Zone09 also Basic01-02 will be run. If I instead run zonemaster-cli --test Zone/Zone09 then only Zone09 is run.

I want to keep the behavior of --test, especially when developing test zones for test scenarios for test cases. I just want to make sure that the zone has the expected behavior for the specific test case. The test zones are only created for the specific test case.

When unit tests are run for test cases, are also Basic1-2 run?

Will this PR removed the current behavior of --test?

The current behavior of --test is unchanged (provided zonemaster/zonemaster-cli#361 is merged as well when this PR is). These Basic tests cases are run regardless of profile configuration only when using the whole testing suite (so without --test for CLI), not when a single test module/case is run.

[tgreenx]

Maybe we need to clarify how Basic tests are run. I'm okay with all following proposal (and I might be missing other possibilities):

[ ... ]

  * clear documentation on how Basic test cases are run

[ ... ]

I'm going with this option. I started doing it in #1308, so it will be finalized there.

[matsduf]

* remove `basic0{0..3}` from the profile (but then you won't be able to run them specifically)

That is not acceptable. We must be able to run every test case when we troubleshooting something, or when we develop test zones.

[matsduf]

If the issues in zonemaster/zonemaster-cli#361 are fixed, and it is merged in combination with this, it will be fine.

There is a solution.

[tgreenx]

If the issues in zonemaster/zonemaster-cli#361 are fixed, and it is merged in combination with this, it will be fine.

I rebased on latest develop and it should now work properly. @matsduf @PNAX @mattias-p please re-review.

[tgreenx]

@matsduf I fixed one forgotten mention of basic00 in this PR, please re-approve.

[ghost]

v2023.2 release testing

Tested with the "How to test" section and works as expected.