Add unit testing for the new close_fd functions using mocks

Should get them to complete coverage.

Also move our test dependencies into the standard 'test' extra and
install mock (and futures) only on Python 2.7.

Makefile

@@ -179,7 +179,7 @@ develop:
 # disables the cache.
 # We need wheel>=0.26 on Python 3.5. See previous revisions.
 	time ${PYTHON} -m pip install -U -r ci-requirements.txt
-	GEVENTSETUP_EV_VERIFY=3 time ${PYTHON} -m pip install -U -e .
+	GEVENTSETUP_EV_VERIFY=3 time ${PYTHON} -m pip install -U -e .[test,dnspython,events]
 	${PYTHON} -m pip freeze
 	ccache -s
 	@${PYTHON} scripts/travis.py fold_end install

appveyor.yml

@@ -142,7 +142,7 @@ build_script:
   # into a variable, using python to glob.
   - "%PYEXE% -c \"import glob; print(glob.glob('dist/*whl')[0])\" > whl.txt"
   - set /p PYWHL=<whl.txt
-  - "%PYEXE% -m pip install %PYWHL%"
+  - "%PYEXE% -m pip install %PYWHL%[test,events,dnspython]"
 
 test_script:
   # Run the project tests

ci-requirements.txt

@@ -12,23 +12,8 @@ greenlet>=0.4.13 ; platform_python_implementation == "CPython"
 pylint>=1.8.0
 # pyyaml is included here and doesn't install on travis with 3.7a3
 prospector[with_pyroma] ; python_version < '3.7'
-# We don't run coverage on Windows, and pypy can't build it there
-# anyway (coveralls -> cryptopgraphy -> openssl)
-coverage>=4.0 ; sys_platform != 'win32'
-coveralls>=1.0 ; sys_platform != 'win32'
 # See version requirements in setup.py
 cffi >= 1.11.5 ; platform_python_implementation == "CPython"
-futures
-dnspython
-idna
-# Makes tests faster
-# Fails to build on PyPy on Windows.
-psutil ; platform_python_implementation == "CPython" or sys_platform != "win32"
+
 # benchmarks use this
 perf
-# examples, called from tests, use this
-requests
-
-# Events
-zope.event
-zope.interface

dev-requirements.txt

@@ -4,5 +4,4 @@
 restview
 
 -r ci-requirements.txt
--r rtd-requirements.txt
--e .
+-e .[test,events,dnspython,doc]

setup.py

@@ -327,6 +327,27 @@ def run_setup(ext_modules, run_make):
                 'zope.event',
                 'zope.interface',
             ],
+            'doc': [
+                'repoze.sphinx.autointerface',
+            ],
+            'test': [
+                'zope.interface',
+                'zope.event',
+
+                # Makes tests faster
+                # Fails to build on PyPy on Windows.
+                'psutil ; platform_python_implementation == "CPython" or sys_platform != "win32"',
+                # examples, called from tests, use this
+                'requests',
+
+                # We don't run coverage on Windows, and pypy can't build it there
+                # anyway (coveralls -> cryptopgraphy -> openssl)
+                'coverage>=4.0 ; sys_platform != "win32"',
+                'coveralls>=1.0 ; sys_platform != "win32"',
+
+                'futures ; python_version == "2.7"',
+                'mock ; python_version == "2.7"',
+            ]
         },
         # It's always safe to pass the CFFI keyword, even if
         # cffi is not installed: it's just ignored in that case.

src/gevent/subprocess.py

@@ -1151,27 +1151,31 @@ class Popen(object):
             self._set_cloexec_flag(w)
             return r, w
 
-        def _close_fds(self, keep, errpipe_write):
+        _POSSIBLE_FD_DIRS = (
+            '/proc/self/fd', # Linux
+            '/dev/fd', # BSD, including macOS
+        )
+
+        @classmethod
+        def _close_fds(cls, keep, errpipe_write):
             # From the C code:
             # errpipe_write is part of keep. It must be closed at
             # exec(), but kept open in the child process until exec() is
             # called.
+            for path in cls._POSSIBLE_FD_DIRS:
+                if os.path.isdir(path):
+                    return cls._close_fds_from_path(path, keep, errpipe_write)
+            return cls._close_fds_brute_force(keep, errpipe_write)
 
-            if os.path.isdir('/proc/self/fd'): # Linux
-                self._close_fds_from_path('/proc/self/fd', keep, errpipe_write)
-            elif os.path.isdir('/dev/fd'): # BSD, including macOS
-                self._close_fds_from_path('/dev/fd', keep, errpipe_write)
-            else:
-                self._close_fds_brute_force(keep, errpipe_write)
-
-        def _close_fds_from_path(self, path, keep, errpipe_write):
+        @classmethod
+        def _close_fds_from_path(cls, path, keep, errpipe_write):
             # path names a directory whose only entries have
             # names that are ascii strings of integers in base10,
             # corresponding to the fds the current process has open
             try:
                 fds = [int(fname) for fname in os.listdir(path)]
             except (ValueError, OSError):
-                self._close_fds_brute_force(keep, errpipe_write)
+                cls._close_fds_brute_force(keep, errpipe_write)
             else:
                 for i in keep:
                     if i == errpipe_write:
@@ -1186,7 +1190,8 @@ class Popen(object):
                     except:
                         pass
 
-        def _close_fds_brute_force(self, keep, errpipe_write):
+        @classmethod
+        def _close_fds_brute_force(cls, keep, errpipe_write):
             # `keep` is a set of fds, so we
             # use os.closerange from 3 to min(keep)
             # and then from max(keep + 1) to MAXFD and

src/greentest/greentest/__init__.py

@@ -119,3 +119,10 @@ BaseTestCase = unittest.TestCase
 
 from greentest.flaky import reraiseFlakyTestTimeout
 from greentest.flaky import reraiseFlakyTestRaceCondition
+
+try:
+    from unittest import mock
+except ImportError: # Python 2
+    import mock
+
+mock = mock

src/greentest/test__subprocess.py

 import sys
 import os
 import errno
+import unittest
+
+import time
+import gc
+import tempfile
+
 import greentest
 import gevent
+from greentest import mock
 from gevent import subprocess
 
 if not hasattr(subprocess, 'mswindows'):
     # PyPy3, native python subprocess
     subprocess.mswindows = False
-import time
-import gc
-import tempfile
 
 
 PYPY = hasattr(sys, 'pypy_version_info')
@@ -236,32 +240,31 @@ class Test(greentest.TestCase):
         r = p.stdout.readline()
         self.assertEqual(r, b'foobar\n')
 
-    if sys.platform != 'win32':
+    @greentest.ignores_leakcheck
+    @greentest.skipOnWindows("Not sure why?")
+    def test_subprocess_in_native_thread(self):
+        # gevent.subprocess doesn't work from a background
+        # native thread. See #688
+        from gevent import monkey
 
-        def test_subprocess_in_native_thread(self):
-            # gevent.subprocess doesn't work from a background
-            # native thread. See #688
-            from gevent import monkey
+        # must be a native thread; defend against monkey-patching
+        ex = []
+        Thread = monkey.get_original('threading', 'Thread')
 
-            # must be a native thread; defend against monkey-patching
-            ex = []
-            Thread = monkey.get_original('threading', 'Thread')
+        def fn():
+            with self.assertRaises(TypeError) as exc:
+                gevent.subprocess.Popen('echo 123', shell=True)
+                raise AssertionError("Should not be able to construct Popen")
+            ex.append(exc.exception)
 
-            def fn():
-                with self.assertRaises(TypeError) as exc:
-                    gevent.subprocess.Popen('echo 123', shell=True)
-                    raise AssertionError("Should not be able to construct Popen")
-                ex.append(exc.exception)
+        thread = Thread(target=fn)
+        thread.start()
+        thread.join()
 
-            thread = Thread(target=fn)
-            thread.start()
-            thread.join()
+        self.assertEqual(len(ex), 1)
+        self.assertTrue(isinstance(ex[0], TypeError), ex)
+        self.assertEqual(ex[0].args[0], 'child watchers are only available on the default loop')
 
-            self.assertEqual(len(ex), 1)
-            self.assertTrue(isinstance(ex[0], TypeError), ex)
-            self.assertEqual(ex[0].args[0], 'child watchers are only available on the default loop')
-
-        test_subprocess_in_native_thread.ignore_leakcheck = True
 
     @greentest.skipOnLibuvOnPyPyOnWin("hangs")
     def __test_no_output(self, kwargs, kind):
@@ -295,6 +298,91 @@ class Test(greentest.TestCase):
         # https://github.com/gevent/gevent/pull/939
         self.__test_no_output({}, bytes)
 
+@greentest.skipOnWindows("Testing POSIX fd closing")
+class TestFDs(unittest.TestCase):
+
+    @mock.patch('os.closerange')
+    @mock.patch('gevent.subprocess._set_inheritable')
+    @mock.patch('os.close')
+    def test_close_fds_brute_force(self, close, set_inheritable, closerange):
+        keep = (
+            4, 5,
+            # Leave a hole
+            # 6,
+            7,
+        )
+        subprocess.Popen._close_fds_brute_force(keep, None)
+
+        closerange.assert_has_calls([
+            mock.call(3, 4),
+            mock.call(8, subprocess.MAXFD),
+        ])
+
+        set_inheritable.assert_has_calls([
+            mock.call(4, True),
+            mock.call(5, True),
+        ])
+
+        close.assert_called_once_with(6)
+
+    @mock.patch('gevent.subprocess.Popen._close_fds_brute_force')
+    @mock.patch('os.listdir')
+    def test_close_fds_from_path_bad_values(self, listdir, brute_force):
+        listdir.return_value = 'Not an Integer'
+
+        subprocess.Popen._close_fds_from_path('path', [], 42)
+        brute_force.assert_called_once_with([], 42)
+
+    @mock.patch('os.listdir')
+    @mock.patch('os.closerange')
+    @mock.patch('gevent.subprocess._set_inheritable')
+    @mock.patch('os.close')
+    def test_close_fds_from_path(self, close, set_inheritable, closerange, listdir):
+        keep = (
+            4, 5,
+            # Leave a hole
+            # 6,
+            7,
+        )
+        listdir.return_value = ['1', '6', '37']
+
+        subprocess.Popen._close_fds_from_path('path', keep, 5)
+
+        self.assertEqual([], closerange.mock_calls)
+
+        set_inheritable.assert_has_calls([
+            mock.call(4, True),
+            mock.call(7, True),
+        ])
+
+        close.assert_has_calls([
+            mock.call(6),
+            mock.call(37),
+        ])
+
+    @mock.patch('gevent.subprocess.Popen._close_fds_brute_force')
+    @mock.patch('os.path.isdir')
+    def test_close_fds_no_dir(self, isdir, brute_force):
+        isdir.return_value = False
+
+        subprocess.Popen._close_fds([], 42)
+        brute_force.assert_called_once_with([], 42)
+        isdir.assert_has_calls([
+            mock.call('/proc/self/fd'),
+            mock.call('/dev/fd'),
+        ])
+
+    @mock.patch('gevent.subprocess.Popen._close_fds_from_path')
+    @mock.patch('gevent.subprocess.Popen._close_fds_brute_force')
+    @mock.patch('os.path.isdir')
+    def test_close_fds_with_dir(self, isdir, brute_force, from_path):
+        isdir.return_value = True
+
+        subprocess.Popen._close_fds([7], 42)
+
+        self.assertEqual([], brute_force.mock_calls)
+        from_path.assert_called_once_with('/proc/self/fd', [7], 42)
+
 class RunFuncTestCase(greentest.TestCase):
     # Based on code from python 3.6