Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#184 closed task (fixed)

cfgmgr test coverage

Reported by: jinmei Owned by: jelte
Priority: medium Milestone: 04. 2nd Incremental Release: Early Adopters
Component: configuration Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

This is backlog item 79 (see http://bind10.isc.org/wiki/f2F1_Y2_Wed).

Subtickets

Attachments (1)

bind10_cfgmgr_tmpfile.patch (2.0 KB) - added by jelte 10 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 10 years ago by jinmei

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

Frankly, I'm not sure what we should do for this backlog.

At least line-based coverage wise it's already quite complete:
http://bind10.isc.org/~tester/LATEST_PYTHON_UNITTEST_COVERAGE/src_lib_python_isc_config_cfgmgr.html

Nevertheless, I've added a couple of tests for minor corner cases to make it even more complete. The new tests are available in branches/184, which are ready for review.

While writing this tests I've noticed some possible issues of the code:

  • when ConfigManagerData.write_to_file() fails after creating a temporary file, it won't clean up the tmp file. It probably should do so.
  • on a related note, we should probably use Python's tmpfile module so that the temporary file will have a unique file name and will be cleaned up automatically.
  • as commented in the test case, however, if we do this the newly added test won't work as expected because it assumes a fixed name temporary file. but it's probably okay; hard-coding such an assumption in test code is probably not a good practice anyway.

I'm going to give this small ticket to Jelte, because he should be most familiar with the code, and this ticket contains some comments on the implementation (i.e. tested code) itself.

comment:2 Changed 10 years ago by jinmei

  • Status changed from assigned to reviewing

Changed 10 years ago by jelte

comment:3 follow-up: Changed 10 years ago by jelte

  • Owner changed from jelte to jinmei

Ok, i've made it use tempfile.NamedTempFile? (for which the online documentation turns out to be wrong/incomplete, sigh, if i remember i need to submit that).

Please review attached patch too :)

It doesn't use the auto-delete function (because we need to move the file if everything is ok), so there is an extra check+delete later.

I'm not sure we still need the second of these new tests if we use this patch (kind of what you said already). Having the first seems ok.

comment:4 in reply to: ↑ 3 Changed 10 years ago by jinmei

  • Owner changed from jinmei to jelte

Replying to jelte:

Please review attached patch too :)

This one looks okay (but see below).

I'm not sure we still need the second of these new tests if we use this patch (kind of what you said already). Having the first seems ok.

maybe we can temporarily tweak self.config_manager_data.data_path before doing write_to_file() so that NamedTemporaryFile() will fail.

Whether or not we do this test, note that if NamedTemporaryFile() fails it will raise an an OSError, not an IOError. So we should probably remove the except block for IOError (as it would not actually be "expected" in this case and should be handled at a higher level if it ever happens).

comment:5 follow-up: Changed 10 years ago by jelte

Actually, IOError is there in case write() fails (disk full for instance), in which case I believe this is the right level to catch it, so unless you disagree (or I am wrong), i'm marking this as fixed and commit it.

comment:6 in reply to: ↑ 5 Changed 10 years ago by jinmei

Replying to jelte:

Actually, IOError is there in case write() fails (disk full for instance), in which case I believe this is the right level to catch it, so unless you disagree (or I am wrong), i'm marking this as fixed and commit it.

Yeah I overlooked the case of disk full (or other failure of write). So the current code is okay for me.

comment:7 Changed 10 years ago by jelte

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

Committed patch in trunk r1859

comment:8 Changed 9 years ago by shane

  • Component changed from Unclassified to configuration
Note: See TracTickets for help on using tickets.