Opened 9 years ago

Closed 9 years ago

#610 closed task (complete)

Stop/Start/Run tests of b10-auth

Reported by: stephen Owned by: jinmei
Priority: medium Milestone: A-Team-Sprint-20110316
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

Starting, stopping, running bindctl to show stats and change configurations. We can do with shell scripts.

Subtickets

Change History (19)

comment:1 follow-up: Changed 9 years ago by jreed

This can also be zone transfers, dig queries, "Xfrin retransfer", loading in-memory datasource, etc.

comment:2 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 3

comment:3 in reply to: ↑ 1 Changed 9 years ago by jinmei

Replying to jreed:

This can also be zone transfers, dig queries, "Xfrin retransfer", loading in-memory datasource, etc.

Supplemental note: according to discussions in the sprint planning session,
the "also" things (if needed) should go to a separate ticket.

Also, once we merge #606 it should be relatively easy to do this task
using that framework.

comment:4 follow-up: Changed 9 years ago by jinmei

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

comment:5 in reply to: ↑ 4 Changed 9 years ago by jinmei

Branch trac610 is ready for review.

Similar to #606 I had to add a new knob for testing. This time I added
an option to bindctl to allow the user to specify the path to user/pass
CSV file.

I believe the test scenarios are straightforward.

One possibly controversial point is bindctl could hang (e.g. waiting
for password due tom some misconfigured setup) indefinitely. For
automated test environments this would not be good, so I guess we'll
need a wrapper invoker with a timeout. It shouldn't be that hard to
implement, but I'd leave it to a different task. (I'll create a
ticket if that's okay).

I'm not sure if we want to have a changelog entry for this ticket.
The newly added test probably isn't worth it, but the new option for
bindctl is a user-visible change (but still quite minor and mostly
test only purposes at the moment). For now I'm skipping to propose
it.

comment:6 Changed 9 years ago by jinmei

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

comment:7 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei

Hello

I agree the changelog is probably more spam in this case than useful information.

With the code and tests, I mostly agree they are OK, but I still have some comments:

  • There was a stray copy-pasted comment (updated)
  • Typo ("HOMEE") in the bindctl tests (updated)
  • The tests failed for me the first time. I discovered that my sh doesn't expand \n in single quotes (AFAIK it even shouldn't). So I replaced them with real newlines. It runs then.
  • Should there be some tests for the command line parsing? (for bindctl)
  • What is the purpose of n=expr $n+1 in the test? I don't see the n being used anywhere and it just keeps increasing unconditionally after each test.

Please check if the changes I propose are OK.

Thanks

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

Replying to vorner:

Hello

I agree the changelog is probably more spam in this case than useful information.

With the code and tests, I mostly agree they are OK, but I still have some comments:

  • There was a stray copy-pasted comment (updated)
  • Typo ("HOMEE") in the bindctl tests (updated)
  • The tests failed for me the first time. I discovered that my sh doesn't expand \n in single quotes (AFAIK it even shouldn't). So I replaced them with real newlines. It runs then.

Please check if the changes I propose are OK.

The changes are okay. Thanks for fixing them.

  • Should there be some tests for the command line parsing? (for bindctl)

I may misunderstand it, but I think such tests should go to bindctl itself.

  • What is the purpose of n=expr $n+1 in the test? I don't see the n being used anywhere and it just keeps increasing unconditionally after each test.

It's a convention of BIND 9 system tests (probably introduced at a
later stage - as we saw the glue tests don't have it). My
understanding is that it helps check the corresponding log (such as
dig output) on failure.

For example, if we see:

I:Checking b10-auth is working by default (0)
I:failed

it means we may want to check dig.out.0 to see what went wrong.

comment:10 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

  • Should there be some tests for the command line parsing? (for bindctl)

I may misunderstand it, but I think such tests should go to bindctl itself.

Well, they should go to bindctl's tests, sure. But my question was about if it would be possible to test if parsing the arguments work (because you added new argument --csv-file-dir). But on the other hand, it is tested kind of implicitly in the tests you just added, so it's probably not really critical.

Thanks

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

Replying to vorner:

Hello

Replying to jinmei:

  • Should there be some tests for the command line parsing? (for bindctl)

I may misunderstand it, but I think such tests should go to bindctl itself.

Well, they should go to bindctl's tests, sure. But my question was about if it would be possible to test if parsing the arguments work (because you added new argument --csv-file-dir). But on the other hand, it is tested kind of implicitly in the tests you just added, so it's probably not really critical.

Ah, okay. I was confused with the command sequence to bindctl (via
echo and pipe) used in the system tests.

For the command line option to bindctl, yes, I was aware that it was
missing and should ideally be tested explicitly. I skipped the test
partly because (as you mentioned) it was indirectly tested through the
system test, and partly because it seemed to require larger
refactoring as option parsing was part of main. On looking at it
again, however, I found it not that difficult; we can use
set_bindctl_options() for testing.

So, according to the "we shouldn't allow an easy excuse of skipping
tests" principle, I added tests for the option. For that I had to
rename "bindctl-source.py", but the original name didn't seem to be a
good one anyway, so renaming itself may also make sense.

Please let me know if this change is okay and if there's anything else
I should address.

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:

Please let me know if this change is okay and if there's anything else
I should address.

I added small hack to make it work with separate builddir (eg. in distcheck). Does it look OK?

If yes, please merge.

Thanks

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

Replying to vorner:

Hello

Replying to jinmei:

Please let me know if this change is okay and if there's anything else
I should address.

I added small hack to make it work with separate builddir (eg. in distcheck). Does it look OK?

It's okay, but didn't you also re-add bindctl-source.py.in? Why?

comment:16 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Ups, that is by mistake. I checked it out to see if there were any changes as well as renaming and when I did git commit -a later on, git misunderstood my intentions and commited it as well.

Anyway, you can just remove it while merging. Apologies for the delay.

Thanks

comment:18 in reply to: ↑ 17 Changed 9 years ago by jinmei

Replying to vorner:

Ups, that is by mistake. I checked it out to see if there were any changes as well as renaming and when I did git commit -a later on, git misunderstood my intentions and commited it as well.

Anyway, you can just remove it while merging. Apologies for the delay.

Okay, and no problem. This is not urgent.

Thanks for review, merge done, closing ticket.

comment:19 Changed 9 years ago by jinmei

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