[Bug 33525] Helper class(es) for observing calls

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Fri Apr 29 18:59:18 CEST 2011


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

--- Comment #11 from Olli Salli <ollisal at gmail.com> 2011-04-29 09:59:17 PDT ---
+        ret << ChannelPtr::qObjectCast(channel);

Tp::SharedPtr supports upcasting, shouldn't ret << channel just work too?

+        memset(mConns, 0, sizeof(mConns) / sizeof(ConnInfo));
+        memset(mMessagesChanServices, 0, sizeof(mMessagesChanServices) /
sizeof(ExampleEcho2Channel*));
+        memset(mCallableChanServices, 0, sizeof(mCallableChanServices) /
sizeof(ExampleCallableMediaChannel*));

I'd rather make ConnInfo have a ctor which initializes the members to NULL, and
use std::fill(mMessagesChanServices, mMessagesChanServices + 2, NULL) etc to be
cleaner C++ code.

+            SimpleTextObserverPtr textObserver =
SimpleTextObserver::create(acc, contact);
+            QCOMPARE(textObserver->account(), acc);
+            QCOMPARE(textObserver->contactIdentifier(), contact);
+            QVERIFY(textObserver->textChats().isEmpty());
+            textObservers.append(textObserver);
+            QCOMPARE(ourObservers().size(), numRegisteredObservers);
+
+            textObserver = SimpleTextObserver::create(acc, contact);
+            QCOMPARE(textObserver->account(), acc);
+            QCOMPARE(textObserver->contactIdentifier(), contact);
+            QVERIFY(textObserver->textChats().isEmpty());
+            textObservers.append(textObserver);
+            QCOMPARE(ourObservers().size(), numRegisteredObservers);

What's your intention with creating two text observers in a row?

In case your intention is to check that two STOs can share an observer, then
you have a bug: the creation of the second STO frees the first one, because you
assign to the same STOPtr, so they co-exist only for the duration of the
assignment operation.

OTOH, if your intention is to just test that the STO picks up the observer
registered for the SO directly created earlier, why have two STOs at all?

In general, please add oneliner comments every now and then to describe the
intention of the tests when you have a lot of not very explicit things going
on. The crosstalk test could really use a few "we should have this, but not
that, because the filter blahblah" type of comments between the big blocks of
checks too.

In addition to helping the reviewer see just what is being tested (or should
be, enabling cross referencing with what's actually being checked), it also
helps you to think about whether you're testing the right things or not :)

+    while (observers[0]->channels().size() != 1 ||
+           observers[1]->channels().size() != 1 ||
+           textObservers[0]->textChats().size() != 1 ||
+           textObservers[1]->textChats().size() != 1 ||
+           textObserversNoContact[0]->textChats().size() != 1 ||
+           textObserversNoContact[1]->textChats().size() != 1 ||
+           callObservers[0]->streamedMediaCalls().size() != 1 ||
+           callObservers[1]->streamedMediaCalls().size() != 1 ||
+           callObserversNoContact[0]->streamedMediaCalls().size() != 1 ||
+           callObserversNoContact[1]->streamedMediaCalls().size() != 1) {
+        mLoop->processEvents();
+    }

If there IS, in fact, crosstalk, this'd hang forever, which is an irritating
way to discover you've caused a regression (not making the root cause very
clear). So maybe change this loop to wait for the channels()/chats()/calls() to
be !isEmpty()? And after the loop, QCOMPARE their channel counts individually
with 1. Then you know directly from the failed check which one either lost
their channel prematurely or got extra ones due to crosstalk if you cause a
regression.

+void TestSimpleObserver::cleanupTestCase()
+{
+    for (int i = 0; i < 2; ++i) {

You might want to check what I changed TestConnRosterGroups(Legacy)::cleanup()
code to: it's both shorter/cleaner, and doesn't hang if one of the conns didn't
connect, or started disconnecting already (but we didn't catch the invalidation
yet). I hit both cases due to races in the roster code, which was again very
irritating to debug as make check just froze forever...

Perhaps we should actually make the "here's a Tp::Connection and a
TpBaseConnection, make them die" piece of cleanup code shared between the tests
in the Test baseclass?

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



More information about the telepathy-bugs mailing list