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.

Sign up for GitHub

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

Jump to bottom

Extend --test option to allow passing only a testcase #333

Merged
2 commits merged into from Nov 29, 2023
Merged

Extend --test option to allow passing only a testcase #333

2 commits merged into from Nov 29, 2023

Conversation

ghost
Copy link

@ghost ghost commented May 25, 2023

Purpose

Make it possible to only pass the name of a testcase to the --test option.

The following commands are now both ok and identical:

zonemaster-cli --test basic/basic01
zonemaster-cli --test basic01

Context

Fixes #232

Changes

  • allow passing only a testcase name to the --test option

How to test this PR

  • run all testcases from the Basic module: 
    zonemaster-cli --test basic
  • run only one testcase 
    zonemaster-cli --test basic/basic01
zonemaster-cli --test basic01

@ghost ghost added this to the v2023.2 milestone May 25, 2023
@ghost ghost requested a review from matsduf May 25, 2023 09:09
matsduf
matsduf requested changes May 25, 2023
View reviewed changes
Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change that I have wanted for a long time!

lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
matsduf
matsduf reviewed Jun 30, 2023
View reviewed changes
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
@ghost ghost mentioned this pull request Jul 3, 2023
Merged
@ghost ghost requested a review from matsduf July 4, 2023 11:57
matsduf
matsduf reviewed Jul 4, 2023
View reviewed changes
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
matsduf
matsduf reviewed Jul 20, 2023
View reviewed changes
lib/Zonemaster/CLI.pm Outdated
Comment on lines 625 to 631
my ( $module, $method ) = split( '/', lc($t), 2 );
if ( $module =~ /^([a-z]+)[0-9]+$/ ) {
$method = $module;
$module = $1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If $module matches a test case identifier then $method can be any garbage. I guess that is OK in this context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good point and that it should be fixed. Caring about proper input validation is a good thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
matsduf previously approved these changes Jul 20, 2023
View reviewed changes
mattias-p
mattias-p reviewed Jul 21, 2023
View reviewed changes
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
mattias-p
mattias-p reviewed Jul 21, 2023
View reviewed changes
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
@mattias-p mattias-p mentioned this pull request Jul 21, 2023
Open
@matsduf matsduf dismissed their stale review July 21, 2023 13:38

This is a good change, but there are possible improvements.

@tgreenx tgreenx linked an issue Sep 7, 2023 that may be closed by this pull request
Update the --test option #232
Open
marc-vanderwal
marc-vanderwal reviewed Nov 22, 2023
View reviewed changes
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.

Good idea, but I think there is room for improvement.

Extend --test option to pass only a test case
46d0d97 
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
matsduf previously approved these changes Nov 22, 2023
View reviewed changes
Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great improvement.

@marc-vanderwal
Copy link
Contributor

I'd approve it, but it’s best if someone else had a look at the code.

@mattias-p mattias-p mentioned this pull request Nov 23, 2023
Open
tgreenx
tgreenx reviewed Nov 23, 2023
View reviewed changes
Copy link
Contributor

@tgreenx tgreenx left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and it works as advertised. This is probably not a breaking change since the old behavior is kept so V-Minor, right?

lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Show resolved Hide resolved
@tgreenx tgreenx added the V-Minor Versioning: The change gives an update of minor in version. label Nov 23, 2023
mattias-p
mattias-p reviewed Nov 23, 2023
View reviewed changes
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
script/zonemaster-cli Show resolved Hide resolved
@ghost ghost merged commit e6c1504 into zonemaster:develop Nov 29, 2023
1 check passed
@ghost ghost deleted the extend-option-test branch November 29, 2023 09:04
@tgreenx tgreenx mentioned this pull request Nov 29, 2023
Merged
@matsduf matsduf mentioned this pull request Dec 11, 2023
Closed
@mattias-p
Copy link
Member

v2023.2 Release Testing

I successfully tested this on Rocky Linux 8.9.

@mattias-p mattias-p added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jan 15, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hannaeko hannaeko Awaiting requested review from hannaeko

Assignees
No one assigned
Labels
S-ReleaseTested Status: The PR has been successfully tested in release testing V-Minor Versioning: The change gives an update of minor in version.
Projects
None yet
Milestone
v2023.2
Development

Successfully merging this pull request may close these issues.

Update the --test option
4 participants
@marc-vanderwal @mattias-p @matsduf @tgreenx