Merge lp:~sil2100/unity/tests_glib_timeout_modifications into lp:unity

[Łukasz Zemczak]

Change some test_glib_source unit tests. Those tests are measuring times of execution of timeouts, which is not really suitable for unit testing. Those tests are also failing sometimes on ARM machines, as the timeouts are longer than expected. We're dropping time measurement from these tests, only testing if the timeouts actually happen, because that seems enough.

Change for comment.

- Problem:

3 tests from the test_glib_source.cpp (TestGLibTimeout) suite are failing on most ARM devices. Those tests measure time of the glib::Timeout executions, which are slower on ARM. Also, we shouldn't be really doing tests like this in the unit test suite.

- Fix:

Remove time measurement in the glib::Timeout failing test functions. Instead, all that we need is actually just making sure that the one-shot and multiple-shots just work (i.e. the callbacks are executed). We don't really care if the timeouts happen a bit later, since we can't do anything about that really if the hardware has issues.

- Tests:

N/A

[Francis Ginther (fginther)]

The MultipleShotsRun test doesn't look very useful now as it waits until the callback count is 6, then checks that it is 6. Also, what happens if the callback count never increments? The other test changes look appropriate.

Perhaps it should test that callback count is within a given range, but I'm unsure of the appropriate tolerance. Just a suggestion:
TEST(TestGLibTimeout, MultipleShotsRun)
{
  callback_called = false;
  callback_call_count = 0;
  {
    Timeout timeout(100, &OnSourceCallbackContinue);
    Utils::WaitForTimeoutMSec(650);
    EXPECT_TRUE(timeout.IsRunning());
  }
  EXPECT_TRUE(callback_called);
  EXPECT_GE(callback_call_count, 5);
  EXPECT_LT(callback_call_count, 7);
}

[Łukasz Zemczak (sil2100)]

The test, as it is right now, works in the following way. We wait until the callback_call_count is 6 OR when the timeout happens (I think the default is 10 seconds? Or something like that). That is why there is that callback_call_count check at the end actually. Since if the timeout happens, we check how many times the callback_call_count happened. Actually, EXPECT_GE() is better there!

I would prefer not to measure time in a test like this. Since when this test fails, the number of callback calls is sometimes 3-4, so we would have to be REALLY tolerant. This would make such testing a bit pointless... That's why I wanted to actually just make sure that it can timeout more than once (6 times).

Anyway, I still wonder about this test though, since you're right, maybe we could make it more useful somehow. Also, there are many other tests MultipleShotsRun in this source file which in fact also measure time. So maybe I would have to change those as well.

[Marco Trevisan (Treviño) (3v1n0)]

Of course we lose some checks, but we can survive without them... However probably you can use:

Utils::WaitUntil(check_function, true, 1); instead...

53 - timeout.removed.connect([&] (unsigned int id) { clock_gettime(CLOCK_MONOTONIC, &post); });

Replace it with
timeout.removed.connect([&] (unsigned int id) { removed_called = true; });});

And check for removed_called value at the end...

[Łukasz Zemczak (sil2100)]

I changed it as suggested. Do you think this would be a sufficient workaround for now?

[Marco Trevisan (Treviño) (3v1n0)]

Yeah, it should... Let's see :)

review: Approve

=== modified file 'tests/test_glib_source.cpp'
--- tests/test_glib_source.cpp	2012-10-11 01:44:15 +0000
+++ tests/test_glib_source.cpp	2012-11-28 12:28:20 +0000
@@ -131,55 +131,46 @@
 {
   callback_called = false;
   callback_call_count = 0;
-  struct timespec pre, post;
+  bool removed_called = false;
 
   Timeout timeout(100, &OnSourceCallbackStop);
-  clock_gettime(CLOCK_MONOTONIC, &pre);
-  timeout.removed.connect([&] (unsigned int id) { clock_gettime(CLOCK_MONOTONIC, &post); });
+  timeout.removed.connect([&] (unsigned int id) { removed_called = true; });
 
   Utils::WaitForTimeoutMSec(500);
   EXPECT_FALSE(timeout.IsRunning());
   EXPECT_TRUE(callback_called);
   EXPECT_EQ(callback_call_count, 1);
-  int time_delta = unity::TimeUtil::TimeDelta(&post, &pre);
-  EXPECT_GE(time_delta, 100);
-  EXPECT_LT(time_delta, 110);
+  EXPECT_TRUE(removed_called);
 }
 
 TEST(TestGLibTimeout, MultipleShotsRun)
 {
   callback_called = false;
   callback_call_count = 0;
-  struct timespec pre, post;
+  bool removed_called = false;
 
   {
+  auto check_function = []() { return (callback_call_count < 6) ? false : true; };
   Timeout timeout(100, &OnSourceCallbackContinue);
-  clock_gettime(CLOCK_MONOTONIC, &pre);
-  timeout.removed.connect([&] (unsigned int id) { clock_gettime(CLOCK_MONOTONIC, &post); });
-
-  Utils::WaitForTimeoutMSec(650);
+  timeout.removed.connect([&] (unsigned int id) { removed_called = true; });
+  Utils::WaitUntil(check_function, true, 1);
   EXPECT_TRUE(timeout.IsRunning());
   }
 
   EXPECT_TRUE(callback_called);
-  EXPECT_EQ(callback_call_count, 6);
-  int time_delta = unity::TimeUtil::TimeDelta(&post, &pre);
-  EXPECT_GE(time_delta, 600);
-  EXPECT_LT(time_delta, 700);
+  EXPECT_GE(callback_call_count, 6);
+  EXPECT_TRUE(removed_called);
 }
 
 TEST(TestGLibTimeout, OneShotRunWithEmptyCallback)
 {
-  struct timespec pre, post;
+  bool removed_called = false;
   Timeout timeout(100, Source::Callback());
-  clock_gettime(CLOCK_MONOTONIC, &pre);
-  timeout.removed.connect([&] (unsigned int id) { clock_gettime(CLOCK_MONOTONIC, &post); });
+  timeout.removed.connect([&] (unsigned int id) { removed_called = true; });
 
   Utils::WaitForTimeoutMSec(500);
   EXPECT_FALSE(timeout.IsRunning());
-  int time_delta = unity::TimeUtil::TimeDelta(&post, &pre);
-  EXPECT_GE(time_delta, 100);
-  EXPECT_LT(time_delta, 110);
+  EXPECT_TRUE(removed_called);
 }
 
 TEST(TestGLibTimeout, Removal)