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

[Misha Brukman]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090102/17101f0f/attachment.html>