More flexible --test option

[mattias-p]

Purpose

This PR implements flexible command line overriding of which test cases to run from the command line.

Context

• This is an extension to the improvements in Extend --test option to allow passing only a testcase #333.

• Replaces Better performance when running specified test. Function to exclude tests #130. The point of that PR is to allow users to exclude test cases from the command line. This PR includes the same functionality using a different command line syntax.

• Leverages Ability to run single testcase via test_zone() zonemaster-engine#1312 to properly handle the Basic test module (c.f. Better performance when running specified test. Function to exclude tests #130 (comment)).

• Handles the interaction of --test and --profile in accordance with Better performance when running specified test. Function to exclude tests #130 (comment).

• For what it's worth, the idea for this design came to me in a discussion about Refactoring of function Zonemaster::Engine::Test::run_one() zonemaster-engine#1294.

Changes

• The set of values accepted by --test is extended.

• The semantics are changed of repeated --test options. They used to be cumulative but now the last one takes precedence.

• Which test cases to include in the run is now controlled by setting the test_cases profile property and running Zonemaster::Engine->test_zone().

• When --test=basic or --test=basic02 is specified, and basic02 considers the domain too broken to test, a CANNOT_CONTINUE message is now emitted, whereas it wasn't emitted before this PR.

Reviewing

• All updates for this PR are contained in the very last commit. The commits before the last one all belong to Organize --help text #389 and should be disregarded when reviewing this PR.

• Look especially for lacking test coverage.

Testing

• Unit tests should pass.

[matsduf]

• Before this PR CLI would call either test_zone, test_module or test_method in Zonemaster::Engine
  to run the test. After this PR CLI always calls test_zone and uses the test_cases profile property to
  control which tests are run. This means that the Basic tests are always included even when a single test case
  is specified.

• Since the semantics of specifying a single test case or single test module is changed so that the Basic tests
  are included, an argument could be made that this is a breaking change, but I believe an argument could also
  be made that it's still just an addition.

That is a bad change. When working with test zones you want to be able to run just the specific test case and nothing else.

Secondly, with that change, if Basic fails completely no other test case is run, and that is not what we want when testing and creating test zones.

I am against a change that makes it impossible to run just the selected test case.

[mattias-p]

All I'm hearing is "no" and "bad". Is there anything about this proposal that you do like?

Also, I'm trying to understand your objection here. AFAICT you're saying that when a user requests that a single test case is run on a domain that doesn't fulfill the requirements posed by Basic, the requested test case still needs to run. Is that correct? And if so, could you explain why this is important, because it's not obvious to me.

[matsduf]

All I'm hearing is "no" and "bad". Is there anything about this proposal that you do like?

It would be good to be able to exclude test cases without creating a new profile. It would be good to be able to select two or more test cases and they are run without overlap.

Sometimes it is probably good that Basic is run before a selected test.

Also, I'm trying to understand your objection here. AFAICT you're saying that when a user requests that a single test case is run on a domain that doesn't fulfill the requirements posed by Basic, the requested test case still needs to run. Is that correct? And if so, could you explain why this is important, because it's not obvious to me.

I want to see what happens in that test case for the test zone even when there are errors. And I want minimal output without the extra output from Basic when testing test zones. Sometimes I have to run with --level debug which gives many lines, which will be anymore if Basic is run.

Give the possibility to run a test without adding Basic.

If Basic is not in the profile, why should Basic still be run? The magic could rather be to put the Basic test cases in the profile first, if there are any.

[mattias-p]

I want to see what happens in that test case for the test zone even when there are errors. And I want minimal output without the extra output from Basic when testing test zones. Sometimes I have to run with --level debug which gives many lines, which will be anymore if Basic is run.

If Basic is not in the profile, why should Basic still be run? The magic could rather be to put the Basic test cases in the profile first, if there are any.

Ok. Yeah, I see how this could be very valuable in development contexts. And I also don't particularly like that Basic is forced.

I have an idea of how we could simplify things in Engine in a way that supports this. I think I'll make a draft PR there too.

I won't make any more updates to the code here before Engine is in a better shape to support it. But if anyone has more feedback on the direction of this PR, please let me know. Maybe it'll affect the updates in Engine.

[matsduf]

@mattias-p, is this still relevant or can it be closed now?

[mattias-p]

This conflicts with #371 as well as some of its documentation improvement followups. I plan to rebase this PR once those things have been merged. It seems somewhat unreasonable to get this merged for 2024.2 so I postponed it.

[mattias-p]

I rebased this onto latest develop and fixed some typos added some unit tests. I also tightened some validations for sanity reasons, but that shouldn't have any effect on zonemaster-cli behavior.

[matsduf]

This is not true anymore. Please update description.

* This PR contains the commits from [Organize --help text #389](https://github.com/zonemaster/zonemaster-cli/pull/389), which should be merged before this one.

[matsduf]

Do --test=-dnssec and --test=all-dnssec mean the same thing?

---> No

Do --test= and --test=all mean the same thing?

---> No

[matsduf]

Suggested change

	runs the entire Nameserver test module, and nothing else.
	runs the entire Nameserver test module, including any test cases
	disabled by the profile, but nothing else.

[mattias-p]

I agree that the documentation should make this clear, but I don't think it needs to be repeated for every example. I'll add a separate paragraph clarifying how it interacts with other options.

[matsduf]

That could be fine too.

[matsduf]

Suggested change

	runs the entire DNSSEC test module except the DNSSEC05 test case, and nothing
	else.
	runs the entire DNSSEC test module including any test cases
	disabled by the profile, except the DNSSEC05 test case,
	and nothing else.

[matsduf]

Suggested change

	=item B<--test>=TESTCASE, B<--test>=TESTMODULE
	=item B<--test>=TESTCASE, B<--test>=TESTMODULE, B<--test>=EXPR

[matsduf]

Suggested change

	otherwise have run.
	otherwise have run, i.e. the test cases enabled by the profile.

[matsduf]

Suggested change

	otherwise have run.
	otherwise have run, i.e. the test cases enabled by the profile.

[matsduf]

Suggested change

	runs all test cases of all test modules except for the Nameserver test module in
	which only the Nameserver03 test case is run.
	runs all defined test cases of all defined test modules (disregardning the profile)
	except for the Nameserver test module in which only the Nameserver03 test
	case is run.

[matsduf]

Nice to have it consistent. Nice to be able to control what test cases to run without creating a custom profile.

[mattias-p]

Shouldn't it be --test=EXPR?

Fixing.

Do --test=-dnssec and --test=all-dnssec mean the same thing?

---> No

Do --test= and --test=all mean the same thing?

---> No

Agreed. In case you modify the test_cases property using --profile they won't be the same.

[mattias-p]

I think the current implementation is flawed regarding the interpretation of this command sequence:

printf "%s\n" "--test=-delegation01" > ~/.zonemaster/cli.args
zonemaster-cli example.com
zonemaster-cli --test=-delegation02 example.com

With the current proposal, in the first command delegation01 would be disabled, but in the second command it would be enabled again. This is because the second command would effectively be zonemaster-cli --test=-delegation01 --test=-delegation02 example.com and the second --test would override the first one. It would make much more sense if both --test arguments were applied as they appear on the command line. zonemaster-cli --test=-delegation01 --test=basic01 would still mean execute basic01 and nothing else.

[matsduf]

I think the current implementation is flawed regarding the interpretation of this command sequence:

printf "%s\n" "--test=-delegation01" > ~/.zonemaster/cli.args
zonemaster-cli example.com
zonemaster-cli --test=-delegation02 example.com

With the current proposal, in the first command delegation01 would be disabled, but in the second command it would be enabled again. This is because the second command would effectively be zonemaster-cli --test=-delegation01 --test=-delegation02 example.com and the second --test would override the first one. It would make much more sense if both --test arguments were applied as they appear on the command line. zonemaster-cli --test=-delegation01 --test=basic01 would still mean execute basic01 and nothing else.

I think it is correct that --test on the command line overrides --test in the configuration file. That is how it works in other cases, e.g. --level=debugon the command line overrides --level=info in the configuration file.

[mattias-p]

We have two kinds of option today: overwriting ones and aggregating ones. Most of our options overwrite, but --test, --ns and --ds aggregate. I'm arguing that it makes more sense to keep --test as an aggregating option than to change it into an overwriting one. I.e., --test=basic01 --test=+basic02 should be equivalent to --test=basic01+basic02, not to --test=+basic02.

[mattias-p]

I pushed an update makes --test aggregate again. It's in a separate commit in case you want to play with both the overwriting version and the aggregating one. The man page needs an update to clarify the semantics in of repeated --test options but I haven't got around to that.

I haven't forgotten your unresolved review comments, @matsduf. It's just that before I take care of them I want to settle this overwriting/aggregating business.

[matsduf]

We have two kinds of option today: overwriting ones and aggregating ones. Most of our options overwrite, but --test, --ns and --ds aggregate. I'm arguing that it makes more sense to keep --test as an aggregating option than to change it into an overwriting one. I.e., --test=basic01 --test=+basic02 should be equivalent to --test=basic01+basic02, not to --test=+basic02.

There is a clear difference between --ns and --ds, on one hand, and --test, on the other hand.

Options --ns and --ds are typically used for a specific zone. It is unlikely that you would put that into the the configuration file cli.args. Therefor it works to have them aggregating. On the other hand, it would also work to change them so that only the last one is used, but so that you can specify multiple NS or DS, respectively, for the option, e.g. with a + sign or in quotemarks.

Option --test can be used in cli.args instead of creating a custom profile. Then it should be possible to override that with the settings on the command line.

[matsduf]

I guess the aggregate version could work too, but it is quite complex and requires updated documentation that clearly states how it should work.

cli.arg	command line	resulting
--test basic	--test consistency	Only consistency will be run
--test consistency	--test basic	Only basic will be run
--test basic	--test +consistency	basic and consistency will be run

Also on the command line, --test basid and --test consistency can be combined, but the order will govern the result. That must be stated.