Issue 395 - Filter hits statistics backend

sitescripts/filterhits/test/log_tests.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 os
 import shutil
 import time
 import unittest
 
 from sitescripts.filterhits.test import test_helpers
 from sitescripts.filterhits.web import submit
 
-class LogTestCase(unittest.TestCase):
-  longMessage = True
-  maxDiff = None
-
-  def setUp(self):
-    self.config = test_helpers.setup_config()

[Sebastian Noack, 2015/03/31 07:55:21]

Why do you assign this attribute? It doesn't seem to be used in  the other
methods. Do we actually need to return it from setup_config()?

[kzar, 2015/03/31 09:48:56]

Well the reason I've done it this way is that I'm trying to set a convention by
calling the setup_config and resore_config functions in the same way for all the
tests. I think having access to the configuration is generally something useful
for tests to have and I want to avoid the situation where in the future someone
forgets to call the setup_config function for a test and we end up trashing real
data.

It's true we could remove the return value, add an extra include here and get
the configuration ourselves but I think it's cleaner how it is.

[Sebastian Noack, 2015/03/31 10:19:04]

I see, but this is the wrong way doing it then. Instead you should implement a
base class or mixin, implementing the setUp() and tearDown() methods in
test_helpers.py, deriving from it here and for the other tests.

[kzar, 2015/03/31 10:27:47]

I agree, I thought of doing it that way originally. The problem I saw was that I
would still have to call the base class' setup + teardown methods manually[1]
and therefore I don't think the added complexity would actually help us.

[1] - "If you want the setUpClass and tearDownClass on base classes called then
you must call up to them yourself."
https://docs.python.org/2/library/unittest.html#setupclass-and-teardownclass

[Sebastian Noack, 2015/03/31 10:32:23]

Well, this documentation refers to setupClass/tearDownClass, not setUp/tearDown.
Did you actually try to inherit the latter form a base class? I didn't find any
documentation saying that it wouldn't work.

[kzar, 2015/04/01 19:09:39]

You were right, I've done it that way now and it works fine.

Done.

-    self.test_dir = self.config.get("filterhitstats", "log_dir")
-
-  def tearDown(self):
-    test_helpers.restore_config()
-
+class LogTestCase(test_helpers.FilterhitsTestCase):
   def test_log_filterhits(self):
     def list_files(d):
       return filter(os.path.isfile, [os.path.join(d, f) for f in os.listdir(d)])
 
-    todays_date = time.strftime('%Y-%m-%d', time.gmtime())
+    todays_date = time.strftime("%Y-%m-%d", time.gmtime())
     todays_folder = os.path.join(self.test_dir, todays_date)
 
     # The temporary logging directory is created at the start of all tests but
     # we want to test that the directory is created if it doesn't already exist.
     # So we'll delete the directory here and make sure it's re-created later on.
     shutil.rmtree(self.test_dir)
     self.assertEqual(os.path.exists(self.test_dir), False)
 
     log_file = submit.log_filterhits({"some": "thing"}, self.test_dir, "a=1")
-    now = time.strftime('%d/%b/%Y:%H:%M:%S', time.gmtime())
+    now = time.strftime("%d/%b/%Y:%H:%M:%S", time.gmtime())
     self.assertEqual(os.path.exists(self.test_dir), True)
     self.assertEqual(os.path.exists(todays_folder), True)
     self.assertEqual(len(list_files(todays_folder)), 1)
     self.assertEqual(os.path.exists(log_file), True)
-    with open(list_files(todays_folder)[0], 'r') as f:
+    with open(list_files(todays_folder)[0], "r") as f:
       self.assertEqual(f.read(), '[%s] a=1\n{"some": "thing"}' % now)
 
     submit.log_filterhits({"some": "thing"}, self.test_dir, "")
     self.assertEqual(os.path.exists(self.test_dir), True)
     self.assertEqual(os.path.exists(todays_folder), True)
     self.assertEqual(len(list_files(todays_folder)), 2)
 
-if __name__ == '__main__':

[Sebastian Noack, 2015/03/31 07:55:21]

Nit: Consistent whitespaces please.

[Sebastian Noack, 2015/03/31 09:20:18]

I meant quotes of course.

[kzar, 2015/03/31 09:48:56]

Done.

+if __name__ == "__main__":
   unittest.main()