Opened 8 years ago

Closed 8 years ago

#1213 closed task (complete)

Implement IXFR system tests

Reported by: stephen Owned by: stephen
Priority: high Milestone: Sprint-20111025
Component: xfrin Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 7 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This depends on #1210.

It may be possible to re-use the BIND 9 system tests for IXFR.

Subtickets

Change History (20)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 7

comment:2 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20110927 to Sprint-20111011

comment:3 Changed 8 years ago by jelte

  • Priority changed from major to critical

comment:4 Changed 8 years ago by stephen

  • Owner set to stephen
  • Status changed from new to assigned

comment:5 Changed 8 years ago by stephen

  • Owner changed from stephen to UnAssigned
  • Status changed from assigned to reviewing

To avoid having to merge this branch with a current development branch, the work to date contains just the framework and data files for the tests. The final details (and checking of the log files) have been deferred until the IXFR branch has been merged.

Still to be done then is:
a) Add a test to check the order of record addition/deletion (as in this comment on the test specification).
b) Add the BIND 10 configuration for IXFR and checks on the log files. (Awaiting completion of IXFR work to determine exact details.)

However, the work has got to a point where it can be usefully reviewed.

The scripts can be run individually as specified in tests/system/README although, as expected, each reports a failure.

comment:6 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 follow-ups: Changed 8 years ago by vorner

  • Owner changed from vorner to stephen

Hello

I've read the tests (I didn't run them yet, as at makes little sense anyway).

Anyway, first point ‒ did we decide we will use this shell way? Because it's still hard to read and follow (it's not your fault, it's fundamental problem of the framework). I think we talked about some cucumbers or something. Or are we using the shell just for now, until we start using the cucumber?

There are bunch of really long zones. Could they maybe get generated (at last the long middle part) somehow instead of storing (and distributing) the boring part four times?

Why do the zones have a .db suffix? Can't that be confused with our config files, which are also .db? Other test zone usually have .zone.

Some files don't have updated years in their legal disclaimers. Does it matter?

This code looks long:

for i in 1 2 3 4 5 6 7 8 9 10 11 12

Why aren't you using seq?

Also, there's a lot of error-checking. Wouldn't set -e help at some places?

The fourth test has IXFR disabled in configuration, but the description says it should check the client performs IXFR. This might be slightly confusing, maybe say it should try IXFR and fallback to AXFR?

You check only the SOA of zones transfered. Should the other data be also checked?

A lot of the named.conf and similar files are the same for multiple tests. Is there a chance to share them instead of keeping multiple copies? Git handles symlinks…

Some strange things in comments:

  • „before going anything else“
  • „from and sens IXFR“
  • „so ther client“

comment:8 in reply to: ↑ 7 Changed 8 years ago by jreed

This code looks long:

for i in 1 2 3 4 5 6 7 8 9 10 11 12

Why aren't you using seq?

seq is not portable. Some of our platforms do not have seq.

comment:9 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by jreed

Some files don't have updated years in their legal disclaimers. Does it matter?

http://git.bind10.isc.org/~tester/OLD_COPYRIGHTS

We should consider updating the years.

comment:10 in reply to: ↑ 9 Changed 8 years ago by jreed

Replying to jreed:

Some files don't have updated years in their legal disclaimers. Does it matter?

http://git.bind10.isc.org/~tester/OLD_COPYRIGHTS

We should consider updating the years.

I responded without checking which files. Maybe some of this code was not changed from original BIND 9, so old dates are fine.

comment:11 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to vorner

Anyway, first point ‒ did we decide we will use this shell way? Because it's still hard to read and follow (it's not your fault, it's fundamental problem of the framework). I think we talked about some cucumbers or something. Or are we using the shell just for now, until we start using the cucumber?

Cucumber is a long-term aim. We have not yet decided on the test framework, so the tests have been added to the current framework which is written in Bourne shell.

There are bunch of really long zones. Could they maybe get generated (at last the long middle part) somehow instead of storing (and distributing) the boring part four times?

b10-loadzone currently doesn't support the $GENERATE directive. However, I have updated the files so that the "boring part" :-) is included via a $INCLUDE directive.

Why do the zones have a .db suffix? Can't that be confused with our config files, which are also .db? Other test zone usually have .zone.

I followed the naming convention for zone files in the other subdirectories of test/system, which use the .db suffix. However, you are right, so I've renamed the files to follow the convention used in Cricket Liu's "DNS and BIND" book, which use "db.<zone-name>".

Some files don't have updated years in their legal disclaimers. Does it matter?

Altered. As most of these are new files, I've replaced the header.

This code looks long:
for i in 1 2 3 4 5 6 7 8 9 10 11 12
Why aren't you using seq?

... because I'm rusty at shell programming. Changed.

Also, there's a lot of error-checking. Wouldn't set -e help at some places?

Yes it would. Done.

The fourth test has IXFR disabled in configuration, but the description says it should check the client performs IXFR. This might be slightly confusing, maybe say it should try IXFR and fallback to AXFR?

The documentation there was confusing there and has been corrected. The IXFR server has IXFRs enabled; the test is that the client sends an IXFR request when its SOA refresh time is reached.

You check only the SOA of zones transfered. Should the other data be also checked?

I was assuming that if the SOA was updated the zone had been transferred. On reflection though, there is no other system test that checks the transfer, so I've updated the tests to perform that check.

A lot of the named.conf and similar files are the same for multiple tests. Is there a chance to share them instead of keeping multiple copies? Git handles symlinks…

Good point. (I never knew that git could handle symlinks...)

Many of the .conf files are copied during the tests, I've put them all in the main ixfr directory and they are copied from there. Doing that has meant that all the clean.sh files are now identical, so I've done as you've suggested and added symlinks.

Some strange things in comments:

Corrected.

Other Things
As the current implementation of IXFR:

  • Does not support IXFR-out
  • Does not support sending an IXFR request when the SOA refresh expires
  • Does not support IXFR over UDP

... I would like to change the title of this ticket to "Implement IXFR System Tests (part)" and open tickets for:

  • IXFR-out tests
  • IXFR-in test for SOA refresh expiry
  • IXFR-in test for UDP
  • IXFR-in tests requiring special programs to be written to handle the sending of packets to test the add/deletion order of RRs and to test the handling of interrupted IXFR sessions (see #1210 for details).

In other words, close the ticket with what we have now and open tickets for the remaining work (in the knowledge that some of those tickets will be deferred until the features they test are scheduled to be implemented).

comment:12 in reply to: ↑ 11 Changed 8 years ago by vorner

  • Owner changed from vorner to stephen

Hello

Replying to stephen:

There are bunch of really long zones. Could they maybe get generated (at last the long middle part) somehow instead of storing (and distributing) the boring part four times?

b10-loadzone currently doesn't support the $GENERATE directive. However, I have updated the files so that the "boring part" :-) is included via a $INCLUDE directive.

Hmm, I didn't really know there was a $GENERATE directive, I meant a shell script that would generate it before the test. But this way is OK too I think, it both removes the duplicates and shows the differences between the zone versions well.

This code looks long:
for i in 1 2 3 4 5 6 7 8 9 10 11 12
Why aren't you using seq?

... because I'm rusty at shell programming. Changed.

Hmm, I didn't know about the nonportability. Stupid old platforms :-(. Should we revert it then, according to Jeremy? I wanted to propose grep -q, but it seems it's not portable as well. And I guess this is bashism?:

if ! grep ... | grep ; then
  :
fi

You check only the SOA of zones transfered. Should the other data be also checked?

I was assuming that if the SOA was updated the zone had been transferred. On reflection though, there is no other system test that checks the transfer, so I've updated the tests to perform that check.

Nice trick, I was wondering how complicated that might be and it turns out quite simple :-).

A lot of the named.conf and similar files are the same for multiple tests. Is there a chance to share them instead of keeping multiple copies? Git handles symlinks…

Good point. (I never knew that git could handle symlinks...)

That's not so common knowledge, not many projects use it.

Other Things
As the current implementation of IXFR:

  • Does not support IXFR over UDP

Really? Hmm, surprise.

  • IXFR-in tests requiring special programs to be written to handle the sending of packets to test the add/deletion order of RRs and to test the handling of interrupted IXFR sessions (see #1210 for details).

I guess requiring socat is too much, right? Then we wouldn't need the special programs.

Anyway, such programs would be quite simple anyway I guess.

In other words, close the ticket with what we have now and open tickets for the remaining work (in the knowledge that some of those tickets will be deferred until the features they test are scheduled to be implemented).

I'm not against postponing the rest of tests until we implement the features, that makes sense (and splitting the task as well, it's getting big).

But, few points:

  • I noticed this line:
    return 1t
    
    Is it a typo or some shell trick I don't know?
  • First line of tests/system/ixfr/in-2/setup.sh.in is empty. Couldn't it make trouble if the #!/bin/sh isn't at the very top?
  • The README files are indeed a way to make sure git keeps the directories. But AFAIK having an empty .keep file in the directory is more common way to do this. Do you think it is better to have README with explanation, or should we use the common way?
  • Would you merge master into this, so we can check if the test actually passes?

Thank you

comment:13 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111011 to Sprint-20111025

comment:14 Changed 8 years ago by stephen

  • Owner changed from stephen to vorner

Hmm, I didn't know about the nonportability. Stupid old platforms :-(. Should we revert it then, according to Jeremy?

I've reverted it to be on the safe side.

And I guess this is bashism?:

I'm not sure, but the way that it has been done (do the pipeline then test the result) is certainly safe.

I guess requiring socat is too much, right? Then we wouldn't need the special programs.

socat might handle the first, but then we are faced with the problem that it is another package to install. A program to listen on a socket and send back a known bytestream would not be too complicated to write.

return 1t
Is it a typo or some shell trick I don't know?

The former - corrected.

First line of tests/system/ixfr/in-2/setup.sh.in is empty. Couldn't it make trouble if the #!/bin/sh isn't at the very top?

Typo - corrected.

The README files are indeed a way to make sure git keeps the directories. But AFAIK having an empty .keep file in the directory is more common way to do this. Do you think it is better to have README with explanation, or should we use the common way?

I would leave the README. .keep is invisible in a normal "ls" and at least the README gives some explanation as to what is happening.

Would you merge master into this, so we can check if the test actually passes?

Commit f2b5473fc2f2dfa13485fe9822e84fadd69ac950 contains the changes made as a result of the review above (plus a couple of others in preparation for the merge).

Commit 044381e03b7f178c7c322861960b79c8a27bb4b1 includes the merge of master into it.

Last edited 8 years ago by stephen (previous) (diff)

comment:15 Changed 8 years ago by vorner

  • Owner changed from vorner to stephen

Hello

The new changes with grep make sense. I run the test and it complained it can't return from a script, only from functions, so I changed it to exit and it passed the tests. I pushed the change.

Would you remember if you used return in any other places that were not functions? If there are no more, I think it could be merged to master.

Thanks

comment:16 Changed 8 years ago by stephen

  • Owner changed from stephen to vorner

Will do.

This probably needs a ChangeLog entry. How about:

xxx.	[func]		stephen
	Add system test for IXFR over TCP.
	(Trac #1213, git xxx...)

comment:17 Changed 8 years ago by vorner

  • Owner changed from vorner to stephen

Hello.

Yes, that looks OK. I'm not sure if it's func, but I don't think there's anything better anyway.

comment:18 Changed 8 years ago by stephen

  • Resolution set to worksforme
  • Status changed from reviewing to closed

comment:19 Changed 8 years ago by stephen

  • Resolution worksforme deleted
  • Status changed from closed to reopened

comment:20 Changed 8 years ago by stephen

  • Resolution set to complete
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.