[Telepathy] fix checkValidProtocolName test in BaseConnection (etc.)

[Daniele E. Domenichelli]

Hi,

On 07/01/13 12:44, Simon McVittie wrote:
> On 06/01/13 19:12, Maksim Melnikau wrote:
>> [PATCH 1/3] fix checkValidProtocolName test in BaseConnection
> 
> This patch looks reasonable, but I'm out of touch with telepathy-qt, so
> someone who is active on that project should review merge it. Please
> consider opening a bug (bugs.freedesktop.org, product = Telepathy,
> component = tp-qt) so it doesn't get lost.

Thanks for those patches, the first 2 look good to me, I'll try to get
tp-qt maintainers to review and merge those changes...


> The error message should probably also gain a space, so that if you use
> an invalid protocol name like "###" it doesn't have a missing space like
> "###is not a valid protocol name".

+1


> I think the need for this change demonstrates how much testing
> BaseConnection has had... I'm disappointed that it was merged without a
> regression test that would have caught this (and also the bug fixed by
> your patch 2/3).

I'm not the author, but BaseConnection is built and installed only if
you enable the flag "ENABLE_EXPERIMENTAL_SERVICE_SUPPORT", that by
definition of experimental might not be tested enough and might contain
bugs...
Anyway I agree about the lack of unit tests of this class.


>> [PATCH 3/3] add .(dot) at TP_QT_ACCOUNT_OBJECT_PATH_BASE end
> 
> No, this is an incompatible change (code that was correct before this
> change would become incorrect) and should be rejected. The commit
> message is also wrong, it talks about dots where in fact the relevant
> character is a slash.

Hmm yeah, that sucks. I really doubt that anyone is using that directly
constant outside tp-qt, though... (We don't use it in KDE Telepathy, and
also see [1])



On 06/01/13 20:46, Maksim Melnikau wrote:
>> diff --git a/TelepathyQt/base-connection.cpp
b/TelepathyQt/base-connection.cpp
> Frankly speaking I would like to add tests for this fixes, could
> smbd suggest good place (class/file)? BaseConnection hasn't any
> tests itself, if I understand correctly.

Yeah tests for BaseConnection are missing. I suggest to add a new test
in tests/dbus...


Cheers,
 Daniele


[1]http://code.ohloh.net/search?s=%22TP_QT_ACCOUNT_OBJECT_PATH_BASE%22&browser=Default&mp=1&ml=1&me=1&md=1&filterChecked=true