zonemaster / zonemaster-engine

The Zonemaster Engine - part of the Zonemaster project
Other
34 stars 33 forks source link

Fix polymorphism in Translator #1346

Closed marc-vanderwal closed 4 months ago

marc-vanderwal commented 4 months ago

Purpose

This PR fixes some regressions, presumably introduced by #1319, that could not be caught in Zonemaster::Engine’s unit tests.

Context

Problems

In Zonemaster::Backend, there is a Zonemaster::Backend::Translator class which inherits from Zonemaster::Engine::Translator. This is done in order to override the translate_tag method with a custom version.

When “un-Moosing” Zonemaster::Engine, however, changes were introduced that were harmless for Zonemaster::Engine, but not for Zonemaster::Backend. This caused two issues to emerge:

Cause

The root cause of the first problem is that when calling Zonemaster::Backend::Translator->new(), the newly-created instance was always blessed as Zonemaster::Engine::Translator (the superclass), not as the inheriting class. Thus, translate_tag() method calls always called the method in the superclass instead of the inheriting class.

As for the second problem, this is due to Backend’s Translator overriding another method, _build_all_tag_descriptions, from Engine’s Translator, and the initialization code in Zonemaster::Engine::Translator failing to call the overridden _build_all_tag_description when invoked as the Backend’s Translator class.

Changes

How to test this PR

  1. Make sure https://github.com/zonemaster/zonemaster-backend/pull/1166 is merged first.

  2. Run a test with zmb and grab the test ID (or have a test ID of a previous test handy and skip the following command):

    TEST_ID=`zmb start_domain_test --domain localhost | jq -r .result`
  3. Then, run the following after the test is done:

    zmb get_test_results --lang fr --test-id $TEST_ID | jq -e .result.params.domain

    This command should print "localhost" (or whatever the domain under test was) and set an exit status of 0. Any other result, or null, is abnormal.

  4. Using zmtest, run a test on a domain name that causes Zonemaster to time out, such as pool.ntp.org. The error message stating that the test took too long to complete should be translated (i.e., the raw untranslated message tag must not appear).

matsduf commented 4 months ago

In the "how to test" it should be jq -r .result not jq -r result, at least with the jq I have.

marc-vanderwal commented 4 months ago

In the "how to test" it should be jq -r .result not jq -r result, at least with the jq I have.

That’s because it was a typo. I’ve fixed the procedure.

mattias-p commented 3 months ago

Release testing for v2024.1

I've tested this according to the testing instruction above with successful results.