Bug 29777

URL updated with results from latest git master. The line is coverage 68.1%, function coverage 64.8% and branch coverage 45.9%. So there has been some improvement, but we aren't there yet. The function coverage improvement is probably mostly thanks to removing the deprecated functions from earlier the release series in 0.5.

(In reply to comment #1)
> URL updated with results from latest git master. The line is coverage 68.1%,
> function coverage 64.8% and branch coverage 45.9%. So there has been some
> improvement, but we aren't there yet. The function coverage improvement is
> probably mostly thanks to removing the deprecated functions from earlier the
> release series in 0.5.

The URL is not public, I've tried my normal Meego username/account and still could not login. Can you publish the document you are referring directly into bugs.fdo.org attachements ?

Sorry, I pasted the wrong link!

Thanks for pointing that out.

LCOV generated html is essentially unreadable without the CSS stylesheet, so attaching a HTML page here doesn't really work.

Andre is working on cleaning up the test suite so the test coverage can be more easily improved. His WIP branch is at http://cgit.collabora.com/git/user/andrunko/telepathy-qt4.git/log/?h=tests-cleanup. That doesn't increase the test coverage yet, but rather enables doing so.

I'll have to leave in a minute, but some quick observations:

     add_subdirectory(glib)
+    add_subdirectory(glib-helpers)

The "glib" subdirectory means "this is GLib code, from tp-glib". So I'd rather not use the naming glib-helpers: just put it in top-level tests/lib/?

+    TestConnHelper(Test *parent,
+            GType gType, const QString &account, const QString &protocol);
+    TestConnHelper(Test *parent,
+            const Tp::ChannelFactoryConstPtr &channelFactory,
+            const Tp::ContactFactoryConstPtr &contactFactory,
+            GType gType, const QString &account, const QString &protocol);


Why separate ctors for passing factories or not - couldn't you do the same, and additionally enable passing only a channel factory, by using default parameters. You can't obviously for the varargs one though, so arguably this could be OK for consistency reasons.

I'll review thoroughly tomorrow.

+    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?

(In reply to comment #5)
>      add_subdirectory(glib)
> +    add_subdirectory(glib-helpers)
> 
> The "glib" subdirectory means "this is GLib code, from tp-glib". So I'd rather
> not use the naming glib-helpers: just put it in top-level tests/lib/?
tests/lib does not depend on glib code and tests/lib/glib is glib specific, hence the new glib-helpers, no change here.

> +    TestConnHelper(Test *parent,
> +            GType gType, const QString &account, const QString &protocol);
> +    TestConnHelper(Test *parent,
> +            const Tp::ChannelFactoryConstPtr &channelFactory,
> +            const Tp::ContactFactoryConstPtr &contactFactory,
> +            GType gType, const QString &account, const QString &protocol);
> 
> 
> Why separate ctors for passing factories or not - couldn't you do the same, and
> additionally enable passing only a channel factory, by using default
> parameters. You can't obviously for the varargs one though, so arguably this
> could be OK for consistency reasons.
> 
This was added for consistency, so no change here.

(In reply to comment #6)
> +    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.
Removed TestConnHelper::disconnectWithDBusError completely. 

> > 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.
Removed the conn-basics patch.
 
> 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.
Cool :)
 
> 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.
Removed this patch also.
 
> -    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.
True true, I missed that, also reverted this patch, keeping only one place calling TEST_VERIFY_OP.
 
> 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?
The idea is to start improving the coverage now. I am happy with the refactoring. If I find any other place that could gain some helper methods I will do it also.

Andre's test suite cleanup branch is now merged and he's moved on to working on expanding the test coverage.

LCOV report previously in URL is outdated with the most recent test additions, which bring up test coverage to a more manageable level. However, we have some issues where we've had to disable certain tests with specific Qt versions because they have race conditions or other bugs.

These tests should be enabled and then the test coverage re-checked with all tests enabled, to see if this bug could finally be closed.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-qt/issues/9.