[llvm-commits] [llvm] r61576 -/llvm/trunk/unittests/ADT/ImmutableSetTest.cpp

[Nuno Lopes]

Hi,

So it was the first time I was using gtests, so I didn't know those details. 
Anyway thanks for reviewing the test. I've commited a patch that should 
address your comments.

Thanks,
Nuno


----- Original Message -----
> Hi Nuno,
>
> Thanks for adding unittests!  I have some comments below.
>
> 2009/1/2 Nuno Lopes <nunoplopes at sapo.pt>
>
>> Author: nlopes
>> Date: Fri Jan  2 07:49:50 2009
>> New Revision: 61576
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=61576&view=rev
>> Log:
>> fist short at a new unit test for ImmutableSets. no bugs found, though :P
>>
>> Added:
>>    llvm/trunk/unittests/ADT/ImmutableSetTest.cpp
>>
>> Added: llvm/trunk/unittests/ADT/ImmutableSetTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/ImmutableSetTest.cpp?rev=61576&view=auto
>>
>>
>> ==============================================================================
>> --- llvm/trunk/unittests/ADT/ImmutableSetTest.cpp (added)
>> +++ llvm/trunk/unittests/ADT/ImmutableSetTest.cpp Fri Jan  2 07:49:50 
>> 2009
>> @@ -0,0 +1,208 @@
>> +// llvm/unittest/ADT/ImmutableSetTest.cpp - ImmutableSet unit tests -*-
>> C++ -*-//
>>
>
> You can put the description below the typical file header to make this 
> line
> look standard, but this is minor.
>
>
>> +namespace {
>> +class ImmutableSetTest : public testing::Test {
>> +};
>> +
>> +
>> +TEST_F(ImmutableSetTest, EmptyIntSetTest) {
>> +  ImmutableSet<int>::Factory f;
>
>
> I see you use a Factory in every test -- you can make this a member of the
> ImmutableSetTest above, declare it once, and drop the declaration from 
> every
> specific test -- that will make your code shorter.  A new Factory will be
> created for each test, so you don't need to worry about getting a used
> Factory. :-)
>
>
>> +static char *ptr; // tmp var
>> +struct MyIter {
>> +  int counter;
>> +  MyIter() : counter(0) {}
>> +  void operator()(char c) {
>> +    *ptr++ = c;
>> +    ++counter;
>> +  }
>> +};
>>
>
> Global state is frowned upon -- if you don't properly clear it between
> tests, strange bugs will occur, and they will depend on the order of the
> test execution (which aren't guaranteed to execute in any particular
> order).  This will lead to strange Heisenbugs.
>
> Instead, you can make another Fixture class, and make this state a part of
> that class.  If you set up that state, you'll know you're always getting a
> clean beginning state, and you'll have nothing to worry about.
>
>
>> +TEST_F(ImmutableSetTest, CallbackCharSetTest) {
>> +  ImmutableSet<char>::Factory f;
>> +  ImmutableSet<char> S = f.GetEmptySet();
>> +
>> +  ImmutableSet<char> S2 = f.Add(f.Add(f.Add(S, 'a'), 'e'), 'i');
>> +  ImmutableSet<char> S3 = f.Add(f.Add(S2, 'o'), 'u');
>> +
>> +  char buffer[6] = {0};
>> +  ptr = buffer;
>> +  S3.foreach<MyIter>();
>> +
>> +  ASSERT_EQ(buffer[0], 'a');
>> +  ASSERT_EQ(buffer[1], 'e');
>> +  ASSERT_EQ(buffer[2], 'i');
>> +  ASSERT_EQ(buffer[3], 'o');
>> +  ASSERT_EQ(buffer[4], 'u');
>> +  ASSERT_EQ(buffer[5], 0);
>>
>
> No need to use ASSERT here, EXPECT will work just as well, and will tell 
> you
> if you have multiple errors (e.g., you'll see if 2 of them were swapped, 
> for
> instance, which ASSERT won't tell you.
>
> Also, and this is very important, the expected value in ASSERT and EXPECT
> values should go FIRST (you do this correctly above, but not here).  This 
> is
> important because if this assertion fails, the error will print as:
> ASSERT failed:
> expected value: <foo>
> actual value: <bar>
>
> If you mix and match the order of the values, you'll get confusing error
> messages.
>
> +    ASSERT_EQ(*I, i++);
>> +  }
>> +  ASSERT_EQ(i, 0);
>> +
>> +  i = 0;
>> +  for (ImmutableSet<long>::iterator I = S2.begin(), E = S2.end(); I != 
>> E;
>> ++I) {
>> +    ASSERT_EQ(*I, i++);
>> +  }
>> +  ASSERT_EQ(i, 3);
>> +
>> +  i = 0;
>> +  for (ImmutableSet<long>::iterator I = S3.begin(), E = S3.end(); I != 
>> E;
>> I++) {
>> +    ASSERT_EQ(*I, i++);
>> +  }
>> +  ASSERT_EQ(i, 6);
>>
>
> Same here on the order of expected vs. actual values -- please reverse 
> them.
>
>
> Misha