Extend --test option to allow passing only a testcase #333
Conversation
ghost
requested a review
from 
matsduf
left a comment
matsduf
left a comment
There was a problem hiding this comment.
This is a change that I have wanted for a long time!
ghost
mentioned this pull request
| my ( $module, $method ) = split( '/', lc($t), 2 ); | ||
| if ( $module =~ /^([a-z]+)[0-9]+$/ ) { | ||
| $method = $module; | ||
| $module = $1; | ||
| } |
There was a problem hiding this comment.
If $module matches a test case identifier then $method can be any garbage. I guess that is OK in this context.
There was a problem hiding this comment.
I think this is a good point and that it should be fixed. Caring about proper input validation is a good thing.
There was a problem hiding this comment.
Yes, and the error message has room for improvement. Currently, this happens:
$ zonemaster-cli --test bogus42 domain.example
Seconds Level Message
======= ======== =======
0.00 CRITICAL Request to run bogus42 in unknown module bogus. Known modules: Address:Connectivity:Consistency:DNSSEC:Delegation:Nameserver:Syntax:Zone.
It would be more useful if the error message suggests the use of --list-tests for correct inputs. But that’s a problem best addressed in another PR.
There was a problem hiding this comment.
Good idea. Currently the message comes from the Engine which has no idea of the --list-tests CLI option. I guess it would mean rethinking how this message/error is emitted.
But that’s a problem best addressed in another PR.
I'll open an issue with your suggestion.
matsduf
dismissed
their stale review
This is a good change, but there are possible improvements.
marc-vanderwal
left a comment
marc-vanderwal
left a comment
There was a problem hiding this comment.
Good idea, but I think there is room for improvement.
Following commands are identical: zonemaster-cli --test delegation/delegation01 zonemaster-cli --test delegation01 Passing a trailing '/' assumes a module should be run. Credits to @marc-vanderwal for most of this code.
matsduf
left a comment
matsduf
left a comment
There was a problem hiding this comment.
I think this is a great improvement.
|
I'd approve it, but it’s best if someone else had a look at the code. |
tgreenx
added
the
V-Minor
v2023.2 Release TestingI successfully tested this on Rocky Linux 8.9. |
mattias-p
added
the
S-ReleaseTested
4 participants
Purpose
Make it possible to only pass the name of a testcase to the
--testoption.The following commands are now both ok and identical:
Context
Fixes #232
Changes
--testoptionHow to test this PR