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

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Fri Apr 29 21:35:05 CEST 2011


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

--- Comment #13 from Andre Moreira Magalhaes <andrunko at gmail.com> 2011-04-29 12:35:02 PDT ---
(In reply to comment #11)
> +        ret << ChannelPtr::qObjectCast(channel);
>
> Tp::SharedPtr supports upcasting, shouldn't ret << channel just work too?
Done.

> +        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.
Add the default ctor to ConnInfo, but didn't change the rest to use std::fill,
first because I never used it before and second because
std::fill(mMessagesChanServices, mMessagesChanServices + 2, NULL) won't build
and I won't bother trying to figure it out why. I would rather use memset here
as everybody knows how it works.

> +            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?
As I said on IRC, you missed the line that calls observers.append(observer), so
the first one won't get deleted when the second one is created. And yes the
idea is to test if the two share the same observer, by checking the number of
observers returned by ourObservers() does not change between the two calls.

> 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 :)
Still missing, but I will add some code comments there to make it easier to
follow it.

> +    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.
Done.

> +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...
Done.

Pushed -f the branch, will add some comments and merge/release. Thanks for the
review.

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