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

Fixes broken unit tests #1192

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Conversation

matsduf
Copy link
Contributor

@matsduf matsduf commented Nov 30, 2024

Purpose

Unit test file t/test01.t leaks queries when network is set to off. A change in .fr NS and DS triggered the tests to break. This PR:

  • Updates t/test01.t with correct expectations on .fr.
  • Updates t/test01.t so that it does not leak queries.
  • Updates t/test01.data to match updates on .fr and updated t file.
  • Updates t/idn.t so that it does not leak queries.

Context

Issue #1191

How to test this PR

  1. Unit tests must pass.
  2. Review.
  3. Verify that no queries are sent when network is set to off.

@matsduf matsduf added T-Bug Type: Bug in software or error in test case description P-High Priority: Issue to be solved before other labels Nov 30, 2024
@matsduf matsduf added this to the v2024.2 milestone Nov 30, 2024
@matsduf matsduf linked an issue Nov 30, 2024 that may be closed by this pull request
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.

Looks fine, except for one teeny tiny little thing.

t/test01.t Outdated Show resolved Hide resolved
Co-authored-by: Marc van der Wal <[email protected]>
Copy link
Contributor

@tgreenx tgreenx left a comment

Choose a reason for hiding this comment

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

After investigating, I would suggest an alternative fix:

Firstly, the need for making new queries is due to zonemaster/zonemaster-engine#1397, which requires all DNSSEC queries to be re-recorded. I forgot to do it here, sorry. The unit tests do call Zonemaster::Backend::RPCAPI::get_data_from_parent_zone() which does indeed make a DNSSEC query.

While creating and using distinct profiles can also solve the issue, I think the issue can be better addressed in Zonemaster::Backend::Config. When loading profiles, currently Backend will start with an entirely new profile and disregard any values from the effective profile. So a simple one-line fix would be:

$ git diff
diff --git a/lib/Zonemaster/Backend/Config.pm b/lib/Zonemaster/Backend/Config.pm
index 78eba7b..6b59c62 100644
--- a/lib/Zonemaster/Backend/Config.pm
+++ b/lib/Zonemaster/Backend/Config.pm
@@ -93,7 +93,7 @@ sub load_profiles {
     foreach my $name ( keys %profile_paths ) {
         my $path = $profile_paths{$name};

-        my $full_profile = Zonemaster::Engine::Profile->default;
+        my $full_profile = Zonemaster::Engine::Profile->effective;
         if ( defined $path ) {
             my $json = eval { read_file( $path, err_mode => 'croak' ) }    #
               // die "Error loading profile '$name': $@";

Regarding the unexpected making of new queries without ZONEMASTER_RECORD=1, this can be explained by the following: in the t/test01.t unit test file, method TestUtil::restore_datafile() is called and sets profile parameter no_network to 1 so it should prevent the sending of new queries. But this is rendered useless as Zonemaster::Backend::TestAgent::run() is called afterwards, which will override the settings in the effective profile with a profile that has been instantiated with default values instead of effective values.

@matsduf
Copy link
Contributor Author

matsduf commented Dec 3, 2024

After investigating, I would suggest an alternative fix:

I suggest that you create a new PR instead.

@tgreenx
Copy link
Contributor

tgreenx commented Dec 3, 2024

After investigating, I would suggest an alternative fix:

I suggest that you create a new PR instead.

Done, see #1193.

Copy link
Contributor

@tgreenx tgreenx left a comment

Choose a reason for hiding this comment

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

Temporary fix. Will be replaced by #1193 in another release.

@matsduf matsduf merged commit c29b3bd into zonemaster:develop Dec 4, 2024
1 check passed
@matsduf matsduf deleted the fix-broken-unit-tests branch December 4, 2024 09:19
@mattias-p
Copy link
Member

It's a bit awkward, but it looks right to me. Not saying it's your fault that it's awkward.

@matsduf matsduf added the V-Patch Versioning: The change gives an update of patch in version. label Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-High Priority: Issue to be solved before other T-Bug Type: Bug in software or error in test case description V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit test fails in develop branch
4 participants