Opened 7 years ago

Closed 7 years ago

#2973 closed defect (fixed)

write tests for ZoneWriter::install

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20130625
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 3.27 Internal?: no

Description

complete this FIXME:

    // FIXME: This retry is currently untested, as there seems to be no
    // reasonable way to create a zone table segment with non-local memory
    // segment. Once there is, we should provide the test.

I guess it was left open because at that time ZoneWriter was an
abstract class and cannot easily be instantiated. It's not the case
anymore, so we should be able to test this (even if we cannot reliably
cause MemorySegmentGrown). I suggest doing something similar to
ZoneDataLoaterTest.relocate.

Subtickets

Change History (13)

comment:1 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 4

comment:2 Changed 7 years ago by jinmei

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed

comment:3 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130625

comment:4 Changed 7 years ago by jinmei

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

comment:5 Changed 7 years ago by jinmei

trac2973 is ready for review.

It actually revealed a bug as I anticipated:-) So I fixed that, too.

I also made one small piggyback fix at 40d9424.

While the branch contains a bug fix, this part hasn't been user
visible in practice, so I don't think we need a changelog for this fix.

comment:6 Changed 7 years ago by jinmei

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

comment:7 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei

Hello

The code looks good. However, the semantics of the addZone method chandeg (the caller is now responsible to free it). But I don't see any other place updated in such manner. Is it that it was already freed (twice) before?

I agree we don't need a changelog entry for a bugfix to code that is not yet used.

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

Thanks for the review.

Replying to vorner:

The code looks good. However, the semantics of the addZone method chandeg (the caller is now responsible to free it). But I don't see any other place updated in such manner. Is it that it was already freed (twice) before?

zone_table_unittest in a169232953e34690749708a29b9f6c22e94b2dbf was
updated according to the semantics change, although it may not look so
straightforward.

The main ZoneWriter class has already freed it on exception other
than MemorySegmentGrown thanks to the holder object. But it's quite
difficult to trigger an exception (other than MemorySegmentGrown)
from ZoneWriter, so it's not been tested.

comment:10 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei
  • Total Hours changed from 0 to 0.77

OK, then it can be merged.

comment:12 in reply to: ↑ 11 Changed 7 years ago by jinmei

Replying to vorner:

OK, then it can be merged.

thanks, merge done, closing.

comment:13 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0.77 to 3.27
Note: See TracTickets for help on using tickets.