Merge lp:~alan-griffiths/compiz-core/test-infrastructure-for-plugin into lp:compiz-core/0.9.5

[Alan Griffiths]

Introduce test framework around the plugin loader

[Daniel van Vugt (vanvugt)]

All good, except the INSTALL rule. It's installing lib/libcompiz_plugin.a when you do 'make install', which I don't think is desirable. It looks like the whole INSTALL rule should just be deleted.

review: Needs Fixing

[lp:~alan-griffiths/compiz-core/test-infrastructure-for-plugin]

2942. By Alan Griffiths on 2012-01-25

Kill unnecessary install rule

[Alan Griffiths (alan-griffiths)]

> All good, except the INSTALL rule. It's installing lib/libcompiz_plugin.a when
> you do 'make install', which I don't think is desirable. It looks like the
> whole INSTALL rule should just be deleted.

Oops: copy & paste error. Fixed

[Daniel van Vugt (vanvugt)]

review: Approve

[lp:~alan-griffiths/compiz-core/test-infrastructure-for-plugin]

2943. By Alan Griffiths on 2012-01-25

Fixes to memory management

2944. By Alan Griffiths on 2012-01-25

remove commented out code

[Sam Spilsbury (smspillaz)]

124 + sprintf (plugindir.get(), "%s/%s", home, HOME_PLUGINDIR);

Can you replace that with a std::string and stringstream?

341 +class PluginFilesystem
342 +{
343 +public:
344 + virtual bool
345 + LoadPlugin(CompPlugin *p, const char *path, const char *name) const = 0;
346 +
347 + virtual void
348 + UnloadPlugin(CompPlugin *p) const = 0;
349 +
350 + virtual CompStringList
351 + ListPlugins(const char *path) const = 0;
352 +
353 + static PluginFilesystem const* instance;
354 +
355 +protected:
356 + PluginFilesystem();
357 + virtual ~PluginFilesystem() {}
358 +};

This interface can probably go in plugin.h and be implemented by CompScreen no ? (or at least, replace

CompPlugin::load
CompPlugin::unload
CompPlugin::availablePlugins

by that).

No other plugins use the CompPlugin api (nor should they be allowed to!)

=== modified file 'include/core/action.h'
--- include/core/action.h	2012-01-24 08:24:18 +0000
+++ include/core/action.h	2012-01-25 12:11:35 +0000
@@ -28,7 +28,7 @@
 #ifndef _COMPACTION_H
 #define _COMPACTION_H
 
-#include <core/option.h>
+#include "core/option.h"
 
 #include <boost/function.hpp>
 

=== modified file 'src/CMakeLists.txt'
--- src/CMakeLists.txt	2012-01-24 08:24:18 +0000
+++ src/CMakeLists.txt	2012-01-25 12:11:35 +0000
@@ -11,6 +11,7 @@
 
 IF (COMPIZ_BUILD_TESTING)
 add_subdirectory( wrapsystem/tests )
+add_subdirectory( plugin )
 add_subdirectory( option/tests )
 ENDIF (COMPIZ_BUILD_TESTING)
 

=== added directory 'src/plugin'
=== modified file 'src/plugin.cpp'
--- src/plugin.cpp	2012-01-19 18:12:31 +0000
+++ src/plugin.cpp	2012-01-25 12:11:35 +0000
@@ -26,6 +26,9 @@
 #include "core/plugin.h"
 #include "privatescreen.h"
 
+#include <boost/scoped_array.hpp>
+#include <boost/foreach.hpp>
+
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -34,21 +37,12 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/types.h>
-#include <list>
-
-#include <boost/foreach.hpp>
+
+#include <algorithm>
+#include <set>
+
 #define foreach BOOST_FOREACH
 
-#include "privatescreen.h"
-
-#if defined(HAVE_SCANDIR_POSIX)
-  // POSIX (2008) defines the comparison function like this:
-  #define scandir(a,b,c,d) scandir((a), (b), (c), (int(*)(const dirent **, const dirent **))(d));
-#else
-  #define scandir(a,b,c,d) scandir((a), (b), (c), (int(*)(const void*,const void*))(d));
-#endif
-
-
 
 CompPlugin::Map pluginsMap;
 CompPlugin::List plugins;
@@ -403,17 +397,10 @@
 {
     bool status = true;
 
-    CompPlugin::List::reverse_iterator rit = plugins.rbegin ();
-
-    CompPlugin *p = NULL;
-
-    while (rit != plugins.rend ())
+    for (List::reverse_iterator rit = plugins.rbegin ();
+         rit != plugins.rend (); ++rit)
     {
-	p = (*rit);
-
-	status &= p->vTable->initWindow (w);
-
-	rit++;
+	status &= (*rit)->vTable->initWindow (w);
     }
 
     return status;
@@ -443,47 +430,34 @@
 void
 CompPlugin::unload (CompPlugin *p)
 {
-    (*loaderUnloadPlugin) (p);
+    loaderUnloadPlugin (p);
     delete p;
 }
 
 CompPlugin *
 CompPlugin::load (const char *name)
 {
-    CompPlugin *p;
-    char       *home, *plugindir;
-    bool       status;
-
-    p = new CompPlugin ();
-    if (!p)
-	return 0;
+    std::auto_ptr<CompPlugin>p(new CompPlugin ());
 
     p->devPrivate.uval = 0;
     p->devType	       = "";
     p->vTable	       = 0;
 
-    home = getenv ("HOME");
-    if (home)
+
+    if (char* home = getenv ("HOME"))
     {
-	plugindir = (char *) malloc (strlen (home) + strlen (HOME_PLUGINDIR) + 3);
-	if (plugindir)
-	{
-	    sprintf (plugindir, "%s/%s", home, HOME_PLUGINDIR);
-	    status = (*loaderLoadPlugin) (p, plugindir, name);
-	    free (plugindir);
+        boost::scoped_array<char> plugindir(new char [strlen (home) + strlen (HOME_PLUGINDIR) + 3]);
+        sprintf (plugindir.get(), "%s/%s", home, HOME_PLUGINDIR);
 
-	    if (status)
-		return p;
-	}
+        if (loaderLoadPlugin (p.get(), plugindir.get(), name))
+            return p.release();
     }
 
-    status = (*loaderLoadPlugin) (p, PLUGINDIR, name);
-    if (status)
-	return p;
+    if (loaderLoadPlugin (p.get(), PLUGINDIR, name))
+        return p.release();
 
-    status = (*loaderLoadPlugin) (p, NULL, name);
-    if (status)
-	return p;
+    if (loaderLoadPlugin (p.get(), NULL, name))
+        return p.release();
 
     compLogMessage ("core", CompLogLevelError,
 		    "Couldn't load plugin '%s'", name);
@@ -550,60 +524,29 @@
     return plugins;
 }
 
-static bool
-stringExist (CompStringList &list,
-	     CompString     s)
-{
-    foreach (CompString &l, list)
-	if (s.compare (l) == 0)
-	    return true;
-
-    return false;
-}
-
 CompStringList
 CompPlugin::availablePlugins ()
 {
-    char *home, *plugindir;
-    CompStringList list, currentList, pluginList, homeList;
-
-    home = getenv ("HOME");
-    if (home)
-    {
-	plugindir = (char *) malloc (strlen (home) + strlen (HOME_PLUGINDIR) + 3);
-	if (plugindir)
-	{
-	    sprintf (plugindir, "%s/%s", home, HOME_PLUGINDIR);
-	    homeList = (*loaderListPlugins) (plugindir);
-	    free (plugindir);
-	}
-    }
-
-    pluginList  = (*loaderListPlugins) (PLUGINDIR);
-    currentList = (*loaderListPlugins) (NULL);
-
-    if (!homeList.empty ())
-    {
-	foreach (CompString &s, homeList)
-	    if (!stringExist (list, s))
-		list.push_back (s);
-    }
-
-    if (!pluginList.empty ())
-    {
-	foreach (CompString &s, pluginList)
-	    if (!stringExist (list, s))
-		list.push_back (s);
-    }
-
-    if (!currentList.empty ())
-    {
-	foreach (CompString &s, currentList)
-	    if (!stringExist (list, s))
-		list.push_back (s);
-    }
-
-    return list;
+    CompStringList homeList;
+
+    if (char* home = getenv ("HOME"))
+    {
+        boost::scoped_array<char> plugindir(new char [strlen (home) + strlen (HOME_PLUGINDIR) + 3]);
+        sprintf (plugindir.get(), "%s/%s", home, HOME_PLUGINDIR);
+
+	homeList = loaderListPlugins (plugindir.get());
+    }
+
+    std::set<CompString> set;
+
+    CompStringList pluginList  = loaderListPlugins (PLUGINDIR);
+    CompStringList currentList = loaderListPlugins (0);
+
+    std::copy(homeList.begin(), homeList.end(), std::inserter(set, set.end()));
+    std::copy(pluginList.begin(), pluginList.end(), std::inserter(set, set.end()));
+    std::copy(currentList.begin(), currentList.end(), std::inserter(set, set.end()));
+
+    return CompStringList(set.begin(), set.end());
 }
 
 int

=== added file 'src/plugin/CMakeLists.txt'
--- src/plugin/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ src/plugin/CMakeLists.txt	2012-01-25 12:11:35 +0000
@@ -0,0 +1,48 @@
+include (CompizDefaults)
+include (CompizCommon)
+
+INCLUDE_DIRECTORIES(
+  ${compiz_SOURCE_DIR}/build/generated
+  ${compiz_SOURCE_DIR}/include
+
+  ${compiz_SOURCE_DIR}/src/timer/include
+  ${compiz_SOURCE_DIR}/src/timer/src
+
+  ${compiz_SOURCE_DIR}/src/rect/include
+  ${compiz_SOURCE_DIR}/src/window/geometry/include
+  ${compiz_SOURCE_DIR}/src/window/extents/include
+
+  ${compiz_SOURCE_DIR}/src/pluginclasshandler/include
+
+  ${COMPIZ_INCLUDE_DIRS}
+
+  ${Boost_INCLUDE_DIRS}
+)
+
+SET( PUBLIC_HEADERS )
+SET( PRIVATE_HEADERS )
+
+add_definitions (
+    -DHAVE_CONFIG_H
+    -DPLUGINDIR=\\\"${compiz_plugindir}\\\"
+    -DSHAREDIR=\\\"${compiz_sharedir}\\\"
+    -DMETADATADIR=\\\"${compiz_metadatadir}\\\"
+)
+
+SET( SRCS ${compiz_SOURCE_DIR}/src/plugin.cpp )
+
+ADD_LIBRARY( 
+  compiz_plugin STATIC
+ 		  
+  ${SRCS}
+  
+  ${PUBLIC_HEADERS}
+  ${PRIVATE_HEADERS}
+)
+
+ADD_SUBDIRECTORY( ${CMAKE_CURRENT_SOURCE_DIR}/tests )
+
+SET_TARGET_PROPERTIES(
+  compiz_plugin PROPERTIES
+  PUBLIC_HEADER "${PUBLIC_HEADERS}"
+)

=== added directory 'src/plugin/tests'
=== added file 'src/plugin/tests/CMakeLists.txt'
--- src/plugin/tests/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ src/plugin/tests/CMakeLists.txt	2012-01-25 12:11:35 +0000
@@ -0,0 +1,33 @@
+include_directories( 
+  ${CMAKE_CURRENT_SOURCE_DIR} 
+
+  ${compiz_SOURCE_DIR}/include
+  ${compiz_SOURCE_DIR}/src
+)
+
+add_definitions (
+    -DHAVE_CONFIG_H
+    -DPLUGINDIR=\\\"${compiz_plugindir}\\\"
+    -DSHAREDIR=\\\"${compiz_sharedir}\\\"
+    -DMETADATADIR=\\\"${compiz_metadatadir}\\\"
+)
+
+
+add_executable( 
+  compiz_plugin_test
+  
+  ${CMAKE_CURRENT_SOURCE_DIR}/test-plugin.cpp
+)
+
+target_link_libraries( 
+  compiz_plugin_test 
+  
+  compiz_plugin
+  compiz_core
+  
+  ${GTEST_BOTH_LIBRARIES}
+  ${GMOCK_LIBRARY}
+  ${CMAKE_THREAD_LIBS_INIT} # Link in pthread.
+)
+
+gtest_add_tests( compiz_plugin_test "" ${CMAKE_CURRENT_SOURCE_DIR}/test-plugin.cpp )

=== added file 'src/plugin/tests/test-plugin.cpp'
--- src/plugin/tests/test-plugin.cpp	1970-01-01 00:00:00 +0000
+++ src/plugin/tests/test-plugin.cpp	2012-01-25 12:11:35 +0000
@@ -0,0 +1,294 @@
+#include "core/plugin.h"
+
+// This prevents an instantiation error - not sure why ATM
+#include "core/screen.h"
+
+// Get rid of stupid macro from X.h
+// Why, oh why, are we including X.h?
+#undef None
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+#include <iostream>
+
+namespace {
+
+class PluginFilesystem
+{
+public:
+    virtual bool
+    LoadPlugin(CompPlugin *p, const char *path, const char *name) const = 0;
+
+    virtual void
+    UnloadPlugin(CompPlugin *p) const = 0;
+
+    virtual CompStringList
+    ListPlugins(const char *path) const = 0;
+
+    static PluginFilesystem const* instance;
+
+protected:
+    PluginFilesystem();
+    virtual ~PluginFilesystem() {}
+};
+
+class MockPluginFilesystem : public PluginFilesystem
+{
+public:
+    MOCK_CONST_METHOD3(LoadPlugin, bool (CompPlugin *, const char *, const char *));
+
+    MOCK_CONST_METHOD1(UnloadPlugin, void (CompPlugin *p));
+
+    MOCK_CONST_METHOD1(ListPlugins, CompStringList (const char *path));
+};
+
+class MockVTable : public CompPlugin::VTable
+{
+public:
+    MOCK_METHOD0(init, bool ());
+};
+
+
+bool
+ThunkLoadPluginProc(CompPlugin *p, const char *path_, const char *name)
+{
+    return PluginFilesystem::instance->LoadPlugin(p, path_, name);
+}
+
+void
+ThunkUnloadPluginProc(CompPlugin *p)
+{
+    PluginFilesystem::instance->UnloadPlugin(p);
+}
+
+
+CompStringList
+ThunkListPluginsProc(const char *path)
+{
+    return PluginFilesystem::instance->ListPlugins(path);
+}
+
+PluginFilesystem::PluginFilesystem()
+{
+	::loaderLoadPlugin = ::ThunkLoadPluginProc;
+	::loaderUnloadPlugin = ::ThunkUnloadPluginProc;
+	::loaderListPlugins = ::ThunkListPluginsProc;
+
+	instance = this;
+}
+
+PluginFilesystem const* PluginFilesystem::instance = 0;
+
+} // (abstract) namespace
+
+
+
+TEST(PluginTest, load_non_existant_plugin_must_fail)
+{
+    MockPluginFilesystem mockfs;
+
+    using namespace testing;
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("dummy"))).
+	WillOnce(Return(false));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(PLUGINDIR), StrEq("dummy"))).
+	WillOnce(Return(false));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), Eq((void*)0), StrEq("dummy"))).
+	WillOnce(Return(false));
+
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(AtMost(0));
+    EXPECT_CALL(mockfs, ListPlugins(_)).Times(AtMost(0));
+
+    ASSERT_EQ(0, CompPlugin::load("dummy"));
+}
+
+TEST(PluginTest, load_plugin_from_HOME_PLUGINDIR_succeeds)
+{
+    MockPluginFilesystem mockfs;
+
+    using namespace testing;
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("dummy"))).
+	WillOnce(Return(true));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(PLUGINDIR), StrEq("dummy"))).
+	Times(AtMost(0));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), Eq((void*)0), StrEq("dummy"))).
+	Times(AtMost(0));
+
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1);
+    EXPECT_CALL(mockfs, ListPlugins(_)).Times(AtMost(0));
+
+    CompPlugin* cp = CompPlugin::load("dummy");
+    ASSERT_NE((void*)0, cp);
+
+    CompPlugin::unload(cp);
+}
+
+TEST(PluginTest, load_plugin_from_PLUGINDIR_succeeds)
+{
+    MockPluginFilesystem mockfs;
+
+    using namespace testing;
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("dummy"))).
+	WillOnce(Return(false));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(PLUGINDIR), StrEq("dummy"))).
+	WillOnce(Return(true));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), Eq((void*)0), StrEq("dummy"))).
+	    Times(AtMost(0));;
+
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1);
+    EXPECT_CALL(mockfs, ListPlugins(_)).Times(AtMost(0));
+
+    CompPlugin* cp = CompPlugin::load("dummy");
+    ASSERT_NE((void*)0, cp);
+
+    CompPlugin::unload(cp);
+}
+
+TEST(PluginTest, load_plugin_from_void_succeeds)
+{
+    MockPluginFilesystem mockfs;
+
+    using namespace testing;
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("dummy"))).
+	WillOnce(Return(false));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(PLUGINDIR), StrEq("dummy"))).
+	WillOnce(Return(false));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), Eq((void*)0), StrEq("dummy"))).
+	WillOnce(Return(true));
+
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1);
+    EXPECT_CALL(mockfs, ListPlugins(_)).Times(AtMost(0));
+
+    CompPlugin* cp = CompPlugin::load("dummy");
+    ASSERT_NE((void*)0, cp);
+
+    CompPlugin::unload(cp);
+}
+
+
+TEST(PluginTest, list_plugins_none)
+{
+    MockPluginFilesystem mockfs;
+
+    using namespace testing;
+
+    EXPECT_CALL(mockfs, LoadPlugin(_, _, _)).Times(AtMost(0));
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(AtMost(0));
+
+    EXPECT_CALL(mockfs, ListPlugins(EndsWith(HOME_PLUGINDIR))).
+	WillOnce(Return(CompStringList()));
+
+    EXPECT_CALL(mockfs, ListPlugins(EndsWith(PLUGINDIR))).
+	WillOnce(Return(CompStringList()));
+
+    EXPECT_CALL(mockfs, ListPlugins(Eq((void*)0))).
+	WillOnce(Return(CompStringList()));
+
+    CompStringList cl = CompPlugin::availablePlugins();
+    ASSERT_EQ(0, cl.size());
+}
+
+
+TEST(PluginTest, list_plugins_some)
+{
+    std::string one = "one";
+    std::string two = "two";
+    std::string three = "three";
+
+    CompStringList home(1, one);
+    CompStringList plugin(1, two);
+    CompStringList local(1, three);
+
+    MockPluginFilesystem mockfs;
+
+    using namespace testing;
+
+    EXPECT_CALL(mockfs, LoadPlugin(_, _, _)).Times(AtMost(0));
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(AtMost(0));
+
+    EXPECT_CALL(mockfs, ListPlugins(EndsWith(HOME_PLUGINDIR))).WillOnce(Return(home));
+    EXPECT_CALL(mockfs, ListPlugins(EndsWith(PLUGINDIR))).WillOnce(Return(plugin));
+    EXPECT_CALL(mockfs, ListPlugins(Eq((void*)0))).WillOnce(Return(local));
+
+    CompStringList cl = CompPlugin::availablePlugins();
+    ASSERT_EQ(3, cl.size());
+
+    ASSERT_THAT(cl, Contains(one));
+    ASSERT_THAT(cl, Contains(two));
+    ASSERT_THAT(cl, Contains(three));
+}
+
+TEST(PluginTest, list_plugins_are_deduped)
+{
+    std::string one = "one";
+    std::string two = "two";
+    std::string three = "three";
+
+    CompStringList home(1, one);
+    CompStringList plugin(1, two);
+    CompStringList local(1, three);
+
+    plugin.push_back(one);
+    local.push_back(two);
+
+    MockPluginFilesystem mockfs;
+
+    using namespace testing;
+
+    EXPECT_CALL(mockfs, LoadPlugin(_, _, _)).Times(AtMost(0));
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(AtMost(0));
+
+    EXPECT_CALL(mockfs, ListPlugins(EndsWith(HOME_PLUGINDIR))).WillOnce(Return(home));
+    EXPECT_CALL(mockfs, ListPlugins(EndsWith(PLUGINDIR))).WillOnce(Return(plugin));
+    EXPECT_CALL(mockfs, ListPlugins(Eq((void*)0))).WillOnce(Return(local));
+
+    CompStringList cl = CompPlugin::availablePlugins();
+    ASSERT_EQ(3, cl.size());
+
+    ASSERT_THAT(cl, Contains(one));
+    ASSERT_THAT(cl, Contains(two));
+    ASSERT_THAT(cl, Contains(three));
+}
+
+
+TEST(PluginTest, when_we_push_plugin_init_is_called)
+{
+    MockPluginFilesystem mockfs;
+
+    using namespace testing;
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("dummy"))).
+	WillOnce(Return(true));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(PLUGINDIR), StrEq("dummy"))).
+	Times(AtMost(0));
+
+    EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), Eq((void*)0), StrEq("dummy"))).
+	Times(AtMost(0));
+
+    EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1);
+    EXPECT_CALL(mockfs, ListPlugins(_)).Times(0);
+
+    MockVTable mockVtable;
+    EXPECT_CALL(mockVtable, init()).WillOnce(Return(true));
+
+    CompPlugin* cp = CompPlugin::load("dummy");
+
+    cp->vTable = &mockVtable;
+
+    CompPlugin::push(cp);
+    ASSERT_EQ(cp, CompPlugin::pop());
+    CompPlugin::unload(cp);
+}