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

- Modify data for new engine version. #342

Merged
merged 3 commits into from Jan 3, 2018

Conversation

Projects
None yet
4 participants
@vlevigneron
Contributor

vlevigneron commented Dec 21, 2017

@vlevigneron vlevigneron requested review from mattias-p and matsduf Dec 21, 2017

@sandoche2k

This comment has been minimized.

Show comment
Hide comment
@sandoche2k

sandoche2k Jan 2, 2018

Contributor

@matsduf, @mattias-p to review

Contributor

sandoche2k commented Jan 2, 2018

@matsduf, @mattias-p to review

@sandoche2k

This comment has been minimized.

Show comment
Hide comment
@sandoche2k

sandoche2k Jan 2, 2018

Contributor

@matsduf got to verify which commit has created this issue

Contributor

sandoche2k commented Jan 2, 2018

@matsduf got to verify which commit has created this issue

@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Jan 2, 2018

Contributor

The information on the commit has been published on #337

Contributor

matsduf commented Jan 2, 2018

The information on the commit has been published on #337

@matsduf

I think it is important that we understand why a certain change in Engine created the problem.

@vlevigneron

This comment has been minimized.

Show comment
Hide comment
@vlevigneron

vlevigneron Jan 2, 2018

Contributor

@matsduf Yes we'll do our best to understand and a new issue should be raised but as we know how to "fix" it now if we generate a new data file, it should not prevent the merge of last code for the release.

Contributor

vlevigneron commented Jan 2, 2018

@matsduf Yes we'll do our best to understand and a new issue should be raised but as we know how to "fix" it now if we generate a new data file, it should not prevent the merge of last code for the release.

@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Jan 2, 2018

Contributor

@vlevigneron! If we do not understand why it happens we might introduce a bad bug in Zonemaster. The following line in t/test01.t emits the error message:

        Zonemaster::Backend::TestAgent->new( { db => "Zonemaster::Backend::DB::SQLite" } )->run( $test_id );
Contributor

matsduf commented Jan 2, 2018

@vlevigneron! If we do not understand why it happens we might introduce a bad bug in Zonemaster. The following line in t/test01.t emits the error message:

        Zonemaster::Backend::TestAgent->new( { db => "Zonemaster::Backend::DB::SQLite" } )->run( $test_id );
@vlevigneron

This comment has been minimized.

Show comment
Hide comment
@vlevigneron

vlevigneron Jan 3, 2018

Contributor

@matsduf I guess we have to modify data just because we have changed (and fixed) the way undelegated tests work now. Just try this zonemaster-cli afnic.fr --ns ns1.nic.fr --ns ns2.nic.fr/192.134.4.1 (which is what t/test01.t does) with the 2 environnements, you'll see that the results are differents, DNS queries also.

Contributor

vlevigneron commented Jan 3, 2018

@matsduf I guess we have to modify data just because we have changed (and fixed) the way undelegated tests work now. Just try this zonemaster-cli afnic.fr --ns ns1.nic.fr --ns ns2.nic.fr/192.134.4.1 (which is what t/test01.t does) with the 2 environnements, you'll see that the results are differents, DNS queries also.

@mattias-p

The #337 error message complains about not finding blessed in https://github.com/vlevigneron/zonemaster-backend.git. Please use Scalar::Util qw( blessed ) there.

@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Jan 3, 2018

Contributor

@vlevigneron! I have no problem with recreating the data as long as we understand why we should do it. It is not only that the test fails. Zonemaster crashes and spits out messages that indicate that something is incompatible.

If you run the test with just the 72 first lines in the file, then the data file seems to be incomplete for the test and you get the following error message (no crash):

t/test01.t .. 1/? External query for fr, NS attempted to a.root-servers.net/198.41.0.4 while running with no_network at /usr/local/share/perl/5.22.1/Zonemaster/Engine/Zone.pm line 206.

If you increase that to the first 73 lines you get the crash.

  1. Is the format of the data incorrect? We should have a test of the data to make sure the format is correct.
  2. Is the data incomplete? Zonemaster should not crash because of incomplete data.
Contributor

matsduf commented Jan 3, 2018

@vlevigneron! I have no problem with recreating the data as long as we understand why we should do it. It is not only that the test fails. Zonemaster crashes and spits out messages that indicate that something is incompatible.

If you run the test with just the 72 first lines in the file, then the data file seems to be incomplete for the test and you get the following error message (no crash):

t/test01.t .. 1/? External query for fr, NS attempted to a.root-servers.net/198.41.0.4 while running with no_network at /usr/local/share/perl/5.22.1/Zonemaster/Engine/Zone.pm line 206.

If you increase that to the first 73 lines you get the crash.

  1. Is the format of the data incorrect? We should have a test of the data to make sure the format is correct.
  2. Is the data incomplete? Zonemaster should not crash because of incomplete data.
@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Jan 3, 2018

Contributor

@mattias-p! Please update your comment! Where do you want to have use Scalar::Util qw(blessed)?

Contributor

matsduf commented Jan 3, 2018

@mattias-p! Please update your comment! Where do you want to have use Scalar::Util qw(blessed)?

@mattias-p

This comment has been minimized.

Show comment
Hide comment
@mattias-p

mattias-p Jan 3, 2018

Contributor

As @matsduf pointed out I messed up my review comment above. Sadly I can't find an edit button for the comment. This is what I meant to say:

The #337 error message complains about not finding blessed in lib/Zonemaster/Backend/TestAgent.pm. Please use Scalar::Util qw( blessed ) there.

Contributor

mattias-p commented Jan 3, 2018

As @matsduf pointed out I messed up my review comment above. Sadly I can't find an edit button for the comment. This is what I meant to say:

The #337 error message complains about not finding blessed in lib/Zonemaster/Backend/TestAgent.pm. Please use Scalar::Util qw( blessed ) there.

@vlevigneron

This comment has been minimized.

Show comment
Hide comment
@vlevigneron

vlevigneron Jan 3, 2018

Contributor

@matsduf We need to recreate the data because engine behaviour has changed for undelegated tests so current recorded data are not what the engine expects with this new behaviour.

Contributor

vlevigneron commented Jan 3, 2018

@matsduf We need to recreate the data because engine behaviour has changed for undelegated tests so current recorded data are not what the engine expects with this new behaviour.

@vlevigneron

This comment has been minimized.

Show comment
Hide comment
@vlevigneron

vlevigneron Jan 3, 2018

Contributor

@matsduf I also guess there is an issue in t/test01.t where data file is created twice instead of just once at the end of the script. So the current data file is broken, we need to re-create it.

Contributor

vlevigneron commented Jan 3, 2018

@matsduf I also guess there is an issue in t/test01.t where data file is created twice instead of just once at the end of the script. So the current data file is broken, we need to re-create it.

@vlevigneron

This comment has been minimized.

Show comment
Hide comment
@vlevigneron

vlevigneron Jan 3, 2018

Contributor

@mattias-p You are right, I'll add that in TestAgent.pm file.

Contributor

vlevigneron commented Jan 3, 2018

@mattias-p You are right, I'll add that in TestAgent.pm file.

@matsduf

This comment has been minimized.

Show comment
Hide comment
@matsduf

matsduf Jan 3, 2018

Contributor

Can you please update t/test01.t too?

Contributor

matsduf commented Jan 3, 2018

Can you please update t/test01.t too?

@vlevigneron

This comment has been minimized.

Show comment
Hide comment
@vlevigneron

vlevigneron Jan 3, 2018

Contributor

@matsduf I just did it.

Contributor

vlevigneron commented Jan 3, 2018

@matsduf I just did it.

@matsduf

matsduf approved these changes Jan 3, 2018

@vlevigneron vlevigneron merged commit efef50c into zonemaster:develop Jan 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment