Patchwork [2,of,2] run-tests: use context managers for file descriptors

login
register
mail settings
Submitter Matt Harbison
Date Dec. 17, 2017, 10:25 p.m.
Message ID <f59e9334c838da4834a0.1513549549@Envy>
Download mbox | patch
Permalink /patch/26332/
State Accepted
Headers show

Comments

Matt Harbison - Dec. 17, 2017, 10:25 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1513537609 18000
#      Sun Dec 17 14:06:49 2017 -0500
# Node ID f59e9334c838da4834a043d109ddfad1f7bfa8ea
# Parent  5185939f98c5c5e4383eebfa9694a522e6279648
run-tests: use context managers for file descriptors

I've seen the following error a few times recently when running the tests with
`yes | ./run-tests.py --local -j9 -i`:

Errored test-add.t: Traceback (most recent call last):
  File "./run-tests.py", line 821, in run
    self.runTest()
  File "./run-tests.py", line 910, in runTest
    if self._result.addOutputMismatch(self, ret, out, self._refout):
  File "./run-tests.py", line 1774, in addOutputMismatch
    rename(test.errpath, test.path)
  File "./run-tests.py", line 571, in rename
    os.remove(src)
WindowsError: [Error 32] The process cannot access the file because it is being
 used by another process: 'c:\\Users\\Matt\\projects\\hg\\tests\\test-add.t.err'

This change doesn't fix the problem, but it seems like a simple enough
improvement.
Boris Feld - Dec. 18, 2017, 9:44 a.m.
The patches looks good to me, but I'm not an expert in the Windows
world.

On Sun, 2017-12-17 at 17:25 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1513537609 18000
> #      Sun Dec 17 14:06:49 2017 -0500
> # Node ID f59e9334c838da4834a043d109ddfad1f7bfa8ea
> # Parent  5185939f98c5c5e4383eebfa9694a522e6279648
> run-tests: use context managers for file descriptors
> 
> I've seen the following error a few times recently when running the
> tests with
> `yes | ./run-tests.py --local -j9 -i`:
> 
> Errored test-add.t: Traceback (most recent call last):
>   File "./run-tests.py", line 821, in run
>     self.runTest()
>   File "./run-tests.py", line 910, in runTest
>     if self._result.addOutputMismatch(self, ret, out, self._refout):
>   File "./run-tests.py", line 1774, in addOutputMismatch
>     rename(test.errpath, test.path)
>   File "./run-tests.py", line 571, in rename
>     os.remove(src)
> WindowsError: [Error 32] The process cannot access the file because
> it is being
>  used by another process:
> 'c:\\Users\\Matt\\projects\\hg\\tests\\test-add.t.err'
> 
> This change doesn't fix the problem, but it seems like a simple
> enough
> improvement.
> 
> diff --git a/tests/run-tests.py b/tests/run-tests.py
> --- a/tests/run-tests.py
> +++ b/tests/run-tests.py
> @@ -901,10 +901,9 @@
>              # Diff generation may rely on written .err file.
>              if (ret != 0 or out != self._refout) and not
> self._skipped \
>                  and not self._debug:
> -                f = open(self.errpath, 'wb')
> -                for line in out:
> -                    f.write(line)
> -                f.close()
> +                with open(self.errpath, 'wb') as f:
> +                    for line in out:
> +                        f.write(line)
>  
>              # The result object handles diff calculation for us.
>              if self._result.addOutputMismatch(self, ret, out,
> self._refout):
> @@ -941,10 +940,9 @@
>  
>          if (self._ret != 0 or self._out != self._refout) and not
> self._skipped \
>              and not self._debug and self._out:
> -            f = open(self.errpath, 'wb')
> -            for line in self._out:
> -                f.write(line)
> -            f.close()
> +            with open(self.errpath, 'wb') as f:
> +                for line in self._out:
> +                    f.write(line)
>  
>          vlog("# Ret was:", self._ret, '(%s)' % self.name)
>  
> @@ -1087,32 +1085,31 @@
>  
>      def _createhgrc(self, path):
>          """Create an hgrc file for this test."""
> -        hgrc = open(path, 'wb')
> -        hgrc.write(b'[ui]\n')
> -        hgrc.write(b'slash = True\n')
> -        hgrc.write(b'interactive = False\n')
> -        hgrc.write(b'mergemarkers = detailed\n')
> -        hgrc.write(b'promptecho = True\n')
> -        hgrc.write(b'[defaults]\n')
> -        hgrc.write(b'[devel]\n')
> -        hgrc.write(b'all-warnings = true\n')
> -        hgrc.write(b'default-date = 0 0\n')
> -        hgrc.write(b'[largefiles]\n')
> -        hgrc.write(b'usercache = %s\n' %
> -                   (os.path.join(self._testtmp,
> b'.cache/largefiles')))
> -        hgrc.write(b'[lfs]\n')
> -        hgrc.write(b'usercache = %s\n' %
> -                   (os.path.join(self._testtmp, b'.cache/lfs')))
> -        hgrc.write(b'[web]\n')
> -        hgrc.write(b'address = localhost\n')
> -        hgrc.write(b'ipv6 = %s\n' %
> str(self._useipv6).encode('ascii'))
> -
> -        for opt in self._extraconfigopts:
> -            section, key = opt.encode('utf-8').split(b'.', 1)
> -            assert b'=' in key, ('extra config opt %s must '
> -                                 'have an = for assignment' % opt)
> -            hgrc.write(b'[%s]\n%s\n' % (section, key))
> -        hgrc.close()
> +        with open(path, 'wb') as hgrc:
> +            hgrc.write(b'[ui]\n')
> +            hgrc.write(b'slash = True\n')
> +            hgrc.write(b'interactive = False\n')
> +            hgrc.write(b'mergemarkers = detailed\n')
> +            hgrc.write(b'promptecho = True\n')
> +            hgrc.write(b'[defaults]\n')
> +            hgrc.write(b'[devel]\n')
> +            hgrc.write(b'all-warnings = true\n')
> +            hgrc.write(b'default-date = 0 0\n')
> +            hgrc.write(b'[largefiles]\n')
> +            hgrc.write(b'usercache = %s\n' %
> +                       (os.path.join(self._testtmp,
> b'.cache/largefiles')))
> +            hgrc.write(b'[lfs]\n')
> +            hgrc.write(b'usercache = %s\n' %
> +                       (os.path.join(self._testtmp, b'.cache/lfs')))
> +            hgrc.write(b'[web]\n')
> +            hgrc.write(b'address = localhost\n')
> +            hgrc.write(b'ipv6 = %s\n' %
> str(self._useipv6).encode('ascii'))
> +
> +            for opt in self._extraconfigopts:
> +                section, key = opt.encode('utf-8').split(b'.', 1)
> +                assert b'=' in key, ('extra config opt %s must '
> +                                     'have an = for assignment' %
> opt)
> +                hgrc.write(b'[%s]\n%s\n' % (section, key))
>  
>      def fail(self, msg):
>          # unittest differentiates between errored and failed.
> @@ -1232,9 +1229,8 @@
>          return os.path.join(self._testdir, self.bname)
>  
>      def _run(self, env):
> -        f = open(self.path, 'rb')
> -        lines = f.readlines()
> -        f.close()
> +        with open(self.path, 'rb') as f:
> +            lines = f.readlines()
>  
>          # .t file is both reference output and the test input, keep
> reference
>          # output updated with the the test input. This avoids some
> race
> @@ -1246,10 +1242,9 @@
>  
>          # Write out the generated script.
>          fname = b'%s.sh' % self._testtmp
> -        f = open(fname, 'wb')
> -        for l in script:
> -            f.write(l)
> -        f.close()
> +        with open(fname, 'wb') as f:
> +            for l in script:
> +                f.write(l)
>  
>          cmd = b'%s "%s"' % (self._shell, fname)
>          vlog("# Running", cmd)
> @@ -1884,9 +1879,8 @@
>                      continue
>  
>                  if self._keywords:
> -                    f = open(test.path, 'rb')
> -                    t = f.read().lower() + test.bname.lower()
> -                    f.close()
> +                    with open(test.path, 'rb') as f:
> +                        t = f.read().lower() + test.bname.lower()
>                      ignored = False
>                      for k in self._keywords.lower().split():
>                          if k not in t:
> @@ -2822,13 +2816,12 @@
>                      if e.errno != errno.ENOENT:
>                          raise
>          else:
> -            f = open(installerrs, 'rb')
> -            for line in f:
> -                if PYTHON3:
> -                    sys.stdout.buffer.write(line)
> -                else:
> -                    sys.stdout.write(line)
> -            f.close()
> +            with open(installerrs, 'rb') as f:
> +                for line in f:
> +                    if PYTHON3:
> +                        sys.stdout.buffer.write(line)
> +                    else:
> +                        sys.stdout.write(line)
>              sys.exit(1)
>          os.chdir(self._testdir)
>  
> @@ -2836,28 +2829,24 @@
>  
>          if self.options.py3k_warnings and not
> self.options.anycoverage:
>              vlog("# Updating hg command to enable Py3k Warnings
> switch")
> -            f = open(os.path.join(self._bindir, 'hg'), 'rb')
> -            lines = [line.rstrip() for line in f]
> -            lines[0] += ' -3'
> -            f.close()
> -            f = open(os.path.join(self._bindir, 'hg'), 'wb')
> -            for line in lines:
> -                f.write(line + '\n')
> -            f.close()
> +            with open(os.path.join(self._bindir, 'hg'), 'rb') as f:
> +                lines = [line.rstrip() for line in f]
> +                lines[0] += ' -3'
> +            with open(os.path.join(self._bindir, 'hg'), 'wb') as f:
> +                for line in lines:
> +                    f.write(line + '\n')
>  
>          hgbat = os.path.join(self._bindir, b'hg.bat')
>          if os.path.isfile(hgbat):
>              # hg.bat expects to be put in bin/scripts while run-
> tests.py
>              # installation layout put it in bin/ directly. Fix it
> -            f = open(hgbat, 'rb')
> -            data = f.read()
> -            f.close()
> +            with open(hgbat, 'rb') as f:
> +                data = f.read()
>              if b'"%~dp0..\python" "%~dp0hg" %*' in data:
>                  data = data.replace(b'"%~dp0..\python" "%~dp0hg"
> %*',
>                                      b'"%~dp0python" "%~dp0hg" %*')
> -                f = open(hgbat, 'wb')
> -                f.write(data)
> -                f.close()
> +                with open(hgbat, 'wb') as f:
> +                    f.write(data)
>              else:
>                  print('WARNING: cannot fix hg.bat reference to
> python.exe')
>  
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Dec. 18, 2017, 1:05 p.m.
On Mon, 18 Dec 2017 10:44:31 +0100, Boris Feld wrote:
> The patches looks good to me, but I'm not an expert in the Windows
> world.
> 
> On Sun, 2017-12-17 at 17:25 -0500, Matt Harbison wrote:
> > # HG changeset patch
> > # User Matt Harbison <matt_harbison@yahoo.com>
> > # Date 1513537609 18000
> > #      Sun Dec 17 14:06:49 2017 -0500
> > # Node ID f59e9334c838da4834a043d109ddfad1f7bfa8ea
> > # Parent  5185939f98c5c5e4383eebfa9694a522e6279648
> > run-tests: use context managers for file descriptors

Queued, thanks.

> +ispy3 = (sys.version_info[0] >= 3)
> +
> +if ispy3:
> +    osname = os.name.encode('ascii')
> +else:
> +    osname = os.name
> +
> +if osname == 'nt':

Dropped this compat code as 'nt' is a native string.

Patch

diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -901,10 +901,9 @@ 
             # Diff generation may rely on written .err file.
             if (ret != 0 or out != self._refout) and not self._skipped \
                 and not self._debug:
-                f = open(self.errpath, 'wb')
-                for line in out:
-                    f.write(line)
-                f.close()
+                with open(self.errpath, 'wb') as f:
+                    for line in out:
+                        f.write(line)
 
             # The result object handles diff calculation for us.
             if self._result.addOutputMismatch(self, ret, out, self._refout):
@@ -941,10 +940,9 @@ 
 
         if (self._ret != 0 or self._out != self._refout) and not self._skipped \
             and not self._debug and self._out:
-            f = open(self.errpath, 'wb')
-            for line in self._out:
-                f.write(line)
-            f.close()
+            with open(self.errpath, 'wb') as f:
+                for line in self._out:
+                    f.write(line)
 
         vlog("# Ret was:", self._ret, '(%s)' % self.name)
 
@@ -1087,32 +1085,31 @@ 
 
     def _createhgrc(self, path):
         """Create an hgrc file for this test."""
-        hgrc = open(path, 'wb')
-        hgrc.write(b'[ui]\n')
-        hgrc.write(b'slash = True\n')
-        hgrc.write(b'interactive = False\n')
-        hgrc.write(b'mergemarkers = detailed\n')
-        hgrc.write(b'promptecho = True\n')
-        hgrc.write(b'[defaults]\n')
-        hgrc.write(b'[devel]\n')
-        hgrc.write(b'all-warnings = true\n')
-        hgrc.write(b'default-date = 0 0\n')
-        hgrc.write(b'[largefiles]\n')
-        hgrc.write(b'usercache = %s\n' %
-                   (os.path.join(self._testtmp, b'.cache/largefiles')))
-        hgrc.write(b'[lfs]\n')
-        hgrc.write(b'usercache = %s\n' %
-                   (os.path.join(self._testtmp, b'.cache/lfs')))
-        hgrc.write(b'[web]\n')
-        hgrc.write(b'address = localhost\n')
-        hgrc.write(b'ipv6 = %s\n' % str(self._useipv6).encode('ascii'))
-
-        for opt in self._extraconfigopts:
-            section, key = opt.encode('utf-8').split(b'.', 1)
-            assert b'=' in key, ('extra config opt %s must '
-                                 'have an = for assignment' % opt)
-            hgrc.write(b'[%s]\n%s\n' % (section, key))
-        hgrc.close()
+        with open(path, 'wb') as hgrc:
+            hgrc.write(b'[ui]\n')
+            hgrc.write(b'slash = True\n')
+            hgrc.write(b'interactive = False\n')
+            hgrc.write(b'mergemarkers = detailed\n')
+            hgrc.write(b'promptecho = True\n')
+            hgrc.write(b'[defaults]\n')
+            hgrc.write(b'[devel]\n')
+            hgrc.write(b'all-warnings = true\n')
+            hgrc.write(b'default-date = 0 0\n')
+            hgrc.write(b'[largefiles]\n')
+            hgrc.write(b'usercache = %s\n' %
+                       (os.path.join(self._testtmp, b'.cache/largefiles')))
+            hgrc.write(b'[lfs]\n')
+            hgrc.write(b'usercache = %s\n' %
+                       (os.path.join(self._testtmp, b'.cache/lfs')))
+            hgrc.write(b'[web]\n')
+            hgrc.write(b'address = localhost\n')
+            hgrc.write(b'ipv6 = %s\n' % str(self._useipv6).encode('ascii'))
+
+            for opt in self._extraconfigopts:
+                section, key = opt.encode('utf-8').split(b'.', 1)
+                assert b'=' in key, ('extra config opt %s must '
+                                     'have an = for assignment' % opt)
+                hgrc.write(b'[%s]\n%s\n' % (section, key))
 
     def fail(self, msg):
         # unittest differentiates between errored and failed.
@@ -1232,9 +1229,8 @@ 
         return os.path.join(self._testdir, self.bname)
 
     def _run(self, env):
-        f = open(self.path, 'rb')
-        lines = f.readlines()
-        f.close()
+        with open(self.path, 'rb') as f:
+            lines = f.readlines()
 
         # .t file is both reference output and the test input, keep reference
         # output updated with the the test input. This avoids some race
@@ -1246,10 +1242,9 @@ 
 
         # Write out the generated script.
         fname = b'%s.sh' % self._testtmp
-        f = open(fname, 'wb')
-        for l in script:
-            f.write(l)
-        f.close()
+        with open(fname, 'wb') as f:
+            for l in script:
+                f.write(l)
 
         cmd = b'%s "%s"' % (self._shell, fname)
         vlog("# Running", cmd)
@@ -1884,9 +1879,8 @@ 
                     continue
 
                 if self._keywords:
-                    f = open(test.path, 'rb')
-                    t = f.read().lower() + test.bname.lower()
-                    f.close()
+                    with open(test.path, 'rb') as f:
+                        t = f.read().lower() + test.bname.lower()
                     ignored = False
                     for k in self._keywords.lower().split():
                         if k not in t:
@@ -2822,13 +2816,12 @@ 
                     if e.errno != errno.ENOENT:
                         raise
         else:
-            f = open(installerrs, 'rb')
-            for line in f:
-                if PYTHON3:
-                    sys.stdout.buffer.write(line)
-                else:
-                    sys.stdout.write(line)
-            f.close()
+            with open(installerrs, 'rb') as f:
+                for line in f:
+                    if PYTHON3:
+                        sys.stdout.buffer.write(line)
+                    else:
+                        sys.stdout.write(line)
             sys.exit(1)
         os.chdir(self._testdir)
 
@@ -2836,28 +2829,24 @@ 
 
         if self.options.py3k_warnings and not self.options.anycoverage:
             vlog("# Updating hg command to enable Py3k Warnings switch")
-            f = open(os.path.join(self._bindir, 'hg'), 'rb')
-            lines = [line.rstrip() for line in f]
-            lines[0] += ' -3'
-            f.close()
-            f = open(os.path.join(self._bindir, 'hg'), 'wb')
-            for line in lines:
-                f.write(line + '\n')
-            f.close()
+            with open(os.path.join(self._bindir, 'hg'), 'rb') as f:
+                lines = [line.rstrip() for line in f]
+                lines[0] += ' -3'
+            with open(os.path.join(self._bindir, 'hg'), 'wb') as f:
+                for line in lines:
+                    f.write(line + '\n')
 
         hgbat = os.path.join(self._bindir, b'hg.bat')
         if os.path.isfile(hgbat):
             # hg.bat expects to be put in bin/scripts while run-tests.py
             # installation layout put it in bin/ directly. Fix it
-            f = open(hgbat, 'rb')
-            data = f.read()
-            f.close()
+            with open(hgbat, 'rb') as f:
+                data = f.read()
             if b'"%~dp0..\python" "%~dp0hg" %*' in data:
                 data = data.replace(b'"%~dp0..\python" "%~dp0hg" %*',
                                     b'"%~dp0python" "%~dp0hg" %*')
-                f = open(hgbat, 'wb')
-                f.write(data)
-                f.close()
+                with open(hgbat, 'wb') as f:
+                    f.write(data)
             else:
                 print('WARNING: cannot fix hg.bat reference to python.exe')