Project

General

Profile

Tasks #4066

unit tests for src/include/buffer.h

Added by Loic Dachary over 6 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Low
Assignee:
Category:
qa
Target version:
-
Start date:
02/09/2013
Due date:
02/16/2013
% Done:

100%

Spent time:
Tags:
Reviewed:
Affected Versions:
Pull request ID:

Description

The tests for buffer.h are incomplete They must cover 100% of the lines of code.

joshd: loicd: if ever a class could use unit testing (and documentation), it's bufferlist. 
loicd: I'll add to https://github.com/ceph/ceph/blob/master/src/test/bufferlist.cc in order to improve the coverage of http://dachary.org/wp-uploads/2013/01/ceph/common/buffer.cc.gcov.html
joshd: loicd: yeah

Related issues

Related to Ceph - Bug #4170: buffer::ptr::cmp reports a == ab Resolved 02/16/2013
Related to Ceph - Bug #4123: buffer::list::zero overflows and returns before completion Resolved 02/14/2013
Blocked by Ceph - Bug #4157: operator>=(bufferlist& l, bufferlist& r) throws on a >= ab Resolved 02/15/2013

Associated revisions

Revision fb472a57 (diff)
Added by Loic Dachary over 6 years ago

unit tests for src/common/buffer.{cc,h}

Implement unit tests covering most lines of code ( > 92% ) and all
methods as show by the output of make check-coverage :
http://dachary.org/wp-uploads/2013/03/ceph-lcov/ .

The following static constructors are implemented by opaque classes
defined in buffer.cc ( buffer::raw_char, buffer::raw_posix_aligned
etc. ). Testing the implementation of these classes is done by
variations of the calls to the static constructors.

copy(const char *c, unsigned len);
create(unsigned len);
claim_char(unsigned len, char *buf);
create_malloc(unsigned len);
claim_malloc(unsigned len, char *buf);
create_static(unsigned len, char *buf);
create_page_aligned(unsigned len);

The raw_mmap_pages class cannot be tested because it is commented out in
raw_posix_aligned. The raw_hack_aligned class is only tested under Cygwin.
The raw_posix_aligned class is not tested under Cygwin.

The unittest_bufferlist.sh script calls unittest_bufferlist with the
CEPH_BUFFER_TRACK=true environment variable to enable the code
tracking the memory usage. It cannot be done within the bufferlist.cc
file itself because it relies on the initialization of a global
variable ( buffer_track_alloc ).

When raw_posix_aligned is called on DARWIN, the data is not aligned
on CEPH_PAGE_SIZE because it calls valloc(size) which is the equivalent of
memalign(sysconf(_SC_PAGESIZE),size) and not memalign(CEPH_PAGE_SIZE,size).
For this reason the alignment test is de-activated on DARWIN.

The tests are grouped in

TEST for buffer::ptr
TEST for buffer::list::iterator
TEST for buffer::list
TEST for buffer::hash

and each method ( and all variations of the prototype ) are
included into a single TEST function.

Although most aspects of the methods are tested, including exceptions
and border cases, inconsistencies are not highlighted . For
instance

buffer::list::iterator i;
i.advance(1);

would dereference a buffer::raw NULL pointer although

buffer::ptr p;
p.wasted()

asserts instead of dereferencing the buffer::raw NULL pointer. It
would be better to always assert in case a NULL pointer is about to be
used. But this is a minor inconsistency that is probably not worth a
test.

The following buffer::list methods

ssize_t read_fd(int fd, size_t len);
int write_fd(int fd) const;

are not fully tested because the border cases cannot be reliably
reproduced. Going thru a pointer indirection when calling the ::writev
or safe_read functions would allow the test to create mockups to synthetize
the conditions for border cases.

tracker.ceph.com/issues/4066 refs #4066

Signed-off-by: Loic Dachary <>

History

#2 Updated by Loic Dachary over 6 years ago

  • Status changed from New to Need Review
  • % Done changed from 0 to 90

#4 Updated by Loic Dachary over 6 years ago

  • Status changed from Need Review to Resolved
  • % Done changed from 90 to 100

Also available in: Atom PDF