Ensure Backend’s messages stay translated #1166

marc-vanderwal · 2024-05-15T10:04:49Z

Purpose

This PR adds unit tests for Zonemaster::Backend::Translator and addresses an oversight in Backend caused by storing the module’s names as mixed case instead of uppercase.

Context

Submitting PR zonemaster/zonemaster-engine#1346 made it clear that Zonemaster::Backend::Translator lacked appropriate unit tests. When adding them, I found more bugs, now addressed in the same PR. I also noticed that all Backend-generated messages suddenly became untranslated.

Changes

Add unit tests;
Make sure future backend errors are stored as module Backend_Test_Agent;
Introduce a DB migration script to fix existing entries that were missed between Backend version 11.0 (Zonemaster release 2023.2) and the next release.

How to test this PR

Prerequisite: PR zonemaster/zonemaster-engine#1346 must be merged beforehand in Zonemaster-Engine. Alternatively, install a local copy Zonemaster-Engine from a local development branch with this PR merged. If not, one of the newly-added unit tests will fail.

Before merging this PR locally: run a test on a domain name that causes Zonemaster to time out, such as pool.ntp.org, using zmtest. Note the test ID. Wait for the test to time out. Notice that the UNABLE_TO_FINISH_TEST message is untranslated (i.e., only the message tag is visible).

After merging this PR locally: run the unit tests. Ideally, make sure that the tests in t/translator.t have been executed.

Run the database migration script. Then use zmb get_test_results --test-id $TEST_ID where $TEST_ID is the test ID of pool.ntp.org that was run before merging the PR locally. Expect the UNABLE_TO_FINISH_TEST message to be translated.

Run the same test again on pool.ntp.org using zmtest; wait for it to time out. Expect the UNABLE_TO_FINISH_TEST message to be translated as well.

mattias-p · 2024-05-31T08:48:10Z

t/translator.t

+BEGIN { use_ok 'Zonemaster::Backend::Translator'; }
+
+isa_ok 'Zonemaster::Backend::Translator', 'Zonemaster::Engine::Translator';


Wouldn't it be better to move the use_ok and isa_ok checks to 00-load.t and simply use Zonemaster::Backend::Translator here?

I disagree. It’s customary to have a BEGIN { use_ok 'SomeModule' } in a unit test for a module. And the isa_ok assertion ensures that Zonemaster::Backend::Translator is a subclass of Zonemaster::Engine::Translator. These are all properties about the Zonemaster::Backend::Translator module I want to test, so I think it makes sense to keep them here.

Also, it isn’t clear what t/00-load.t is supposed to do. It seems like it is some kind of basic smoke test. It’s probably better left as it is.

What I don't like about use_ok is that it doesn't abort on failure, which IMHO is the reasonable thing to do. Instead it continues executing until it inevitably crashes and prints additional error output for no good reason. I admit this nothing more than a nuisance.

I understand t/00-test.t to be a simple smoke test that just verifies that all module can be loaded without error. If you have good test coverage this is redundant in a sense, but it's also fast and low effort to maintain. And this can be valuable if the rest of the test suite is slow.

With that I rest my case.

t/translator.t

The unit tests suite of Zonemaster::Backend did not test the Translator module directly. Despite Devel::Cover already reporting good coverage of Zonemaster::Backend::Translator, no unit test really looked at the operation or the output of that module’s methods. This causes subtle regressions in Zonemaster::Engine::Translator, for example, to slip by unnoticed. When running the new t/translator.t script, a handful of tests will likely fail if the corresponding fixes for Zonemaster::Engine::Translator aren’t merged yet.

The recent case alteration of module names has caused messages whose module attribute is BACKEND_TEST_AGENT to be stored as Backend_Test_Agent instead. The database migration script for version 11.0 took care of that, but the rest of the code had not been adjusted accordingly. This meant that the Backend-specific TEST_DIED or UNABLE_TO_FINISH_TEST tags were no longer translated appropriately. In the meantime, new tests run on the backend will still store BACKEND_TEST_AGENT instead of Backend_Test_Agent. To fix this inconsistency, a change needs to be done to Zonemaster::Backend::DB and a migration script needs to be written to fix the entries generated with the backend between version 11.0 and the next release.

The recent case alteration of module names has caused messages whose module attribute is BACKEND_TEST_AGENT to be stored as Backend_Test_Agent instead. However, new tests run on the backend still store the module name as BACKEND_TEST_AGENT instead of Backend_Test_Agent. This commit fixes the inconsistency by ensuring that new tests store the correct module name. Now, we still need a migration script to fix the entries generated by the backend between version 11.0 and the next release.

With the previous changes, “BACKEND_TEST_AGENT” should have been stored as “Backend_Test_Agent” in the database. This migration script ensures everything is consistent again.

The clean way of invoking a parent’s method in a class that overrides that method is to use the SUPER pseudo-class, instead of repeating the name of the parent class. Details are in the “perlobj” manual page.

For messages generated by the backend, store them as the “Backend” module instead of “Backend_Test_Agent”. The use of mixed case with underscores is unsatisfactory and the test agent is the only place in the backend that generates these messages. In essence, “Backend” could be a synonym for “System”, but it’s better to keep the Engine’s system messages separate from the Backend’s.

Pull request zonemaster-engine#1347 (zonemaster/zonemaster-engine#1347) has been merged, which fixes Zonemaster::Backend::Translator->instance(). So we can remove one use of the deprecated Zonemaster::Backend::Translator->new() in the unit tests.

matsduf · 2024-06-20T09:40:26Z

@marc-vanderwal, translator.t fails in Travis. See #1170.

MichaelTimbert · 2024-06-27T13:57:29Z

Tested and working on Ubuntu 20.04.6 and 22.04.4.

Bug:

on Ubuntu 20.04 the migration script in cat /usr/local/share/perl/5.30.0/auto/share/dist/Zonemaster-Backend/patch/patch_db_zonemaster_backend_ver_11.2.0.pl fail due to the line use List::MoreUtils qw(zip_unflatten);
Error:

Could not find sub 'zip_unflatten' exported by List::MoreUtils at /usr/local/share/perl/5.30.0/auto/share/dist/Zonemaster-Backend/patch/patch_db_zonemaster_backend_ver_11.2.0.pl line 4.
BEGIN failed--compilation aborted at /usr/local/share/perl/5.30.0/auto/share/dist/Zonemaster-Backend/patch/patch_db_zonemaster_backend_ver_11.2.0.pl line 4.

zip_unflatten is not present in List::MoreUtils version 0.416.

$ perl -MList::MoreUtils -le 'print $List::MoreUtils::VERSION'
0.416

Commenting the line make the migration script work.

Test Procedure:

Install Zonemaster with Backend on commit ca9ad8b3 (before the merge of this PR)
Change max_zonemaster_execution_time = 10 in /etc/zonemaster/backend_config.ini
restart Zonemaster-Backend sudo systemctl restart zm-*
run zmtest on pool.ntp.org and note the test ID.
result of this first test:

...
"results": [
      {
        "module": "BACKEND_TEST_AGENT",
        "testcase": "",
        "level": "CRITICAL",
        "message": "Le test a mis trop de temps à s’exécuter (la limite actuelle est de 10 secondes). Il y a peut-être trop de serveurs de noms,  ou les serveurs de noms sont injoignables ou trop peu réactifs.\n"
      }
...

Re-Install Zonemaster from develop (after the merge of this PR)
restart Zonemaster-Backend
run a new test, result must contain:

"results": [
      {
        "level": "CRITICAL",
        "message": "The test took too long to run (the current limit is 10 seconds). Maybe there are too many name servers or the name servers are either unreachable or not responsive enough.\n",
        "module": "Backend",
        "testcase": ""
      }

get result of the first test zmb get_test_results --test-id <IDTEST> --lang en|jq. Message is not translated.

    "results": [
      {
        "module": "BACKEND_TEST_AGENT",
        "message": "BACKEND_TEST_AGENT:UNABLE_TO_FINISH_TEST max_execution_time=10\n",
        "testcase": "",
        "level": "CRITICAL"
      }

run migration script /usr/local/share/perl/5.30.0/auto/share/dist/Zonemaster-Backend/patch/patch_db_zonemaster_backend_ver_11.2.0.pl
get result of the first test zmb get_test_results --test-id <IDTEST> --lang fr|jq. Message is now translated.

    "results": [
      {
        "module": "Backend",
        "testcase": "",
        "level": "CRITICAL",
        "message": "Le test a mis trop de temps à s’exécuter (la limite actuelle est de 10 secondes). Il y a peut-être trop de serveurs de noms,  ou les serveurs de noms sont injoignables ou trop peu réactifs.\n"
      }
    ],

marc-vanderwal · 2024-06-27T14:14:22Z

Oh, I see. We don’t need List::MoreUtils in this script, so that’s something that should be fixed before the release.

matsduf · 2024-06-30T15:29:32Z

Changes tagging of this from V-Patch to V-Minor to motivate the minor version upgrade.

marc-vanderwal requested review from mattias-p, matsduf and tgreenx May 15, 2024 10:04

marc-vanderwal added the T-Bug Type: Bug in software or error in test case description label May 15, 2024

marc-vanderwal added this to the v2024.1 milestone May 15, 2024

marc-vanderwal mentioned this pull request May 15, 2024

Fix polymorphism in Translator zonemaster/zonemaster-engine#1346

Merged

marc-vanderwal changed the title ~~Ensure messages generated by the test agent are translated again~~ Ensure Backend’s messages stay translated May 15, 2024

marc-vanderwal force-pushed the bugfix/latent-translator-problems branch from 0a5799e to 5fe2a89 Compare May 15, 2024 14:18

marc-vanderwal mentioned this pull request May 16, 2024

Fix Zonemaster::Engine::Translator’s instance() method zonemaster/zonemaster-engine#1347

Merged

marc-vanderwal marked this pull request as ready for review May 16, 2024 07:41

matsduf added the P-High Priority: Issue to be solved before other label May 22, 2024

mattias-p previously approved these changes Jun 3, 2024

View reviewed changes

matsduf previously approved these changes Jun 3, 2024

View reviewed changes

marc-vanderwal added 6 commits June 3, 2024 14:45

Fix module name BACKEND_TEST_AGENT in database

0c722ea

With the previous changes, “BACKEND_TEST_AGENT” should have been stored as “Backend_Test_Agent” in the database. This migration script ensures everything is consistent again.

Translator: don’t use our parent’s name in vain

35b12e7

The clean way of invoking a parent’s method in a class that overrides that method is to use the SUPER pseudo-class, instead of repeating the name of the parent class. Details are in the “perlobj” manual page.

marc-vanderwal dismissed stale reviews from matsduf and mattias-p via 2e0e172 June 3, 2024 12:46

marc-vanderwal force-pushed the bugfix/latent-translator-problems branch from 7ab6203 to 2e0e172 Compare June 3, 2024 12:46

marc-vanderwal requested a review from mattias-p June 3, 2024 12:49

mattias-p approved these changes Jun 3, 2024

View reviewed changes

matsduf approved these changes Jun 3, 2024

View reviewed changes

marc-vanderwal merged commit 38d8c57 into zonemaster:develop Jun 5, 2024

marc-vanderwal added the V-Patch Versioning: The change gives an update of patch in version. label Jun 13, 2024

tgreenx mentioned this pull request Jun 13, 2024

Add missing documentation for v2024.1 release zonemaster/zonemaster#1272

Merged

MichaelTimbert added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jun 27, 2024

marc-vanderwal mentioned this pull request Jun 27, 2024

Clean up “use” statements in database migration script for upcoming 11.2.0 version #1179

Merged

matsduf added V-Minor Versioning: The change gives an update of minor in version. and removed V-Patch Versioning: The change gives an update of patch in version. labels Jun 30, 2024

marc-vanderwal mentioned this pull request Jul 1, 2024

Preparation for release v2024.1 (Backend) #1180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure Backend’s messages stay translated #1166

Ensure Backend’s messages stay translated #1166

marc-vanderwal commented May 15, 2024 •

edited

Loading

mattias-p May 31, 2024

marc-vanderwal Jun 3, 2024

mattias-p Jun 3, 2024 •

edited

Loading

matsduf commented Jun 20, 2024

MichaelTimbert commented Jun 27, 2024

marc-vanderwal commented Jun 27, 2024

matsduf commented Jun 30, 2024

		BEGIN { use_ok 'Zonemaster::Backend::Translator'; }

		isa_ok 'Zonemaster::Backend::Translator', 'Zonemaster::Engine::Translator';

Ensure Backend’s messages stay translated #1166

Ensure Backend’s messages stay translated #1166

Conversation

marc-vanderwal commented May 15, 2024 • edited Loading

Purpose

Context

Changes

How to test this PR

mattias-p May 31, 2024

Choose a reason for hiding this comment

marc-vanderwal Jun 3, 2024

Choose a reason for hiding this comment

mattias-p Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

matsduf commented Jun 20, 2024

MichaelTimbert commented Jun 27, 2024

Bug:

Test Procedure:

marc-vanderwal commented Jun 27, 2024

matsduf commented Jun 30, 2024

marc-vanderwal commented May 15, 2024 •

edited

Loading

mattias-p Jun 3, 2024 •

edited

Loading