zonemaster-cli icon indicating copy to clipboard operation
zonemaster-cli copied to clipboard

More flexible --test option

Open mattias-p opened this issue 2 years ago • 19 comments

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 #333.

  • Replaces #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 zonemaster/zonemaster-engine#1312 to properly handle the Basic test module (c.f. https://github.com/zonemaster/zonemaster-cli/pull/130#issuecomment-572948201).

  • Handles the interaction of --test and --profile in accordance with https://github.com/zonemaster/zonemaster-cli/pull/130#issuecomment-573012219.

  • For what it's worth, the idea for this design came to me in a discussion about https://github.com/zonemaster/zonemaster-engine/pull/1294.

Changes

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

  • 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

  • Look especially for lacking test coverage.

Testing

  • Unit tests should pass.

mattias-p avatar Nov 23 '23 08:11 mattias-p

  • 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.

matsduf avatar Nov 23 '23 08:11 matsduf

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.

mattias-p avatar Nov 23 '23 09:11 mattias-p

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.

matsduf avatar Nov 23 '23 12:11 matsduf

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.

mattias-p avatar Nov 23 '23 15:11 mattias-p