Bug 32902 - review Pidgin log store and dbus testsuite
Summary: review Pidgin log store and dbus testsuite
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: logger (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords:
: 26907 (view as bug list)
Depends on:
Blocks: 27271 33546
  Show dependency treegraph
 
Reported: 2011-01-07 10:22 UTC by Cosimo Alfarano
Modified: 2011-01-27 10:14 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Cosimo Alfarano 2011-01-07 10:22:55 UTC
Summary:

It adds TplLogStorePidgin based on Jonny's version.

It also adds tests/dbus/ for tests in needing of a DBus sessions to work (as Pidgin).

I also added a "testmode" property to TplLogStore interface for those LogStore which need to behave differently when used in a test suite or outside.
For example, Pidgin logstore (the only one currently using the property).

This property is mainly used when testing the TplLogManager, which instantiates automatically some stores.
By default the XML and Empathy ones look for XDG variables, but that's not the case for libpurple, which always uses $(HOME)/.purple
Comment 1 Nicolas Dufresne 2011-01-10 09:56:42 UTC
> +++ b/telepathy-logger/log-store-pidgin-internal.h
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (C) 2008-2009 Collabora Ltd.
> + *

We are in 2011, same in .c file.

>diff --git a/telepathy-logger/log-store-pidgin.c b/telepathy-logger/log-store->pidgin.c
>+static gchar *
>+log_store_pidgin_get_dir (EmpathyLogStore *self,
>+  /* The escaping behaviour here was taken from Pidgin, obviously. */

This escaping is defined in RFC3986 (http://www.ietf.org/rfc/rfc3986.txt) and is implemented in GLib, see g_uri_escape_string/g_uri_unescape_string. There is also helpers for filesname to/from URI.

>+static GList *
>+log_store_pidgin_get_dates (EmpathyLogStore *self,
>+  if (directory == NULL)
>+    {
>+      g_free (directory);
>+      return NULL;
>+    }
No need to free directory, it's NULL (remove scope after)

Ok, I'll stop there for now. I see bunch of stuff that should be rebased/squashed before asking for review. Please cleanup your patchset so every changes mean somtehing. We don't need to know about all your intermediate steps, especially for new files.
Comment 2 Cosimo Alfarano 2011-01-10 10:58:28 UTC
Applied just the two first fixes, I'll apply the escaping suggestion ASAP.

URL changed to another branch, rebased, stashed, etc: http://git.collabora.co.uk/?p=user/kalfa/telepathy-logger.git;a=shortlog;h=refs/heads/pidgin_store_allinone

That should be suitable.
Comment 3 Nicolas Dufresne 2011-01-10 14:13:59 UTC
>diff --git a/telepathy-logger/log-store-pidgin.c b/telepathy-logger/log-store-pidgin.c
>+static void
>+tpl_log_store_pidgin_dispose (GObject *self)
>+{
>+  TplLogStorePidginPriv *priv = TPL_LOG_STORE_PIDGIN (self)->priv;
>+
>+  if (priv->basedir != NULL)
>+    {
>+      g_free (priv->basedir);
>+      priv->basedir = NULL;
>+    }
>+
>+  if (priv->name != NULL)
>+    {
>+      g_free (priv->name);
>+      priv->name = NULL;
>+    }

name and basedir are string, you don't need to do NULL check here, just free them.

>+/* returns am absolute path for the base directory of LogStore */

"am => an" and maybe "for the LogStore's base directory" ?

>+  /* set default based on name if NULL, see prop's comment about it in
>+   * class_init method */
I think I understand this comment but it would benefit some work.

One general style note, the number of white lines between function does not seem constant, I think it should be two.

>diff --git a/telepathy-logger/log-store-pidgin.c b/telepathy-logger/log-store->pidgin.c
>+static gchar *
>+log_store_pidgin_get_dir (EmpathyLogStore *self,
>+  /* The escaping behaviour here was taken from Pidgin, obviously. */

I already mentionned this one, just to make sure you don't forget to use glib utility instead.

>+static GDate *
>+log_store_pidgin_get_time (const gchar *filename)
>+  dir = g_dir_open (directory, 0, NULL);
>+  if (dir == NULL)
>+    {
>+      DEBUG ("Could not open directory:'%s'", directory);
>+      return NULL;
>+    }
You need to free 'directory' inside the if. Also same thing in log_store_pidgin_get_dates().

>+          /* FIXME do we need to set the entity type as well? */
>+
>+          /* FIXME a better way to obtain a log-id from purlple? */
>+          if (TRUE)
>+            {
Could you elaborate more about those, also I don't like so much this runtime if (TRUE).

>+              TPL_ENTRY_DIRECTION_NONE);
Can't we get the direction here ?

>diff --git a/tests/dbus/test-tpl-log-store-pidgin.c b/tests/dbus/test-tpl-log-store-pidgin.c
>+++ b/tests/dbus/test-tpl-log-store-pidgin.c
>@@ -0,0 +1,576 @@
>+/* FIXME: hugly kludge: we need to include all the declarations which are used
>+ * by the GInterface and thus not in the -internal.h */
>+#include "../../telepathy-logger/log-store-pidgin.c"

It's really hugly, anything we could do about that ?

Don't forget to rebase about Emilio's refactoring.
Comment 4 Emilio Pozuelo Monfort 2011-01-11 05:12:42 UTC
*** Bug 26907 has been marked as a duplicate of this bug. ***
Comment 5 Cosimo Alfarano 2011-01-14 10:04:59 UTC
> >+static GDate *
> >+log_store_pidgin_get_time (const gchar *filename)
> >+  dir = g_dir_open (directory, 0, NULL);
> >+  if (dir == NULL)
> >+    {
> >+      DEBUG ("Could not open directory:'%s'", directory);
> >+      return NULL;
> >+    }

I see this problem only in get_dates (fixed).

> You need to free 'directory' inside the if. Also same thing in
> log_store_pidgin_get_dates().
> 
> >+          /* FIXME do we need to set the entity type as well? */
> >+
> >+          /* FIXME a better way to obtain a log-id from purlple? */
> >+          if (TRUE)
> >+            {
> Could you elaborate more about those, also I don't like so much this runtime if
> (TRUE).
> 
> >+              TPL_ENTRY_DIRECTION_NONE);
> Can't we get the direction here ?

Not 100% accurate, direction is anyway useless and will be removed, but so far I'll set it only if is_html==TRUE, which means I can deduct the direction from the logs. Otherwise leave it to NONE.

The same is valid for tpl_entity_set_entity_type(), with the difference that, when is_html==FALSE, it's not possible to understand if the log is actually being send by SELF or not.
This for example will result in Empathy wrongly colouring the log window as it won't understand the user's log. 

> >diff --git a/tests/dbus/test-tpl-log-store-pidgin.c b/tests/dbus/test-tpl-log-store-pidgin.c
> >+++ b/tests/dbus/test-tpl-log-store-pidgin.c
> >@@ -0,0 +1,576 @@
> >+/* FIXME: hugly kludge: we need to include all the declarations which are used
> >+ * by the GInterface and thus not in the -internal.h */
> >+#include "../../telepathy-logger/log-store-pidgin.c"
> 
> It's really hugly, anything we could do about that ?

The way to fix it would be export symbols in a .h just for testing uses.

tp-glib has some testes that use this terrible hack, it's from which I copied it actually. It does not make it any better but I think that it can go with it.

The only side effect is that DEBUG() won't work properly with TPL_DEBUG=testsuite as DEBUG_FLAG has been set in the included file.
Comment 6 Emilio Pozuelo Monfort 2011-01-14 10:09:37 UTC
(In reply to comment #5)
> > >diff --git a/tests/dbus/test-tpl-log-store-pidgin.c b/tests/dbus/test-tpl-log-store-pidgin.c
> > >+++ b/tests/dbus/test-tpl-log-store-pidgin.c
> > >@@ -0,0 +1,576 @@
> > >+/* FIXME: hugly kludge: we need to include all the declarations which are used
> > >+ * by the GInterface and thus not in the -internal.h */
> > >+#include "../../telepathy-logger/log-store-pidgin.c"
> > 
> > It's really hugly, anything we could do about that ?
> 
> The way to fix it would be export symbols in a .h just for testing uses.

JFTR, when I did the log manager refactoring I noticed some symbols where in -internal.h headers just for the tests. It probably doesn't hurt to do that...
Comment 7 Nicolas Dufresne 2011-01-14 11:30:30 UTC
Ship it !
Comment 8 Nicolas Dufresne 2011-01-20 13:47:36 UTC
As the tree has changed significantly I've rebased/fixed everything. I was mainly interested in the tests with real data. So far the new test won't pass and doing this work showed me couple of things I don't like:

- Test should not output debugging by default
- Test should run with valgrind without leaks
- the dbus directoy is annoying since it's Makefile contains bunch of duplicates, I suggest using a DBUS_TEST_EXETRUABLE variable instead
- Tests should always run through the public API (yes the _aynsc/_finish)
- We should probably post-fix test program with -test instead of prefixing them
- etc.

http://git.collabora.co.uk/?p=user/nicolas/telepathy-logger.git;a=shortlog;h=refs/heads/pidgin-log-store
Comment 9 Nicolas Dufresne 2011-01-21 14:23:10 UTC
(In reply to comment #8)
> As the tree has changed significantly I've rebased/fixed everything. I was
> mainly interested in the tests with real data. So far the new test won't pass
> and doing this work showed me couple of things I don't like:
> 
> - Test should not output debugging by default
> - Test should run with valgrind without leaks
> - the dbus directoy is annoying since it's Makefile contains bunch of
> duplicates, I suggest using a DBUS_TEST_EXETRUABLE variable instead
> - Tests should always run through the public API (yes the _aynsc/_finish)
> - We should probably post-fix test program with -test instead of prefixing them
> - etc.

I updated the branch, skipped couple of listed items since they can be carried later. I decided to keep the dbus folder since it's simplier.
Comment 10 Cosimo Alfarano 2011-01-27 08:10:26 UTC
ship it
Comment 11 Cosimo Alfarano 2011-01-27 08:15:05 UTC
sidenote: it was hard to see what was my bits and what yours in the logstore itself, but they seem to be ok.

Just remember (you or Emilio) to fix the use the SEARCH flag as flags, before Empathy uses them
Comment 12 Nicolas Dufresne 2011-01-27 10:14:12 UTC
Pushed.


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.