SimulatedConnection doesn't claim to implement Contacts, so we're testing fallback code paths in telepathy-glib. We need to make GetContactAttributes and GetContactByID realistic - merge information from GetKnownAvatarTokens, GetPresences, GetAliases. We only need to do any of these for the self-contact, though. In addition, the following tests (and maybe more) will need modification to deal with this: tests/twisted/account-manager/irc.py (expects GetAliases) tests/twisted/account-manager/nickname.py (expects GetAliases) This can be done on master.
Created attachment 86921 [details] [review] 01/11] Get rid of vestigial Capabilities code --- This doesn't do anything useful, and won't compile in next.
Created attachment 86922 [details] [review] 02/11] Use AC_PROG_MKDIR_P and MKDIR_P instead of AM_PROG_MKDIR_P and mkdir_p The latter are deprecated, and recent Automake makes a lot of noise about them.
Created attachment 86923 [details] [review] 03/11] Use AM_CPPFLAGS instead of deprecated INCLUDES
Created attachment 86924 [details] [review] 04/11] Use subdir-objects (compile a/b.c to a/b.lo, not ./b.lo) Automake 2 will make this the default, and 1.14 warns about not using it. --- See <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13378>. The Automake 1.9 NEWS mentions some improvements that are conditional on subdir-objects, so I think it's safe to assume that we don't need a dependency bump.
Created attachment 86925 [details] [review] 05/11] dbus-account-plugin: include more telepathy-glib headers On the "next" branch it will stop getting these as a side-effect of something else.
Created attachment 86926 [details] [review] 06/11] servicetest: if expect() fails, log what was expected It's usually possible to derive it from the backtrace, but logging the pattern (as expect_many() does) makes it quicker.
Created attachment 86927 [details] [review] 07/11] McdConnection: replace GetInterfaces with Get(..., "Interfaces") In Telepathy 1.0, GetInterfaces will no longer exist.
Created attachment 86928 [details] [review] 08/11] run-test.sh.in: improve output
Created attachment 86929 [details] [review] 09/11] Implement Contacts properly on SimulatedConnection, and fix tests (I should --amend this text into a long commit message when I commit this) The SimulatedConnection used in the regression tests didn't previously list Contacts in its supported interfaces, even though it implemented at least part of the interface. account-manager/avatar-*.py: stop expecting GetKnownAvatarTokens, since implementing Contacts will mean telepathy-glib uses GetContactAttributes or possibly GetContactByID. This actually unmasks a bug - GetContactAttributes is the wrong thing for MC to be using, because of some subtleties in the spec of GetContactAttributes and GetKnownAvatarTokens - but that was a bug that already existed for all Contacts CMs, and I'll address it in a subsequent commit. account-manager/irc.py, account-manager/nickname.py: stop expecting GetAliases, likewise Because the Contacts interface is more variant-based, this exposed a bug in dbus-python (<https://bugs.freedesktop.org/show_bug.cgi?id=69967>) which I worked around. I didn't add test coverage for CMs *without* Contacts (the sort we were previously simulating) because any such CM is considered to be unusably broken these days; the interface has been mandatory since 2009! --- I'll take a positive review of this as also being a positive review for "copy the extra constants into Gabble".
Created attachment 86930 [details] [review] [10/11] avatar-refresh test: subsume avatar-persist, and test more situations We have some sort of combinatorial explosion going on here, and it seems best to test it in a somewhat systematic way: * is the protocol one where avatars persist on the server (Gabble) or not (Salut)? * if it's like Gabble, does it download our own avatar token before signalling CONNECTED (as I suspect Haze does), or on-demand after GetKnownAvatarTokens (as Gabble appears to)? * if it's like Gabble, is the server storing an avatar for us? * in either case, do we have an avatar stored locally, and has it previously been uploaded or not? In addition, the avatar-refresh and avatar-persist tests exercised migration from ~/.missioncontrol and a low-priority XDG_DATA_DIRS entry (respectively) to ~/.local/share. I didn't do that in a loop, because it isn't applicable in all cases and would lead to even more combinations - testing each case once should be enough. --- This found Bug #70010 in telepathy-glib, and will fail until that bug is fixed. I'm probably going to backport 09/11, this and 11/11 to the 5.16 branch before releasing MC 5.16.0, so please review accordingly.
Created attachment 86932 [details] [review] 11/11] McdAccount: go back to using GetKnownAvatarTokens Sadly, contact attributes aren't enough to distinguish between "this protocol doesn't store avatars and you haven't re-sent your avatar since you last connected", "this protocol stores avatars but the CM hasn't checked for your current avatar yet", and "this protocol stores avatars, but there is no avatar on the server for you". GetKnownAvatarTokens specifically excludes the middle option (blocking on a server round-trip if it needs to), and uses "avatar token missing" for the first and "avatar token empty" for the last. --- Avatars1 (Bug #55920) will fix this hopelessly subtle logic, hopefully... Next time I design an API where NULL/omission and the empty string are both allowed, and mean different things, please remind me how long this bug took to fix. :-)
Comment on attachment 86921 [details] [review] 01/11] Get rid of vestigial Capabilities code Review of attachment 86921 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 86922 [details] [review] 02/11] Use AC_PROG_MKDIR_P and MKDIR_P instead of AM_PROG_MKDIR_P and mkdir_p Review of attachment 86922 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 86923 [details] [review] 03/11] Use AM_CPPFLAGS instead of deprecated INCLUDES Review of attachment 86923 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 86924 [details] [review] 04/11] Use subdir-objects (compile a/b.c to a/b.lo, not ./b.lo) Review of attachment 86924 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 86925 [details] [review] 05/11] dbus-account-plugin: include more telepathy-glib headers Review of attachment 86925 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 86926 [details] [review] 06/11] servicetest: if expect() fails, log what was expected Review of attachment 86926 [details] [review]: ----------------------------------------------------------------- ++ Can't we just sync with servicetest.py from Gabble?
Comment on attachment 86927 [details] [review] 07/11] McdConnection: replace GetInterfaces with Get(..., "Interfaces") Review of attachment 86927 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 86928 [details] [review] 08/11] run-test.sh.in: improve output Review of attachment 86928 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 86929 [details] [review] 09/11] Implement Contacts properly on SimulatedConnection, and fix tests Review of attachment 86929 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 86930 [details] [review] [10/11] avatar-refresh test: subsume avatar-persist, and test more situations Review of attachment 86930 [details] [review]: ----------------------------------------------------------------- ok I guess
Comment on attachment 86932 [details] [review] 11/11] McdAccount: go back to using GetKnownAvatarTokens Review of attachment 86932 [details] [review]: ----------------------------------------------------------------- Pretty nasty indeed, ++
(In reply to comment #17) > 06/11] servicetest: if expect() fails, log what was expected > ++ > > Can't we just sync with servicetest.py from Gabble? This is new code. I'll copy it into Gabble's version too.
(In reply to comment #23) > (In reply to comment #17) > > 06/11] servicetest: if expect() fails, log what was expected > > ++ > > > > Can't we just sync with servicetest.py from Gabble? > > This is new code. I'll copy it into Gabble's version too. Works for me then. ++
Fixed in git for 5.16.0 (Attachment #86929 [details], Attachment #86930 [details], Attachment #86932 [details]) and 5.17.0 (the rest).
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.