Issue 395 - Filter hits statistics backend

sitescripts/filterhits/test/log_tests.py

Index: sitescripts/filterhits/test/log_tests.py
diff --git a/sitescripts/filterhits/test/log_tests.py b/sitescripts/filterhits/test/log_tests.py
new file mode 100644
index 0000000000000000000000000000000000000000..a73ed7fa33f9686b9b59d12b93416b78abf2252e
--- /dev/null
+++ b/sitescripts/filterhits/test/log_tests.py
@@ -0,0 +1,65 @@
+# 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()
+
+  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_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())
+    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:
+      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.

+  unittest.main()