Opened 9 years ago

Closed 9 years ago

#606 closed task (complete)

See which of BIND 9 tests can be re-used or re-implemented for BIND 10

Reported by: stephen Owned by: jinmei
Priority: medium Milestone: A-Team-Sprint-20110309
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description


Subtickets

Change History (16)

comment:1 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 3

comment:2 Changed 9 years ago by jinmei

  • Summary changed from Attempt to see which of BIND 9 tests can be re-used or re-implemented for BIND 10 to See which of BIND 9 tests can be re-used or re-implemented for BIND 10

comment:3 Changed 9 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:4 Changed 9 years ago by jinmei

branch trac606 is ready for review.

Based on my moderate level of look at BIND 9's system tests, I think
we can reuse (at least some part) the following test suites:

  • dnssec (some of it)
  • glue
  • masterfile (maybe some of it)
  • notify (this seems to be difficult though)
  • xfer (some of it)
  • resolver (probably some of it)

These tests can be found in bind9/bin/tests/system.

It also seemed to me that the main framework of the system test
(start/stop the servers, etc) can be easily reused.

So, in this branch, I did the following:

  • port the general framework
  • add some small modifications to BIND10 programs so that they can work under the framework
  • as an initial step of porting actual tests, port the "glue" test

The entire diff may look a bit large, but a large part of is a copy of
the original test code in BIND 9, which can generally be ignored.
These changes are: 53b5297 to ddd7b7c, and 5851a6c.

Adjustments to the existing framework are: 56df4f9 to edfcbca, and
19f9f1.

Other changes are modifications to BIND 10 programs. Important
features are:

  • a9211d7: added --pid-file to BoB so that we can stop it from the test framework
  • various changes to specify the location of config/temporary files so that the can be located under a test directory

For general usage of the test framework, see the README file under
tests/system.

My plan is to get this reviewed and merged, and then add other
possible tests on top it.

Finally, this is the proposed changelog entry (I thought it's a
visible feature, but I'm also okay with skipping it if it looks too
minor in terms of user experience):

  185.?	[func]		jinmei
	Imported system test framework of BIND 9.  It can be run by
	'make systest' at the top source directory.  Notes: currently it
	doesn't work when built in a separate tree.  It also requires
	perl, an inherited dependency from the original framework.
	Also, mainly for the purpose of tests, a new option "--pid-file"
	was added to BoB, with which the boss process will dump its PID
	to the specified file.
	(Trac #606, git TBD)

comment:5 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:6 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

I have few comments.

  • Why is the unlink in boss's dump pid needed? Wouldn't overwriting the file (eg. open(filename, 'w')) be enough?
  • The README contains note of make test, but the toplevel makefile has systest, not test.
  • When I run make distcheck, it fails for me:
    make[3]: Entering directory `/home/vorner/work/bind10/bind10-devel-20110224/_build/tests/system'
    rm -rf .libs _libs
    rm -f *.lo
    test -z "conf.sh" || rm -f conf.sh
    sh cleanall.sh
    test . = "../../../tests/system" || test -z "" || rm -f
    sh: cleanall.sh: No such file or directory
    make[3]: *** [distclean-local] Error 127
    make[3]: *** Waiting for unfinished jobs....
    make[3]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110224/_build/tests/system'
    make[2]: *** [distclean-recursive] Error 1
    make[2]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110224/_build/tests'
    make[1]: *** [distclean-recursive] Error 1
    make[1]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110224/_build'
    make: *** [distcheck] Error 1
    
  • The digcomp.pl seems like a bad perl code (not wrong, just not following usual conventions, like declaring variables with my, has a lot of duplicate code, using "none" instead of undef). I get this one is imported and the goal is not to cleanup all imported code, but I believe it would be worth a note to either port it to python sometime or clean this one up.
  • The last line of the test is kind of confusing. It starts with E: which probably means End, but I thought it was error for a while. I had to examine the exit code to see it terminated successfully.

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

Replying to vorner:

I have few comments.

  • Why is the unlink in boss's dump pid needed? Wouldn't overwriting the file (eg. open(filename, 'w')) be enough?

To be honest I didn't think about that much; it was basically a
straightforward port of the equivalent BIND 9 code (written in C).
On thinking about it, I see some subtle difference: with explicit unlink,
we can make sure that a file is created even if there's an existing one
for which the current process doesn't have the write persmission (as long
as the directory is writable).

But this is relatievely a minor difference. If you want to remove the
explicit unlink, I'm okay with that.

  • The README contains note of make test, but the toplevel makefile has systest, not test.

Good catch, fixed.

  • When I run make distcheck, it fails for me:

Another good catch, fixed. I also added some explicit checks related to
this case.

  • The digcomp.pl seems like a bad perl code (not wrong, just not following usual conventions, like declaring variables with my, has a lot of duplicate code, using "none" instead of undef). I get this one is imported and the goal is not to cleanup all imported code, but I believe it would be worth a note to either port it to python sometime or clean this one up.

I added a brief comment about the code origin and possible future changes.
As you said, furhter changes, if we want to do it, would be beyond the
scope of this ticket.

  • The last line of the test is kind of confusing. It starts with E: which probably means End, but I thought it was error for a while. I had to examine the exit code to see it terminated successfully.

Hmm, I agree 'E' is confusing. But I'd keep it for now to be
compatible with BIND 9's framework as much as possible (e.g. there may
be a post-processing script that assumes this notation and we may want
to reuse it without modifying it). For that matter, some other
notations are not clear to me (I don't know what "A" or "I" actually
means, for example). I think we should clarify (as a separate task)
these with BIND 9 developers, and if agreed, update the both notations
in a consistent manner.

comment:9 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

To be honest I didn't think about that much; it was basically a
straightforward port of the equivalent BIND 9 code (written in C).
On thinking about it, I see some subtle difference: with explicit unlink,
we can make sure that a file is created even if there's an existing one
for which the current process doesn't have the write persmission (as long
as the directory is writable).

On the other hand, if the directory is not writable, but the file is (prepared for the process), the simple version works, while this one doesn't.

But this is relatievely a minor difference. If you want to remove the
explicit unlink, I'm okay with that.

I think for the sake of simplicity, we might want to remove it.

  • The last line of the test is kind of confusing. It starts with E: which probably means End, but I thought it was error for a while. I had to examine the exit code to see it terminated successfully.

Hmm, I agree 'E' is confusing. But I'd keep it for now to be
compatible with BIND 9's framework as much as possible (e.g. there may
be a post-processing script that assumes this notation and we may want
to reuse it without modifying it). For that matter, some other
notations are not clear to me (I don't know what "A" or "I" actually
means, for example). I think we should clarify (as a separate task)
these with BIND 9 developers, and if agreed, update the both notations
in a consistent manner.

Or add a documentation about what they mean somewhere, at last.

Anyway, the make distcheck still fails for me :-(. This time when it tries to distclean:

make[3]: Entering directory `/home/vorner/work/bind10/bind10-devel-20110224/_build/tests/system'
rm -rf .libs _libs
rm -f *.lo
test -z "conf.sh" || rm -f conf.sh
sh ../../../tests/system/cleanall.sh
sh: ../../../tests/system/cleanall.sh: No such file or directory
make[3]: *** [distclean-local] Error 127
make[3]: *** Waiting for unfinished jobs....
test . = "../../../tests/system" || test -z "" || rm -f 
make[3]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110224/_build/tests/system'
make[2]: *** [distclean-recursive] Error 1
make[2]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110224/_build/tests'
make[1]: *** [distclean-recursive] Error 1
make[1]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20110224/_build'
make: *** [distcheck] Error 1

The clenall.sh file isn't included in distribution (well, it seams nearly nothing is included).

comment:11 Changed 9 years ago by vorner

I also fixed an error message in one of the scripts.

comment:12 in reply to: ↑ 10 ; follow-up: Changed 9 years ago by jinmei

Replying to vorner:

To be honest I didn't think about that much; it was basically a
straightforward port of the equivalent BIND 9 code (written in C).
On thinking about it, I see some subtle difference: with explicit unlink,
we can make sure that a file is created even if there's an existing one
for which the current process doesn't have the write persmission (as long
as the directory is writable).

On the other hand, if the directory is not writable, but the file is (prepared for the process), the simple version works, while this one doesn't.

When the directory is not writable we can't even create the PID file
in the normal scenario (note that bind10 cleans it up on normal
shutdown), so the "simple" version won't be useless most of the time.

That said...

But this is relatievely a minor difference. If you want to remove the
explicit unlink, I'm okay with that.

I think for the sake of simplicity, we might want to remove it.

...at least at the moment the PID file is only intended to be used for
tests, where the above failure cases won't matter much anyway. So I
accepted your preferred way, and removed the unlink.

  • The last line of the test is kind of confusing. It starts with E: which probably means End, but I thought it was error for a while. I had to examine the exit code to see it terminated successfully.

Hmm, I agree 'E' is confusing. But I'd keep it for now to be
compatible with BIND 9's framework as much as possible (e.g. there may
be a post-processing script that assumes this notation and we may want
to reuse it without modifying it). For that matter, some other
notations are not clear to me (I don't know what "A" or "I" actually
means, for example). I think we should clarify (as a separate task)
these with BIND 9 developers, and if agreed, update the both notations
in a consistent manner.

Or add a documentation about what they mean somewhere, at last.

I'll handle this as a separate ticket as I don't even know what some
of them mean.

Anyway, the make distcheck still fails for me :-(. This time when it tries to distclean:

Okay, I was lazy:-) and sorry for bothering you again.

This time I made it sure by performing distcheck and creating and
using a tar ball. While I did it I noticed other necessary files are
missing in the tar ball even if they are not used in distcheck, and I
fixed this by adding everything to EXTRA_DIST.

I also noticed some other points that were not covered in the
discussion so far, and fixed them:

  • changed test_dump_pid_notexist. It was not what I actually intended to test and what it did didn't make sense (although it didn't fail).
  • in system tests use port 53210 instead of 5300 to be aligned with #523
  • remove the copy of digcomp.pl and testsock.pl and refer to these in the BIND 9 source tree (this system test already relies on it). these files don't have to be modified for BIND 10 specific purposes, so sharing the same file would make more sense.

comment:13 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:14 in reply to: ↑ 12 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

I'll handle this as a separate ticket as I don't even know what some
of them mean.

Sure, of course.

Anyway, the make distcheck still fails for me :-(. This time when it tries to distclean:

Okay, I was lazy:-) and sorry for bothering you again.

Thanks, it works now.

  • changed test_dump_pid_notexist. It was not what I actually intended to test and what it did didn't make sense (although it didn't fail).

It did make kind of sense. It checked that if you override the file to use, it doesn't destroy the default one.

But anyway, if you meant this, it looks OK too.

  • in system tests use port 53210 instead of 5300 to be aligned with #523
  • remove the copy of digcomp.pl and testsock.pl and refer to these in the BIND 9 source tree (this system test already relies on it). these files don't have to be modified for BIND 10 specific purposes, so sharing the same file would make more sense.

ACK.

So, hopefully I didn't overlook anything. Please merge it.

comment:15 in reply to: ↑ 14 Changed 9 years ago by jinmei

Replying to vorner:

So, hopefully I didn't overlook anything. Please merge it.

Okay, thanks, merged, and closing ticket.

comment:16 Changed 9 years ago by jinmei

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