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.

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

Better performance when running specified test. Function to exclude tests #130

Closed
wants to merge 6 commits into from

Conversation

lowinger42
Copy link

Sometimes it is desired to skip some tests, for example in an Anycast DNS setup where all servers are in the same AS.

When running multiple test with --test, the performance is not that good since a Zonemaster::Engine->zone( $domain ) object is created for each specified test. This fixes this by only creating this object once

@mattias-p
Copy link
Member

Thanks for the pull request, and welcome! I'll come back with a review shortly.

@matsduf matsduf added A-Code T-Feature Type: New feature in software or test case description labels Jan 9, 2020
@mattias-p
Copy link
Member

This seems like a nice feature! Though I have few nits to pick.

First, we're only using the git master branch as a reference to the last release. We do all development on the develop branch. If you could rebase these changes onto the develop branch and change the target branch of the pull request to develop, that would be great. (You'll notice that we accidentally merged stuff into the master branch since last release and fixed it up by adding revert commits. Take care not to carry over those commits to the develop branch when rebasing. I'm sorry for the inconvenience.)

Second, the --test and --exclude_test options don't interact well. For instance you might want to use --test MODULE --exclude_test MODULE/METHOD but that would not work as expected. Making the --test and --exclude_test mutually exclusive would solve this problem. The mutual exclusiveness should be noted in the documentation for both options. (A well-designed tighter integration between the options could be added at a later time. If you want to implement the tighter integration yourself I suggest you do it in a second pull request after this one is accepted.)

Third, in my opinion the level of documentation is really good. USING.md should probably be updated with this new option. If you want to you can make a suggestion for such an update. But you don't have to. @sandoche2k, could you advice here?

@matsduf
Copy link
Contributor

matsduf commented Jan 9, 2020

The document uses the basic module as an example. That is probably a bad example since the basic module will always be run even if not selected. Module basic could probably not be excluded.

@matsduf matsduf changed the base branch from master to develop January 9, 2020 14:35
@matsduf
Copy link
Contributor

matsduf commented Jan 10, 2020

@lowinger42, I suggest that you rebase your branch on the develop branch. Your proposal should probably handle the basic module specially since the engine will always run the basic module.

@matsduf matsduf added this to the v2020.2 milestone Jan 10, 2020
@lowinger42
Copy link
Author

All good suggestions, I'll get back with some updated code, in the correct branch.
Thanks

@mattias-p
Copy link
Member

Your proposal should probably handle the basic module specially since the engine will always run the basic module.

Good catch! Actually, the special handling of Basic is implemented in Z::E::test_zone, which is not invoked by --test either. Oops.

I think both --test and --exclude test should work by updating the test_cases property of the effective profile, and then simply call Z::E::test_zone. This way we'd cover all the intricacies of inter-method dependencies.

The --test option should not read the previous value of the test_cases property, but rather construct a new arrayref from scratch. I.e. if --profile disables a certain case and --test enables that entire module, the entire module should be included in the new property value.

The --exclude_test option should get the test_cases property value and set it again with matching test cases removed.

If --exclude_test is handled after --test, they could coexist in a well-defined way.

Thoughts on this? @vlevigneron, @matsduf, @sandoche2k, @lowinger42.

I'm sorry @lowinger42 if you already started down a different path, and I fully understand if you don't feel like cleaning up old design flaws of ours. If that's the case, just say so and we'll take care of this. Otherwise, I'll be happy to guide you through any weeds :)

@matsduf
Copy link
Contributor

matsduf commented Jan 24, 2020

@vlevigneron, can you take a look at this and the suggestions from @mattias-p?

@matsduf matsduf modified the milestones: v2020.2, v2021.1 Jan 19, 2021
@matsduf matsduf modified the milestones: v2021.1, v2021.2 May 27, 2021
@matsduf matsduf modified the milestones: v2021.2, v2022.1 Nov 8, 2021
@matsduf matsduf modified the milestones: v2022.1, v2022.2 May 4, 2022
@matsduf matsduf removed this from the v2022.2 milestone Nov 8, 2022
@matsduf matsduf added this to the v2023.1 milestone Nov 8, 2022
@matsduf matsduf modified the milestones: v2023.1, v2023.2 May 2, 2023
@matsduf
Copy link
Contributor

matsduf commented Dec 4, 2023

The ideas are planned to be included in #359.

@matsduf matsduf modified the milestones: v2023.2, v2024.1 Dec 4, 2023
@mattias-p
Copy link
Member

This PR is superseded by #359.

@mattias-p mattias-p closed this Jun 17, 2024
@mattias-p
Copy link
Member

Thank you @lowinger42 for raising this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Feature Type: New feature in software or test case description
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants