[Bug 29777] Ridiculously low unit test coverage

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=29777

--- Comment #6 from Olli Salli <ollisal at gmail.com> 2011-06-09 03:10:31 PDT ---
+    tp_base_connection_disconnect_with_dbus_error(
+            TP_BASE_CONNECTION(mService), errorName, details,
(TpConnectionStatusReason) reason);
+    return ((mLoop->exec() == 0) &&
+            !mClient->isValid() && (mClient->status() ==
Tp::ConnectionStatusDisconnected));
+}

You might want to also check in the return condition for the invalidation
reason being correct. It's a bit weird to have this specialized method for
disconnecting with a specific error, which does some checks, but yet doesn't do
that most important one.

> conn-basics test: Use TestConnHelper.

I would actually leave this test doing all the steps manually: it essentially
doesn't do anything else than what TestConnHelper does, but TestConnHelper
makes the failure reporting for the individual steps much less granular than
doing them manually. Actually this probably means the rather specialized
utility function I commented above can be removed from the helper, and can
continue to live in TestConnBasics as explicit code, giving the fine-grained
failure reporting.

For the other tests, where the point is not to test the individual conn basic
setup functions separately, TestConnHelper seems totally sweet though, making
them a lot thinner.

Similarly, I'd rather revert "conn-requests test: Use
TestConnHelper::create/ensureChannel()". Otherwise, it looks like the channel
and contact request helpers had a great effect cleaning the tests.

-    if (!op->isFinished()) {
-        qWarning() << "unfinished";
-        mLoop->exit(1);
-        return;
-    }
-
     if (op->isError()) {
         // The service signaled busy even before tp-qt4 finished
introspection.
         // FIXME: should the error be something else, actually? Such as,
perchance,
         // org.freedesktop.Telepathy.Error.Busy? (fd.o #29757).
         QCOMPARE(op->errorName(),
QLatin1String("org.freedesktop.Telepathy.Error.Cancelled"));
         qDebug() << "request streams finished already busy";
-        mLoop->exit(0);
-        return;
     }

+    TEST_VERIFY_OP(op);
+

This particular change is wrong. Note how previously the error case with the
particular error used to exit the mainloop with the success code; with
TEST_VERIFY_OP it'd exit it with a failure code.

As explained in the comment, this entire codepath is actually a workaround for
a race condition in the StreamedMedia D-Bus API, where we might not get the
chance to find out much about the new stream when it's already removed due to
the target being busy. But both "busy before introspection completed, hence
rror" and "introspection completed, then get notification for being busy" are
indications of being busy, which is what the test is expecting - with your
change, the test would hit a race condition in one of the two cases.

Ditto for expectTerminate... . I'd revert all the changes in that file, except
the one which actually replaces the same code as the macro expands to with the
macro.

That's it! Do you think you're still going to refactor out test utility code,
or will you move to expanding the test coverage now?

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.