[2202] Test improvements

* Reimplementing destroyLocked as death test
* Comments
* Includes to use < > instead of " "
* Don't use c99-isms.

src/lib/util/threads/tests/lock_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2010  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -12,8 +12,8 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include "../lock.h"
-#include "../thread.h"
+#include <util/threads/lock.h>
+#include <util/threads/thread.h>
 
 #include <gtest/gtest.h>
 
@@ -50,21 +50,17 @@ TEST(MutexTest, lockMultiple) {
 }
 
 // Destroying a locked mutex is a bad idea as well
-//
-// FIXME: The test is disabled, since it screws something up in the VM (other
-// tests fail then with rather cryptic messages, memory dumps and stuff).
-// Any idea how to make the test work and reasonably safe?
-TEST(MutexTest, DISABLED_destroyLocked) {
-    // TODO: This probably won't work for non-debug mutexes. Disable on non-debug
-    // compilation.
-    Mutex* mutex = new Mutex;
-    new Mutex::Locker(*mutex);
-    EXPECT_THROW(delete mutex, isc::InvalidOperation);
-    // Note: This leaks the locker. But this is a test for development aid
-    // exception. The exception won't happen in normal build anyway and seeing
-    // it means there's a bug. And we can't delete the locker now, since it
-    // would access uninitialized memory.
+#ifdef EXPECT_DEATH
+TEST(MutexTest, destroyLocked) {
+    EXPECT_DEATH({
+        Mutex* mutex = new Mutex;
+        new Mutex::Locker(*mutex);
+        delete mutex;
+        // This'll leak the locker, but inside the slave process, it should
+        // not be an issue.
+    }, "");
 }
+#endif
 
 // This test tries if a mutex really locks. We could try that with a deadlock,
 // but that's not practical (the test would not end).
@@ -74,6 +70,9 @@ TEST(MutexTest, DISABLED_destroyLocked) {
 // threads are started and the operation must be complicated enough so the
 // compiler won't turn it into some kind of single atomic instruction.
 //
+// FIXME: Any idea for a simpler but non-atomic operation that keeps an
+// invariant?
+//
 // So, we'll have an array of numbers. Each thread will try to repeatedly
 // find a number large at least as half of the average, take the number,
 // distribute the value across the rest of the positions of the array and
@@ -82,29 +81,38 @@ TEST(MutexTest, DISABLED_destroyLocked) {
 // one will add something to the position another one have chosen and
 // the other one will then zero it, not taking the new value into account.
 // That'd lower the total value of the array.
+//
+// We run the threads in opposite directions (so we have no chance of them
+// keeping the same distance to each other and not meeting). Also, the indexing
+// is performed in a circular manner like with a ring buffer.
 const unsigned long long length = 1000;
 const unsigned long long iterations = 10000;
 const unsigned long long value = 2000;
 void
-performStrangeOperation(long long unsigned* array, int direction,
+performStrangeOperation(std::vector<long long unsigned> array, int direction,
                         Mutex* mutex)
 {
     unsigned long long position = 0;
     for (size_t i = 0; i < iterations; i ++) {
         Mutex::Locker lock(*mutex);
+        // Find a place with large enough value
         while (array[position % length] < value) {
             position += direction;
         }
-        unsigned long long value = array[position % length];
+        // Take the value
+        unsigned long long found_value = array[position % length];
+        // And distribute it to places following the found one, by
+        // adding 1 to each.
         unsigned long long p2 = position;
-        while (value > 0) {
+        while (found_value > 0) {
             p2 += direction;
             if (p2 % length == position % length) {
                 continue;
             }
             array[p2 % length] ++;
-            value --;
+            found_value --;
         }
+        // Zero the distributed value
         array[position % length] = 0;
     }
 }
@@ -112,15 +120,17 @@ performStrangeOperation(long long unsigned* array, int direction,
 TEST(MutexTest, swarm) {
     // This type has a low chance of being atomic itself, further raising
     // the chance of problems appearing.
-    long long unsigned array[length];
+    std::vector<long long unsigned> array(length);
     for (size_t i = 0; i < length; ++ i) {
         array[i] = value;
     }
     Mutex mutex;
+    // Run two parallel threads, each in one direction
     Thread t1(boost::bind(&performStrangeOperation, array, 1, &mutex));
     Thread t2(boost::bind(&performStrangeOperation, array, -1, &mutex));
     t1.wait();
     t2.wait();
+    // Check the sum didn't change.
     long long unsigned sum = 0;
     for (size_t i = 0; i < length; ++ i) {
         sum += array[i];

src/lib/util/threads/tests/thread_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2010  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -12,7 +12,7 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include "../thread.h"
+#include <util/threads/thread.h>
 
 #include <boost/bind.hpp>