[Bug 29777] Ridiculously low unit test coverage

[bugzilla-daemon at freedesktop.org]

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

--- Comment #8 from Andre Moreira Magalhaes <andrunko at gmail.com> 2011-06-09 09:02:57 PDT ---
(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.

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