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 _______________________________________________ telepathy mailing list telepathy@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/telepathy
