Re-enable and fix the python tests, add regression tests for the weird PythonQt bugs

src/scripting/python/pythonengine.cpp

@@ -191,7 +191,7 @@ void PythonEngine::AddStringToBuffer(const QString& str,
 
     logging::CreateLogger(logging::Level_Info, buffer_name, -1) <<
         message.toUtf8().constData();
-    manager()->AddLogLine(buffer_name, str, error);
+    manager()->AddLogLine(buffer_name, message, error);
   }
 }
 

src/scripting/python/pythonscript.cpp

@@ -49,6 +49,9 @@ bool PythonScript::Init() {
   PyList_SetItem(__path__, 0, PyString_FromString(info().path().toLocal8Bit().constData()));
   PyModule_AddObject(module_, "__path__", __path__);
 
+  // Set __file__
+  module_.addVariable("__file__", info().script_file());
+
   // Set script object
   module_.addObject("script", interface());
 
@@ -72,7 +75,33 @@ bool PythonScript::Init() {
 }
 
 bool PythonScript::Unload() {
-  module_ = PythonQtObjectPtr();
+  // Remove this module and all its children from sys.modules.  That should be
+  // the only place that references it, so this will clean up the modules'
+  // dict and all globals.
+  PyInterpreterState *interp = PyThreadState_GET()->interp;
+  PyObject* modules = interp->modules;
 
+  QStringList keys_to_delete;
+
+  Py_ssize_t pos = 0;
+  PyObject* key;
+  PyObject* value;
+  while (PyDict_Next(modules, &pos, &key, &value)) {
+    const char* name = PyString_AS_STRING(key);
+    if (PyString_Check(key) && PyModule_Check(value)) {
+      if (QString(name).startsWith(module_name_)) {
+        keys_to_delete << name;
+      }
+    }
+  }
+
+  foreach (const QString& key, keys_to_delete) {
+    // Workaround Python issue 10068 (only affects 2.7.0)
+    _PyModule_Clear(PyDict_GetItemString(modules, key.toAscii().constData()));
+
+    PyDict_DelItemString(modules, key.toAscii().constData());
+  }
+
+  module_ = PythonQtObjectPtr();
   return true;
 }

tests/CMakeLists.txt

@@ -48,14 +48,16 @@ endif(NOT USE_SYSTEM_GMOCK)
 add_definitions(-DGTEST_USE_OWN_TR1_TUPLE=1)
 
 set(TESTUTILS-SOURCES
-    test_utils.cpp
+  mock_networkaccessmanager.cpp
-    mock_networkaccessmanager.cpp
+  mock_taglib.cpp
-    mock_taglib.cpp
+  mock_playlistitem.cpp
-    mock_playlistitem.cpp
+  test_utils.cpp
+  testobjectdecorators.cpp
 )
 
 set(TESTUTILS-MOC-HEADERS
-    mock_networkaccessmanager.h
+  mock_networkaccessmanager.h
+  testobjectdecorators.h
 )
 
 qt4_wrap_cpp(TESTUTILS-SOURCES-MOC ${TESTUTILS-MOC-HEADERS})

tests/python_test.cpp

@@ -26,6 +26,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+#include "testobjectdecorators.h"
 #include "test_utils.h"
 
 #include <QSettings>
@@ -59,32 +60,38 @@ public:
 };
 
 
-class DISABLED_PythonTest : public ::testing::Test {
+class PythonTest : public ::testing::Test {
  protected:
-  void SetUp() {
+  static void SetUpTestCase() {
-    manager_ = new ScriptManager;
+    sManager = new ScriptManager;
-    engine_ = qobject_cast<PythonEngine*>(
+    sEngine = qobject_cast<PythonEngine*>(
-          manager_->EngineForLanguage(ScriptInfo::Language_Python));
+          sManager->EngineForLanguage(ScriptInfo::Language_Python));
+
+    sEngine->EnsureInitialised();
+    PythonQt::self()->addDecorators(new TestObjectDecorators());
   }
 
-  void TearDown() {
+  static void TearDownTestCase() {
-    delete manager_;
+    delete sManager;
   }
 
-  ScriptManager* manager_;
+  static ScriptManager* sManager;
-  PythonEngine* engine_;
+  static PythonEngine* sEngine;
 };
 
+ScriptManager* PythonTest::sManager = NULL;
+PythonEngine* PythonTest::sEngine = NULL;
 
-TEST_F(DISABLED_PythonTest, HasPythonEngine) {
+
-  ASSERT_TRUE(engine_);
+TEST_F(PythonTest, HasPythonEngine) {
+  ASSERT_TRUE(sEngine);
 }
 
-TEST_F(DISABLED_PythonTest, InitFromDirectory) {
+TEST_F(PythonTest, InitFromDirectory) {
   TemporaryScript script("pass");
 
   ScriptInfo info;
-  info.InitFromDirectory(manager_, script.directory_);
+  info.InitFromDirectory(sManager, script.directory_);
 
   EXPECT_TRUE(info.is_valid());
   EXPECT_EQ(script.directory_, info.path());
@@ -92,22 +99,22 @@ TEST_F(DISABLED_PythonTest, InitFromDirectory) {
   EXPECT_EQ(NULL, info.loaded());
 }
 
-TEST_F(DISABLED_PythonTest, StdioIsRedirected) {
+TEST_F(PythonTest, StdioIsRedirected) {
   TemporaryScript script(
         "import sys\n"
         "print 'text on stdout'\n"
         "print >>sys.stderr, 'text on stderr'\n");
   ScriptInfo info;
-  info.InitFromDirectory(manager_, script.directory_);
+  info.InitFromDirectory(sManager, script.directory_);
 
-  engine_->CreateScript(info);
+  sEngine->CreateScript(info);
 
-  QString log = manager_->log_lines_plain().join("\n");
+  QString log = sManager->log_lines_plain().join("\n");
   ASSERT_TRUE(log.contains("text on stdout"));
   ASSERT_TRUE(log.contains("text on stderr"));
 }
 
-TEST_F(DISABLED_PythonTest, CleanupModuleDict) {
+TEST_F(PythonTest, CleanupModuleDict) {
   TemporaryScript script(
         "class Foo:\n"
         "  def __init__(self):\n"
@@ -116,35 +123,89 @@ TEST_F(DISABLED_PythonTest, CleanupModuleDict) {
         "    print 'destructor'\n"
         "f = Foo()\n");
   ScriptInfo info;
-  info.InitFromDirectory(manager_, script.directory_);
+  info.InitFromDirectory(sManager, script.directory_);
 
-  Script* s = engine_->CreateScript(info);
+  Script* s = sEngine->CreateScript(info);
-  ASSERT_TRUE(manager_->log_lines_plain().last().endsWith("constructor"));
+  ASSERT_TRUE(sManager->log_lines_plain().last().endsWith("constructor"));
 
-  engine_->DestroyScript(s);
+  sEngine->DestroyScript(s);
-  ASSERT_TRUE(manager_->log_lines_plain().last().endsWith("destructor"));
+  ASSERT_TRUE(sManager->log_lines_plain().last().endsWith("destructor"));
 }
 
-TEST_F(DISABLED_PythonTest, ModuleConstants) {
+TEST_F(PythonTest, ModuleConstants) {
   TemporaryScript script(
-        "print __builtins__\n"
+        "print type(__builtins__)\n"
         "print __file__\n"
         "print __name__\n"
         "print __package__\n"
         "print __path__\n"
         "print script\n");
   ScriptInfo info;
-  info.InitFromDirectory(manager_, script.directory_);
+  info.InitFromDirectory(sManager, script.directory_);
 
-  engine_->CreateScript(info);
+  sEngine->CreateScript(info);
 
-  const QStringList log = manager_->log_lines_plain();
+  const QStringList log = sManager->log_lines_plain();
   const int n = log.count();
   ASSERT_GE(n, 6);
-  EXPECT_TRUE(log.at(n-6).endsWith("<module '__builtin__' (built-in)>"));     // __builtins__
+  EXPECT_TRUE(log.at(n-6).endsWith("<type 'dict'>"));                  // __builtins__
-  EXPECT_TRUE(log.at(n-5).endsWith(script.directory_ + "/script.py"));        // __file__
+  EXPECT_TRUE(log.at(n-5).endsWith(script.directory_ + "/script.py")); // __file__
-  EXPECT_TRUE(log.at(n-4).endsWith("clementinescripts." + info.id()));        // __name__
+  EXPECT_TRUE(log.at(n-4).endsWith("clementinescripts." + info.id())); // __name__
-  EXPECT_TRUE(log.at(n-3).endsWith("None"));                                  // __package__
+  EXPECT_TRUE(log.at(n-3).endsWith("None"));                           // __package__
-  EXPECT_TRUE(log.at(n-2).endsWith("['" + script.directory_ + "']"));         // __path__
+  EXPECT_TRUE(log.at(n-2).endsWith("['" + script.directory_ + "']"));  // __path__
-  EXPECT_TRUE(log.at(n-1).contains("<clementine.ScriptInterface object at")); // script
+  EXPECT_TRUE(log.at(n-1).contains("ScriptInterface (QObject "));      // script
+}
+
+TEST_F(PythonTest, PythonQtAttrSetWrappedCPP) {
+  // Tests 3rdparty/pythonqt/patches/call-slot-returnvalue.patch
+
+  TemporaryScript script(
+        "import PythonQt.QtGui\n"
+        "PythonQt.QtGui.QStyleOption().version = 123\n"
+        "PythonQt.QtGui.QStyleOption().version = 123\n"
+        "PythonQt.QtGui.QStyleOption().version = 123\n");
+  ScriptInfo info;
+  info.InitFromDirectory(sManager, script.directory_);
+
+  EXPECT_TRUE(sEngine->CreateScript(info));
+}
+
+TEST_F(PythonTest, PythonQtArgumentReferenceCount) {
+  // Tests 3rdparty/pythonqt/patches/argument-reference-count.patch
+
+  TemporaryScript script(
+        "from PythonQt.QtCore import QFile, QObject\n"
+
+        "class Foo(QFile):\n"
+        "  def Init(self, parent):\n"
+        "    QFile.__init__(self, parent)\n"
+
+        "parent = QObject()\n"
+        "Foo().Init(parent)\n"
+        "assert parent\n");
+  ScriptInfo info;
+  info.InitFromDirectory(sManager, script.directory_);
+
+  EXPECT_TRUE(sEngine->CreateScript(info));
+}
+
+TEST_F(PythonTest, PythonQtConversionStack) {
+  // Tests 3rdparty/pythonqt/patches/conversion-stack.patch
+  // This crash is triggered when a C++ thing calls a virtual method on a
+  // python wrapper and that wrapper returns a QString, QByteArray or
+  // QStringList.  In this instance, initStyleOption() calls text() in Foo.
+
+  TemporaryScript script(
+        "from PythonQt.QtGui import QProgressBar, QStyleOptionProgressBar\n"
+
+        "class Foo(QProgressBar):\n"
+        "  def text(self):\n"
+        "    return 'something'\n"
+
+        "for _ in xrange(1000):\n"
+        "  Foo().initStyleOption(QStyleOptionProgressBar())\n");
+  ScriptInfo info;
+  info.InitFromDirectory(sManager, script.directory_);
+
+  EXPECT_TRUE(sEngine->CreateScript(info));
 }

tests/testobjectdecorators.cpp

@@ -0,0 +1,27 @@
+/* This file is part of Clementine.
+   Copyright 2010, David Sansome <me@davidsansome.com>
+
+   Clementine is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation, either version 3 of the License, or
+   (at your option) any later version.
+
+   Clementine is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with Clementine.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#define protected public
+#include <QProgressBar>
+#undef protected
+
+#include "testobjectdecorators.h"
+
+
+void TestObjectDecorators::initStyleOption(QProgressBar* self, QStyleOptionProgressBar* opt) {
+  self->initStyleOption(opt);
+}

tests/testobjectdecorators.h

@@ -0,0 +1,33 @@
+/* This file is part of Clementine.
+   Copyright 2010, David Sansome <me@davidsansome.com>
+
+   Clementine is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation, either version 3 of the License, or
+   (at your option) any later version.
+
+   Clementine is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with Clementine.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#ifndef TESTOBJECTDECORATORS_H
+#define TESTOBJECTDECORATORS_H
+
+#include <QObject>
+
+class QProgressBar;
+class QStyleOptionProgressBar;
+
+class TestObjectDecorators : public QObject {
+  Q_OBJECT
+
+public slots:
+  void initStyleOption(QProgressBar* self, QStyleOptionProgressBar* opt);
+};
+
+#endif // TESTOBJECTDECORATORS_H