Page MenuHomePhabricator

Add unit tests for VMRange
ClosedPublic

Authored by teemperor on Jul 17 2018, 12:10 AM.

Diff Detail

Event Timeline

teemperor created this revision.Jul 17 2018, 12:10 AM
labath added a subscriber: labath.Jul 17 2018, 1:46 AM

The tests seem fine, but as a matter of style I would suggest two changes:

  • replace ASSERT_XXX with EXPECT_XXX: EXPECT macros will not terminate the test, so you'll be able to see additional failures, which is helpful in pinpointing the problem. ASSERT is good for cases where the subsequent checks make no sense or will crash if the previous check is not satisfied (but that is not the case for your checks AFAICT).
  • replace ASSERT_TRUE(foo == bar) with ASSERT_EQ(foo, bar) (and similar for other relational operators). Right not that does not matter much, but if someone later implements operator<< for this class, you will automatically get a helpful error message describing the objects instead of a useless "false is not true" message.
teemperor updated this revision to Diff 155899.Jul 17 2018, 9:16 AM
  • ASSERT_ -> EXPECT_
  • Now using gtest's comparison macros where possible.
  • Added a PrintTo function as otherwise the gtest comparison macros won't compile.
clayborg added inline comments.
unittests/Utility/VMRangeTest.cpp
42

Seems like a few more edge cases might be good:

EXPECT_NE(range1, VMRange(0x100, 0x1ff));
EXPECT_NE(range1, VMRange(0x100, 0x201));
EXPECT_NE(range1, VMRange(0x0ff, 0x200));
EXPECT_NE(range1, VMRange(0x101 0x200));
71

Probably don't need the 0xFE as 0xFF will test the condition of an address before 0x100

78

Should probably test the UINT64_MAX address instead of just 0xfff as the last edge case

88

add a zero sized range check where the address is inside the other:

EXPECT_FALSE(range.Contains(VMRange(0x100, 0x100)));
91

Add a test to see that an equal range is also contained:

EXPECT_TRUE(range.Contains(VMRange(0x100, 0x200));
92

change 0x208 to 0x201 so it is just outside the range

92

Add check for MAX address:

EXPECT_FALSE(range.Contains(VMRange(0x105, UINT64_MAX)));

103

Test ordering of empty ranges? Not sure if that makes sense

  • Added a PrintTo function as otherwise the gtest comparison macros won't compile.

Sorry, I did not anticipate that. It looks like the iterator typedef inside the VMRange is confusing gtest's universal printing logic (normally it does not require a type to be printable and it will just print a hex dump instead). In any case, having the PrintTo function is a good thing, so thanks for adding it.

teemperor updated this revision to Diff 157172.Jul 24 2018, 4:39 PM
  • Applied Greg's requested changes (thank you very much).

Merging this in as Davide suggested.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 24 2018, 4:52 PM
Closed by commit rL337873: Add unit tests for VMRange (authored by teemperor, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.

The new test cases did catch a real bug:

[ RUN ] VMRange.CollectionContains
llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure
Value of: VMRange::ContainsRange(collection, VMRange(0x100, 0x104))

Actual: false

Expected: true

class RangeInRangeUnaryPredicate {
public:
RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} // note that
_range binds to a temporary!
bool operator()(const VMRange &range) const {
return range.Contains(_range);
}
const VMRange &_range;
};

I just sent out a review for the fix (https://reviews.llvm.org/D50290)