Merge lp:~mandel/ubuntuone-dev-tools/fix-squid-tests into lp:ubuntuone-dev-tools

[Manuel de la Peña]

- Ensured that the ncsa_auth.exe can be found on windows (LP: #987213).
- Formatted the path correctly so that the squid config can read them (LP: #987225).
- Use win32api to kill the squid process (LP: #987256)
- Use dirspec to place the data files correctly on windows (LP: #988228).

- Ensured that the ncsa_auth.exe can be found on windows (LP: #987213).
- Formatted the path correctly so that the squid config can read them (LP: #987225).
- Use win32api to kill the squid process (LP: #987256)
- Use dirspec to place the data files correctly on windows (LP: #988228).

In order to tests the branch you have to follow these steps:

1- Download and copy to C:\squid the windows squid binaries from here: http://squid.acmeconsulting.it/
2- Downlaod the apacache msi and install it: http://httpd.apache.org/download.cgi
3- Ensure the following is in your path: C:\squid\sbin;C:\squid\libexec;C:\Program Files\Apache Software Foundation\Apache2.2\bin

All the above will allow you to run the proxy tests on windows.

Note: The path to the apache bins might differ depending on your system.

[Eric Casteleijn (thisfred)]

98 +if sys.platform == 'win32':
99 + from ubuntuone.devtools.services.squid import windows
100 + source = windows
101 +else:
102 + from ubuntuone.devtools.services.squid import linux
103 + source = linux

why not import windows as source, and import linux as source?

[Eric Casteleijn (thisfred)]

Also, what does "source" mean in this context?

[Eric Casteleijn (thisfred)]

I would suggest something like "squid_util" or "squid_helpers" instead of source.

review: Needs Information

[Eric Casteleijn (thisfred)]

Looks good otherwise, use your own judgement on the above...

review: Approve

[dobey (dobey)]

+# Do use doble \ because squids config needs \ to be escaped
+ """Return the path for the formated for the config."""

Some spelling and grammar fixes need to happen. :)

Also, why do we need to split the Linux and Windows pieces up like this? We need to find a way to run all the tests on all platforms, and just skip the ones that are platform-specific on other platforms.

review: Needs Fixing

[Manuel de la Peña (mandel)]

> +# Do use doble \ because squids config needs \ to be escaped
> + """Return the path for the formated for the config."""
>
> Some spelling and grammar fixes need to happen. :)

Fixing!

>
> Also, why do we need to split the Linux and Windows pieces up like this? We
> need to find a way to run all the tests on all platforms, and just skip the
> ones that are platform-specific on other platforms.

Well this is not splitting the tests according to the platform but working around the fact that squid on windows is not a good citizen. The following is a list of the workarounds we have to do to ensure that we can run all squid tests on windows:

* os.kill does not work on windows, we need to use the win api or we will leave the proxy running.
* The squid config is stupid, it needs paths to be C:\\path\\to\\cache rather than C:\path\to\cache, if you don't do that squid wont start.
* The location of the ncsa_auth is diff.

With that fixed we are able to run all tests as long as squid and htpasswd are present in the path.

[Manuel de la Peña (mandel)]

> I would suggest something like "squid_util" or "squid_helpers" instead of
> source.

Fixing!

[dobey (dobey)]

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

Attempt to merge into lp:ubuntuone-dev-tools failed due to conflicts:

text conflict in run-tests.bat

[lp:~mandel/ubuntuone-dev-tools/fix-squid-tests]

76. By Manuel de la Peña on 2012-04-30

Fixed conflicts issues with trunk.

=== modified file 'data/squid.conf.in'
--- data/squid.conf.in	2012-01-05 16:42:37 +0000
+++ data/squid.conf.in	2012-04-30 11:07:20 +0000
@@ -80,19 +80,19 @@
 cache_dir ufs ${spool_temp} 1000 16 256
 
 # access log settings
-access_log ${squid_temp}/access.log squid
+access_log ${squid_temp}access.log squid
 
 # cache log settings
-cache_log ${squid_temp}/cache.log
+cache_log ${squid_temp}cache.log
 
 # cache store log settings
-cache_store_log ${squid_temp}/store.log
+cache_store_log ${squid_temp}store.log
 
 # mime table conf
 # mime_table /usr/share/squid/mime.conf
 
 #Default pid file name
-pid_filename ${squid_temp}/squid.pid
+pid_filename ${squid_temp}squid.pid
 
 # debug options (Full debugging)
 debug_options ALL,1
@@ -117,4 +117,4 @@
 hosts_file /etc/hosts
 
 # Leave coredumps in the first cache dir
-coredump_dir ${spool_temp}/squid
+coredump_dir ${spool_temp}squid
 
=== modified file 'run-tests'
--- run-tests	2012-01-12 12:08:22 +0000
+++ run-tests	2012-04-30 11:07:20 +0000
@@ -16,8 +16,8 @@
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 set -e
 
-bin/u1trial -c ubuntuone
-bin/u1trial --reactor=twisted ubuntuone
+bin/u1trial -i "test_squid_windows.py" -c ubuntuone
+bin/u1trial --reactor=twisted -i "test_squid_windows.py" ubuntuone
 echo "Running style checks..."
 bin/u1lint
 pep8 --repeat . bin/* --exclude=*.bat
 
=== modified file 'run-tests.bat'
--- run-tests.bat	2012-04-25 12:33:48 +0000
+++ run-tests.bat	2012-04-30 11:07:20 +0000
@@ -28,6 +28,8 @@
 :: files in the program, then also delete it here.
 @ECHO off
 
+SET IGNORED="test_squid_linux.py"
+
 ECHO Checking for Python on the path
 :: Look for Python from buildout
 FOR %%A in (python.exe) do (SET PYTHONEXEPATH=%%~$PATH:A)
@@ -67,7 +69,7 @@
 
 ECHO Python found at %PYTHONEXEPATH%, executing the tests...
 :: execute the tests with a number of ignored linux only modules
-"%PYTHONEXEPATH%" "%TRIALPATH%" --reactor=twisted -c %PARAMS% ubuntuone
+"%PYTHONEXEPATH%" bin/u1trial --reactor=twisted -i %IGNORED% -c %PARAMS% ubuntuone
 
 IF %SKIPLINT% == 1 (
     ECHO Skipping style checks
 
=== modified file 'setup.py'
--- setup.py	2012-03-30 17:44:03 +0000
+++ setup.py	2012-04-30 11:07:20 +0000
@@ -28,10 +28,12 @@
 # files in the program, then also delete it here.
 """setup.py"""
 
+import os
 import subprocess
 import sys
 
 from distutils.core import setup, Command
+from dirspec import basedir
 
 PACKAGE = 'ubuntuone-dev-tools'
 VERSION = '3.1'
@@ -65,6 +67,20 @@
 if sys.platform == 'win32':
     # lets add the .bat so that windows users are happy
     scripts.extend(['bin/u1lint.bat', 'bin/u1trial.bat'])
+    DATA_FILES = [(os.path.join(basedir.default_data_path, PACKAGE),
+                  ['pylintrc',
+                   'data/dbus-session.conf.in',
+                   'data/squid.conf.in']),
+                  ]
+else:
+    DATA_FILES = [('share/%s' % PACKAGE,
+                  ['pylintrc',
+                   'data/dbus-session.conf.in',
+                   'data/squid.conf.in']),
+                  ('share/man/man1',
+                   ['man/u1lint.1',
+                    'man/u1trial.1']),
+                 ]
 
 setup(name=PACKAGE,
       version=VERSION,
@@ -78,15 +94,7 @@
                 'ubuntuone.devtools.testcases'],
       extra_path='ubuntuone-dev-tools',
       scripts=scripts,
-      data_files=[('share/%s' % PACKAGE,
-                   ['pylintrc',
-                    'data/dbus-session.conf.in',
-                    'data/squid.conf.in']),
-                  ('share/man/man1',
-                   ['man/u1lint.1',
-                    'man/u1trial.1']),
-                  ],
-
+      data_files=DATA_FILES,
       cmdclass={
         'lint': Lint,
         },
 
=== modified file 'ubuntuone/devtools/services/squid.py'
--- ubuntuone/devtools/services/squid.py	2012-04-20 17:00:51 +0000
+++ ubuntuone/devtools/services/squid.py	2012-04-30 11:07:20 +0000
@@ -33,10 +33,11 @@
 import string
 # pylint:enable=W0402
 import subprocess
+import sys
 import time
 
 from json import dumps, loads
-from os import environ, makedirs, kill, unlink
+from os import environ, kill, makedirs, unlink
 from os.path import abspath, exists, join
 
 from distutils.spawn import find_executable
@@ -46,24 +47,52 @@
     get_arbitrary_port,
 )
 
+# pylint: disable=C0103
+if sys.platform == 'win32':
+    AUTH_PROCESS_PATH = r'C:\squid\libexec\ncsa_auth.exe'
+    SQUID_START_ARGS = ['-f']
+else:
+    AUTH_PROCESS_PATH = '/usr/lib/%s/ncsa_auth'
+    SQUID_START_ARGS = ['-N', '-X', '-f']
+# pylint: enable=C0103
+
 SQUID_CONFIG_FILE = 'squid.conf.in'
 SQUID_DIR = 'squid'
-AUTH_PROCESS_PATH = '/usr/lib/%s/ncsa_auth'
 SPOOL_DIR = 'spool'
 AUTH_FILE = 'htpasswd'
 PROXY_ENV_VAR = 'SQUID_PROXY_SETTINGS'
 
 
+def format_config_path(path):
+    """Return the path correctly formatted for the config file."""
+    # squid cannot handle correctly paths with a single \
+    return path.replace('\\', '\\\\')
+
+
+def get_auth_process_path(squid_version):
+    """Return the path to the auth executable."""
+    if sys.platform == 'win32':
+        path = find_executable('ncsa_auth')
+        if path is None:
+            path = AUTH_PROCESS_PATH
+        return format_config_path(path)
+    else:
+        squid = 'squid3' if squid_version == 3 else 'squid'
+        path = AUTH_PROCESS_PATH % squid
+        return path
+
+
 def get_squid_executable():
     """Return the squid executable of the system."""
     # try with squid and if not present try with squid3 for newer systems
     # (Ubuntu P). We also return the path to the auth process so that we can
     # point to the correct one.
     squid = find_executable('squid3')
-    auth_process = AUTH_PROCESS_PATH % 'squid3'
+    version = 3
     if squid is None:
+        version = 2
         squid = find_executable('squid')
-        auth_process = AUTH_PROCESS_PATH % 'squid'
+    auth_process = get_auth_process_path(version)
     return squid, auth_process
 
 
@@ -72,6 +101,21 @@
     return find_executable('htpasswd')
 
 
+def kill_squid(squid_pid):
+    """Kill the squid process."""
+    if sys.platform == 'win32':
+        # pylint: disable=F0401
+        import win32api
+        import win32con
+        # pylint: enable=F0401
+
+        handle = win32api.OpenProcess(win32con.PROCESS_TERMINATE, 0, squid_pid)
+        win32api.TerminateProcess(handle, 0)
+        win32api.CloseHandle(handle)
+    else:
+        kill(squid_pid, signal.SIGKILL)
+
+
 def _make_random_string(count):
     """Make a random string of the given length."""
     entropy = random.SystemRandom()
@@ -95,7 +139,7 @@
     path = abspath(path)
     if not exists(path):
         makedirs(path)
-    return path
+    return format_config_path(path)
 
 
 def _get_squid_temp_path(tempdir=''):
@@ -105,7 +149,7 @@
     path = abspath(path)
     if not exists(path):
         makedirs(path)
-    return path
+    return format_config_path(join(path, ''))
 
 
 def _get_auth_temp_path(tempdir=''):
@@ -114,7 +158,7 @@
     auth_file = join(basedir, AUTH_FILE)
     if not exists(basedir):
         makedirs(basedir)
-    return auth_file
+    return format_config_path(auth_file)
 
 
 def store_proxy_settings(settings):
@@ -238,7 +282,8 @@
         self._generate_auth_file(tempdir)
         self._generate_config_file(tempdir)
         self._generate_swap(self.config_file)
-        squid_args = ['-N', '-X', '-f', self.config_file]
+        squid_args = SQUID_START_ARGS
+        squid_args.append(self.config_file)
         sp = subprocess.Popen([self.squid] + squid_args,
                               stdout=subprocess.PIPE,
                               stderr=subprocess.PIPE)
@@ -255,7 +300,7 @@
 
     def stop_service(self):
         """Stop our proxy,"""
-        kill(self.squid_pid, signal.SIGKILL)
+        kill_squid(self.squid_pid)
         delete_proxy_settings()
         self.running = False
         unlink(self.config_file)
 
=== modified file 'ubuntuone/devtools/services/tests/test_squid.py'
--- ubuntuone/devtools/services/tests/test_squid.py	2012-04-20 16:42:54 +0000
+++ ubuntuone/devtools/services/tests/test_squid.py	2012-04-30 11:07:20 +0000
@@ -72,6 +72,7 @@
         self.patch(squid, '_get_basedir', fake_basedir_fn)
         self.patch(squid, 'makedirs', fake_makedirs)
         self.patch(squid, 'exists', fake_exists)
+        self.patch(squid, 'format_config_path', lambda path: path)
 
     def test_get_basedir_missing(self):
         """Test the base dir creation."""
@@ -116,24 +117,26 @@
 
     def test_get_squid_temp_path_missing(self):
         """Test the squid path creation."""
-        expected_path = os.path.join(self.basedir, squid.SQUID_DIR)
+        expected_path = os.path.join(self.basedir, squid.SQUID_DIR, '')
+        abspath = os.path.abspath(expected_path)
         result = squid._get_squid_temp_path()
         self.assertEqual(expected_path, result)
         self.assertTrue(('fake_basedir_fn', '') in self.called)
-        self.assertTrue(('fake_makedirs', expected_path) in self.called)
-        self.assertTrue(expected_path in self.created_paths)
-        self.assertTrue(('fake_exists', expected_path) in self.called)
+        self.assertTrue(('fake_makedirs', abspath) in self.called)
+        self.assertTrue(abspath in self.created_paths)
+        self.assertTrue(('fake_exists', abspath) in self.called)
 
     def test_get_squid_temp_path_present(self):
         """Test the squid path creation."""
         self.path_exists = True
-        expected_path = os.path.join(self.basedir, squid.SQUID_DIR)
+        expected_path = os.path.join(self.basedir, squid.SQUID_DIR, '')
         result = squid._get_squid_temp_path()
         self.assertEqual(expected_path, result)
         self.assertTrue(('fake_basedir_fn', '') in self.called)
         self.assertTrue(('fake_makedirs', expected_path) not in self.called)
         self.assertTrue(expected_path not in self.created_paths)
-        self.assertTrue(('fake_exists', expected_path) in self.called)
+        self.assertTrue(('fake_exists',
+                        os.path.abspath(expected_path)) in self.called)
 
     def test_get_auth_temp_path(self):
         """Test the creation of the auth path."""
 
=== added file 'ubuntuone/devtools/services/tests/test_squid_linux.py'
--- ubuntuone/devtools/services/tests/test_squid_linux.py	1970-01-01 00:00:00 +0000
+++ ubuntuone/devtools/services/tests/test_squid_linux.py	2012-04-30 11:07:20 +0000
@@ -0,0 +1,67 @@
+#
+# Copyright 2012 Canonical Ltd.
+#
+# This program 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.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, 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 this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# In addition, as a special exception, the copyright holders give
+# permission to link the code of portions of this program with the
+# OpenSSL library under certain conditions as described in each
+# individual source file, and distribute linked combinations
+# including the two.
+# You must obey the GNU General Public License in all respects
+# for all of the code used other than OpenSSL.  If you modify
+# file(s) with this exception, you may extend this exception to your
+# version of the file(s), but you are not obligated to do so.  If you
+# do not wish to do so, delete this exception statement from your
+# version.  If you delete this exception statement from all source
+# files in the program, then also delete it here.
+"""Tests for the linux squid bits."""
+
+import signal
+
+from twisted.trial.unittest import TestCase
+
+from ubuntuone.devtools.services import squid
+
+
+class SquidLinuxTestCase(TestCase):
+    """"Test the different linux bits."""
+
+    def test_get_auth_process_3(self):
+        """Test getting the auth process for squid3."""
+        expected = squid.AUTH_PROCESS_PATH % 'squid3'
+        self.assertEqual(expected, squid.get_auth_process_path(3))
+
+    def test_get_auth_process(self):
+        """Test getting the auth process."""
+        expected = squid.AUTH_PROCESS_PATH % 'squid'
+        self.assertEqual(expected, squid.get_auth_process_path(2))
+
+    def test_format_config_path(self):
+        """Test formating a config path."""
+        path = '/a/config/path'
+        self.assertEqual(path, squid.format_config_path(path))
+
+    def test_kill_squid(self):
+        """Test killing squid."""
+        called = []
+
+        def fake_kill(pid, kill_signal):
+            """Fake os.kill."""
+            called.append(('kill', pid, kill_signal))
+
+        self.patch(squid, 'kill', fake_kill)
+
+        squid_pid = 4
+        squid.kill_squid(squid_pid)
+        self.assertIn(('kill', squid_pid, signal.SIGKILL), called)
 
=== added file 'ubuntuone/devtools/services/tests/test_squid_windows.py'
--- ubuntuone/devtools/services/tests/test_squid_windows.py	1970-01-01 00:00:00 +0000
+++ ubuntuone/devtools/services/tests/test_squid_windows.py	2012-04-30 11:07:20 +0000
@@ -0,0 +1,108 @@
+#
+# Copyright 2012 Canonical Ltd.
+#
+# This program 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.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, 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 this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# In addition, as a special exception, the copyright holders give
+# permission to link the code of portions of this program with the
+# OpenSSL library under certain conditions as described in each
+# individual source file, and distribute linked combinations
+# including the two.
+# You must obey the GNU General Public License in all respects
+# for all of the code used other than OpenSSL.  If you modify
+# file(s) with this exception, you may extend this exception to your
+# version of the file(s), but you are not obligated to do so.  If you
+# do not wish to do so, delete this exception statement from your
+# version.  If you delete this exception statement from all source
+# files in the program, then also delete it here.
+"""Tests for the windows squid bits."""
+
+# pylint: disable=F0401
+import win32api
+import win32con
+# pylint: enable=F0401
+
+from twisted.trial.unittest import TestCase
+
+from ubuntuone.devtools.services import squid
+
+
+class SquidWindowsTestCase(TestCase):
+    """"Test the different windows bits."""
+
+    def test_get_auth_process(self):
+        """Test getting the auth process for squid3."""
+        called = []
+
+        self.patch(squid, 'find_executable', lambda _: None)
+
+        def fake_format(path):
+            """Fake format of a config path."""
+            called.append(('format', path))
+            return path
+
+        self.patch(squid, 'format_config_path', fake_format)
+        self.assertEqual(squid.AUTH_PROCESS_PATH,
+                         squid.get_auth_process_path(3))
+        self.assertIn(('format', squid.AUTH_PROCESS_PATH), called)
+
+    def test_get_auth_process_path(self):
+        """Test getting the auth process."""
+        called = []
+
+        exec_path = '/path/to/exec'
+        self.patch(squid, 'find_executable', lambda _: exec_path)
+
+        def fake_format(path):
+            """Fake format of a config path."""
+            called.append(('format', path))
+            return path
+
+        self.patch(squid, 'format_config_path', fake_format)
+        self.assertEqual(exec_path, squid.get_auth_process_path(3))
+        self.assertIn(('format', exec_path), called)
+
+    def test_format_config_path(self):
+        """Test formating a config path."""
+        path = '\\a\\config\\path'
+        expected = path.replace('\\', '\\\\')
+        self.assertEqual(expected, squid.format_config_path(path))
+
+    def test_kill_squid(self):
+        """Test killing squid."""
+        called = []
+
+        def fake_open_process(access, inherit, pid):
+            """A fake open process."""
+            called.append(('OpenProcess', access, inherit, pid))
+            return pid
+
+        self.patch(win32api, 'OpenProcess', fake_open_process)
+
+        def fake_terminate(handle, exit_code):
+            """Fake terminate the process."""
+            called.append(('TerminateProcess', handle, exit_code))
+
+        self.patch(win32api, 'TerminateProcess', fake_terminate)
+
+        def fake_close_handle(handle):
+            """Fale closing a handle."""
+            called.append(('CloseHandle', handle))
+
+        self.patch(win32api, 'CloseHandle', fake_close_handle)
+        squid_pid = 4
+        squid.kill_squid(squid_pid)
+        self.assertIn(('OpenProcess', win32con.PROCESS_TERMINATE, 0,
+                      squid_pid), called)
+        self.assertIn(('TerminateProcess', squid_pid, 0), called)
+        self.assertIn(('CloseHandle', squid_pid), called)
 
=== modified file 'ubuntuone/devtools/testcases/tests/test_squid_testcase.py'
--- ubuntuone/devtools/testcases/tests/test_squid_testcase.py	2012-03-30 17:44:03 +0000
+++ ubuntuone/devtools/testcases/tests/test_squid_testcase.py	2012-04-30 11:07:20 +0000
@@ -192,7 +192,7 @@
         """Build the url for this mock server."""
         #pylint: disable=W0212
         port_num = self.tcpserver._port.getHost().port
-        return "http://localhost:%d/" % port_num
+        return "http://127.0.0.1:%d/" % port_num
 
     @defer.inlineCallbacks
     def stop(self):