Issue 395 - Filter hits statistics backend

sitescripts/filterhits/test/test_helpers.py

 # coding: utf-8
 
 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2015 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus 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 Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import tempfile
 import shutil
 import unittest
 
 import MySQLdb
 
 from sitescripts.filterhits import db
 from sitescripts.utils import get_config
 
 class FilterhitsTestCase(unittest.TestCase):
   config = get_config()
+  _db = None
   _live_config = {
     "dbuser": config.get("filterhitstats", "dbuser"),
     "dbpassword": config.get("filterhitstats", "dbpassword"),
     "database": config.get("filterhitstats", "database"),
     "log_dir": config.get("filterhitstats", "log_dir")
   }
   _test_config = {
     "dbuser": config.get("filterhitstats", "test_dbuser"),
     "dbpassword": config.get("filterhitstats", "test_dbpassword"),
     "database": config.get("filterhitstats", "test_database")
   }
 
   def _clear_database(self):
-    db.write(self.db, (("DELETE FROM frequencies",), ("DELETE FROM filters",)))
+    db.write(self._db, (("DELETE FROM frequencies",), ("DELETE FROM filters",)))
+
+  @property
+  def db(self):
+    if (not self._db):
+      self._db = db.connect()
+      self._clear_database()
+    return self._db
 
   def setUp(self):
+    # Set up a temporary log directory for testing
+    self.test_dir = tempfile.mkdtemp()
     # Set up test config

[Sebastian Noack, 2015/04/02 07:37:04]

There is a lot of boilerplate and duplication in the code mocking and restoring
the config. How about this?

def setUp(self):
  self._backup = []

  for option in ("dbuser", "dbpassword", "database"):
    self._backup[option] = config.get("filterhitstats", option)
    config.set("filterhitstats", option, config.get("filterhitstats", "test_" +
option))

  self._backup["log_dir"] = config.get("filterhitstats", "log_dir")
  self._test_dir = tempfile.mkdtemp()
  config.set("filterhitstats", "log_dir", self._test_dir)

  ...

def tearDown(self):
  ...

  for option, value in self._backup.iteritems():
    self._config.set("filterhitstats", option, value)

[kzar, 2015/04/02 07:47:47]

I prefer it as it is, although it doesn't matter much either way. (I suppose one
advantage to my version is that the configuration values are read less often.)

[Sebastian Noack, 2015/04/02 08:11:44]

We don't have to care about performance here. Moreover, this is actually an
advantage. The test should backup the application's state before it runs,
restoring it when it's done. Retrieving the config already on module
intilization might cause problems if the config get's mocked by other parts of
the application. More a theoretical scenario here, since as soon as the module
is loaded the test runs, but still. However, the more important point here is
that duplication and unneeded boilerplate isn't great.

[kzar, 2015/04/02 10:16:54]

Fair enough, well how about this? I've reduced duplication like you've suggested
while keeping the style to my taste.

-    self.config.set("filterhitstats", "database", self._test_config["database"])
-    self.config.set("filterhitstats", "dbuser", self._test_config["dbuser"])
-    self.config.set("filterhitstats", "dbpassword", self._test_config["dbpassword"])
-    self.test_dir = tempfile.mkdtemp()
+    for k, v in self._test_config.items():
+      self.config.set("filterhitstats", k, v)
     self.config.set("filterhitstats", "log_dir", self.test_dir)
-    # Attempt to set up a clean test database
-    try:
-      self.db = db.connect()
-      self._clear_database()
-    except MySQLdb.Error:
-      self.db = None

[Sebastian Noack, 2015/04/02 07:37:04]

Wouldn't it be better to fail here with the actual MySQLdb.Error, rather than
with a meaningless TypeError during the test?

[kzar, 2015/04/02 07:47:47]

The tests that require database access first check that self.db is truthy, if
not they skip. This way we can still run the other tests without a test database
set up.

[Sebastian Noack, 2015/04/02 08:11:44]

How about using a getter for the db property?

@property
def db(self):
  if (not self._db)
    self._db = db.connect()
    self._clear_database()
  return self._db

That way tests that don't need the db won't even try to connect and will still
run if there is a problem with the db. And tests that require the db will fail
with a meaningful exception.

[kzar, 2015/04/02 10:16:54]

Done.

 
   def tearDown(self):
     # Clean the database and close our connection
-    if self.db:
+    if self._db:
       self._clear_database()
-      self.db.close()
-      self.db = None
+      self._db.close()
+      self._db = None
     # Clean any generated logs
     shutil.rmtree(self.test_dir, ignore_errors=True)
     # Restore the configuration
-    self.config.set("filterhitstats", "database", self._live_config["database"])
-    self.config.set("filterhitstats", "dbuser", self._live_config["dbuser"])
-    self.config.set("filterhitstats", "dbpassword", self._live_config["dbpassword"])
-    self.config.set("filterhitstats", "log_dir", self._live_config["log_dir"])
+    for k, v in self._live_config.items():
+      self.config.set("filterhitstats", k, v)