Logger refactoring by hannaeko · Pull Request #1302 · zonemaster/zonemaster-engine

hannaeko · 2023-11-13T13:21:25Z

Purpose

This PR aims to provide an interface to manually set the module and test case name in the Logger.

This is to avoid unnecessary calls to callers and decouple the module / test case name from the file name / method name.

This PR also addresses the letter case of test cases and module to fit the updated specification.

Context

zonemaster/zonemaster#1200

Changes

In test modules, the local method _emit_log is used instead of info
A somewhat global variable $Zonemaster::Engine::Logger::TEST_CASE_NAME is used to set the test case name
Test case and module name have now the letter case defined in change letter case for test case identifier zonemaster#1200 (including Syestem and Unspecified).
Avoid building the caller trace and iterating through it for each log entry this may marginally improve performance.

How to test this PR

Run a test using the CLI,

zonemaster-cli --level info --show-module --show-testcase afnic.fr

check that

the module and test case are no longer full uppercase
the log messages are correctly displayed and translated

hannaeko · 2023-11-13T15:41:58Z

Some tests are failing due to circular dependency, I need to rethink some stuff around the logger. Changing this PR to draft for the time being.

hannaeko · 2023-11-13T17:05:39Z

The circular dependency has been fixed, I still need to fix the unit tests but this should not impact anything else than the unit tests themselves.

tgreenx

Tested and it works as advertised. Note that even with this PR profile options (such as test_levels or logfilter) are still expected to use capitalized Test module names.

I have one suggestion: since module names in share/modules.txt are already in the format suggested by zonemaster/zonemaster#1200, for simplicity purposes we could already remove the forced upper-case module names in Engine where applicable. See the proposed change below to Zonemaster::Engine::Translator:

$ git log -1 --oneline
b96d62ea (HEAD -> test-PR1302) fix log filter

$ git diff
diff --git a/lib/Zonemaster/Engine/Translator.pm b/lib/Zonemaster/Engine/Translator.pm
index 42fef57f..23bdc726 100644
--- a/lib/Zonemaster/Engine/Translator.pm
+++ b/lib/Zonemaster/Engine/Translator.pm
@@ -174,10 +174,10 @@ sub _load_data {
 sub _build_all_tag_descriptions {
     my %all_tag_descriptions;

-    $all_tag_descriptions{SYSTEM} = \%TAG_DESCRIPTIONS;
+    $all_tag_descriptions{System} = \%TAG_DESCRIPTIONS;
     foreach my $mod ( 'Basic', Zonemaster::Engine->modules ) {
         my $module = 'Zonemaster::Engine::Test::' . $mod;
-        $all_tag_descriptions{ uc( $mod ) } = $module->tag_descriptions;
+        $all_tag_descriptions{ $mod } = $module->tag_descriptions;
     }

     return \%all_tag_descriptions;
@@ -231,7 +231,7 @@ sub to_string {
 sub translate_tag {
     my ( $self, $entry ) = @_;

-    return $self->_translate_tag( uc $entry->module, $entry->tag, $entry->printable_args ) // $entry->string;
+    return $self->_translate_tag( $entry->module, $entry->tag, $entry->printable_args ) // $entry->string;
 }

marc-vanderwal · 2023-11-23T08:45:05Z

So far, so good. I wanted to see if there was any improvement in performance but it seems that the time to test a zone has too high of a variance to allow me to arrive at a conclusive result.

marc-vanderwal · 2023-11-23T08:53:21Z

However, when I run the unit tests for the test modules (i.e. prove -rl t/Test-*.t), I notice they run much faster. On the current develop branch, I timed it at 4:13 minutes, and with your branch checked out, it drops to about 2:43 minutes. Nice!

hannaeko · 2023-11-23T14:11:58Z

However, when I run the unit tests for the test modules (i.e. prove -rl t/Test-*.t), I notice they run much faster. On the current develop branch, I timed it at 4:13 minutes, and with your branch checked out, it drops to about 2:43 minutes. Nice!

Thanks for the measurements that's really good to know!
EDIT : Although I wonder how much was because of the crashing tests ^^

hannaeko · 2023-11-23T15:46:20Z

Please re-review, the tests are now passing.

hannaeko · 2023-11-23T16:14:53Z

However, when I run the unit tests for the test modules (i.e. prove -rl t/Test-*.t), I notice they run much faster. On the current develop branch, I timed it at 4:13 minutes, and with your branch checked out, it drops to about 2:43 minutes. Nice!

I redid some measurements now that the tests are passing, the gain is only marginal.
Before : Files=79, Tests=1711, 92 wallclock secs ( 0.57 usr 0.20 sys + 87.43 cusr 4.56 csys = 92.76 CPU)
After: Files=79, Tests=1711, 86 wallclock secs ( 0.54 usr 0.21 sys + 80.24 cusr 4.53 csys = 85.52 CPU)

matsduf · 2023-11-24T16:54:40Z

Purpose

Provide interface to manually set the module and test case name.

Is this a correct description of this PR?

mattias-p

Super nice to get rid of the call stack walking! I added comments for a few minor things but all in all I like it a lot.

matsduf

Please provide a better text under "Purpose" so that you easily can see what this PR is about.

hannaeko · 2023-11-27T13:19:00Z

Please provide a better text under "Purpose" so that you easily can see what this PR is about.

I tried to improve it.

@mattias-p please can you re-review?

matsduf · 2023-11-27T15:20:20Z

Please provide a better text under "Purpose" so that you easily can see what this PR is about.

I tried to improve it.

Is the new presentation of the module and test case names in mostly lower case part of the purpose? Or is that just a side effect?

hannaeko · 2023-11-27T16:46:51Z

Is the new presentation of the module and test case names in mostly lower case part of the purpose? Or is that just a side effect?

It was the main motivation but has become a side effect, I updated the description to mention it in the purpose section.

mattias-p

I really like this.

ghost · 2024-01-10T15:00:20Z

v2023.2 release testing

Tested based on the "How to test" section and woks as expected.

Set the test case name using dynamic scoping instead of trace

a54ccf8

hannaeko requested review from a user, marc-vanderwal, matsduf, mattias-p and tgreenx November 13, 2023 13:21

hannaeko mentioned this pull request Nov 13, 2023

Fix unspecified zonemaster/zonemaster-gui#444

Merged

hannaeko marked this pull request as draft November 13, 2023 15:42

use a specialized method for per module logging

2c53cfd

hannaeko force-pushed the logger-refacto branch from 5fa3b61 to 2c53cfd Compare November 13, 2023 16:31

fix log filter

b96d62e

hannaeko marked this pull request as ready for review November 13, 2023 17:05

tgreenx reviewed Nov 21, 2023

View reviewed changes

Comment thread lib/Zonemaster/Engine/Logger.pm Outdated

tgreenx added this to the v2023.2 milestone Nov 21, 2023

tgreenx added the V-Major Versioning: The change gives an update of major in version. label Nov 21, 2023

tgreenx reviewed Nov 21, 2023

View reviewed changes

Comment thread lib/Zonemaster/Engine/Util.pm Outdated

hannaeko added 7 commits November 23, 2023 15:15

avoid string conversion to uppercase

cbaca86

fix comments

d8a7ad4

make emit_log private

c94f61d

fix translator test

9d551b7

fix reference to trace in unit tests

f35b385

Merge branch 'develop' into logger-refacto

5ca0d00

add new method argstr to be used in CLI

0815643

hannaeko mentioned this pull request Nov 23, 2023

Fix raw ouput zonemaster/zonemaster-cli#360

Merged

tgreenx previously approved these changes Nov 23, 2023

View reviewed changes

mattias-p previously approved these changes Nov 24, 2023

View reviewed changes

Comment thread lib/Zonemaster/Engine/Logger.pm Outdated

Comment thread lib/Zonemaster/Engine/Test/Address.pm Outdated

Comment thread t/Test-connectivity.t Outdated

Comment thread t/Test-connectivity.t Outdated

matsduf previously approved these changes Nov 27, 2023

View reviewed changes

make emit_log method more explicit

c78f34a

hannaeko dismissed stale reviews from matsduf, mattias-p, and tgreenx via c78f34a November 27, 2023 12:41

simplify unit tests

68dd34c

matsduf previously approved these changes Nov 27, 2023

View reviewed changes

mattias-p previously approved these changes Nov 28, 2023

View reviewed changes

tgreenx previously approved these changes Nov 28, 2023

View reviewed changes

Merge branch 'develop' into logger-refacto

b2861fd

hannaeko dismissed stale reviews from tgreenx, mattias-p, and matsduf via b2861fd November 28, 2023 17:18

tgreenx approved these changes Nov 28, 2023

View reviewed changes

hannaeko merged commit ba0f7e0 into zonemaster:develop Nov 28, 2023

This was referenced Nov 29, 2023

Provide implementation for Zone11 (SPF test) #1287

Merged

Document and update method '_emit_log()' in all Test modules #1310

Merged

matsduf mentioned this pull request Dec 11, 2023

PRs for v2023.2 zonemaster/zonemaster#1232

Closed

marc-vanderwal mentioned this pull request Dec 13, 2023

Cleanup in Syntax test module #1293

Merged

ghost added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jan 10, 2024

Uh oh!

Conversation

hannaeko commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Context

Changes

How to test this PR

Uh oh!

hannaeko commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hannaeko commented Nov 13, 2023

Uh oh!

tgreenx left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marc-vanderwal commented Nov 23, 2023

Uh oh!

marc-vanderwal commented Nov 23, 2023

Uh oh!

hannaeko commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hannaeko commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hannaeko commented Nov 23, 2023

Uh oh!

matsduf commented Nov 24, 2023

Purpose

Uh oh!

mattias-p left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matsduf left a comment

Choose a reason for hiding this comment

Uh oh!

hannaeko commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matsduf commented Nov 27, 2023

Uh oh!

hannaeko commented Nov 27, 2023

Uh oh!

mattias-p left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 10, 2024

v2023.2 release testing

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hannaeko commented Nov 13, 2023 •

edited

Loading

hannaeko commented Nov 13, 2023 •

edited

Loading

tgreenx left a comment •

edited

Loading

hannaeko commented Nov 23, 2023 •

edited

Loading

hannaeko commented Nov 23, 2023 •

edited

Loading

hannaeko commented Nov 27, 2023 •

edited

Loading