test: clean up test dirs (#1904)

* test: clean up test dirs

Closes #1770. Partially does #1888.

* dvc: close anything opened

* test: finalize git repo explicitly to remove test dir later

This was previously an issue on Windows.

dvc/system.py

@@ -102,22 +102,13 @@ class System(object):
 
         FICLONE = 0x40049409
 
-        s = open(src, "r")
-        d = open(dst, "w+")
-
         try:
-            ret = fcntl.ioctl(d.fileno(), FICLONE, s.fileno())
-        except IOError:
-            s.close()
-            d.close()
-            os.unlink(dst)
-            raise
-
-        s.close()
-        d.close()
-
-        if ret != 0:
-            os.unlink(dst)
+            ret = 255
+            with open(src, "r") as s, open(dst, "w+") as d:
+                ret = fcntl.ioctl(d.fileno(), FICLONE, s.fileno())
+        finally:
+            if ret != 0:
+                os.unlink(dst)
 
         return ret
 

dvc/utils/__init__.py

@@ -119,28 +119,22 @@ def copyfile(src, dest, no_progress_bar=False, name=None):
     name = name if name else os.path.basename(dest)
     total = os.stat(src).st_size
 
-    fsrc = open(src, "rb")
-
     if os.path.isdir(dest):
-        fdest = open(os.path.join(dest, os.path.basename(src)), "wb+")
-    else:
-        fdest = open(dest, "wb+")
+        dest = os.path.join(dest, os.path.basename(src))
 
-    while True:
-        buf = fsrc.read(LOCAL_CHUNK_SIZE)
-        if not buf:
-            break
-        fdest.write(buf)
-        copied += len(buf)
-        if not no_progress_bar:
-            progress.update_target(name, copied, total)
+    with open(src, "rb") as fsrc, open(dest, "wb+") as fdest:
+        while True:
+            buf = fsrc.read(LOCAL_CHUNK_SIZE)
+            if not buf:
+                break
+            fdest.write(buf)
+            copied += len(buf)
+            if not no_progress_bar:
+                progress.update_target(name, copied, total)
 
     if not no_progress_bar:
         progress.finish_target(name)
 
-    fsrc.close()
-    fdest.close()
-
 
 def move(src, dst):
     dst = os.path.abspath(dst)

tests/basic_env.py

@@ -7,6 +7,7 @@ import shutil
 import uuid
 import tempfile
 import logging
+import warnings
 
 from git import Repo
 from git.exc import GitCommandNotFound
@@ -60,7 +61,8 @@ class TestDirFixture(object):
         self._root_dir = os.path.realpath(root_dir)
 
     def _pushd(self, d):
-        self._saved_dir = os.path.realpath(os.curdir)
+        if not hasattr(self, "_saved_dir"):
+            self._saved_dir = os.path.realpath(os.curdir)
         os.chdir(d)
 
     def _popd(self):
@@ -98,6 +100,21 @@ class TestDirFixture(object):
 
     def tearDown(self):
         self._popd()
+        try:
+            shutil.rmtree(self._root_dir)
+        except OSError as exc:
+            # We ignore this under Windows with a warning because it happened
+            # to be really hard to trace all not properly closed files.
+            #
+            # Best guess so far is that gitpython is the culprit:
+            # it opens files and uses __del__ to close them, which can happen
+            # late in current pythons. TestGitFixture and TestDvcFixture try
+            # to close that and it works on most of the tests, but not all.
+            # Repos and thus git repos are created all over the dvc ;)
+            if os.name == "nt" and exc.winerror == 32:
+                warnings.warn("Failed to remove test dir: " + str(exc))
+            else:
+                raise
 
 
 class TestGitFixture(TestDirFixture):
@@ -122,6 +139,9 @@ class TestGitFixture(TestDirFixture):
         self.git.index.add([self.CODE])
         self.git.index.commit("add code")
 
+    def tearDown(self):
+        self.git.close()
+
 
 class TestGitSubmoduleFixture(TestGitFixture):
     def __init__(self, root_dir=None):
@@ -145,6 +165,9 @@ class TestDvcFixture(TestGitFixture):
         self.dvc.scm.commit("init dvc")
         logger.setLevel("DEBUG")
 
+    def tearDown(self):
+        self.dvc.scm.git.close()
+
 
 class TestDvcGitInitializedFixture(TestDvcFixture):
     def __init__(self, root_dir=None):
@@ -196,6 +219,9 @@ class TestDvcDataFileFixture(TestDvcGitInitializedFixture):
         )
         self.git.index.commit("Hello world commit")
 
+        # Return to the dir we started to not confuse parent fixture
+        os.chdir(self._saved_dir)
+
 
 # NOTE: Inheritance order in the classes below is important.
 

tests/conftest.py

@@ -41,13 +41,19 @@ def git(repo_dir):
             continue
         break
 
-    git.index.add([repo_dir.CODE])
-    git.index.commit("add code")
-    return git
+    try:
+        git.index.add([repo_dir.CODE])
+        git.index.commit("add code")
+        yield git
+    finally:
+        git.close()
 
 
 @pytest.fixture
 def dvc(repo_dir, git):
-    dvc = DvcRepo.init(repo_dir._root_dir)
-    dvc.scm.commit("init dvc")
-    return dvc
+    try:
+        dvc = DvcRepo.init(repo_dir._root_dir)
+        dvc.scm.commit("init dvc")
+        yield dvc
+    finally:
+        dvc.scm.git.close()

tests/func/test_checkout.py

@@ -126,9 +126,8 @@ class CheckoutBase(TestDvc):
         self.dvc.scm.commit("adding " + fname)
 
     def read_ignored(self):
-        return list(
-            map(lambda s: s.strip("\n"), open(self.GIT_IGNORE).readlines())
-        )
+        with open(self.GIT_IGNORE) as f:
+            return [s.strip("\n") for s in f.readlines()]
 
     def outs_info(self, stage):
         FileInfo = collections.namedtuple("FileInfo", "path inode")

tests/func/test_import.py

@@ -41,7 +41,8 @@ class TestDefaultOutput(TestDvc):
         ret = main(["import", tmpfile])
         self.assertEqual(ret, 0)
         self.assertTrue(os.path.exists(filename))
-        self.assertEqual(open(filename).read(), "content")
+        with open(filename) as fd:
+            self.assertEqual(fd.read(), "content")
 
 
 class TestFailedImportMessage(TestDvc):

tests/func/test_run.py

@@ -381,7 +381,8 @@ class TestRunUnprotectOutsCopy(TestDvc):
         )
         self.assertEqual(ret, 0)
         self.assertFalse(os.access(self.FOO, os.W_OK))
-        self.assertEqual(open(self.FOO, "r").read(), "foo")
+        with open(self.FOO, "r") as fd:
+            self.assertEqual(fd.read(), "foo")
 
         ret = main(
             [
@@ -399,7 +400,8 @@ class TestRunUnprotectOutsCopy(TestDvc):
         )
         self.assertEqual(ret, 0)
         self.assertFalse(os.access(self.FOO, os.W_OK))
-        self.assertEqual(open(self.FOO, "r").read(), "foo")
+        with open(self.FOO, "r") as fd:
+            self.assertEqual(fd.read(), "foo")
 
 
 class TestRunUnprotectOutsSymlink(TestDvc):
@@ -432,7 +434,8 @@ class TestRunUnprotectOutsSymlink(TestDvc):
         self.assertEqual(ret, 0)
         self.assertFalse(os.access(self.FOO, os.W_OK))
         self.assertTrue(System.is_symlink(self.FOO))
-        self.assertEqual(open(self.FOO, "r").read(), "foo")
+        with open(self.FOO, "r") as fd:
+            self.assertEqual(fd.read(), "foo")
 
         ret = main(
             [
@@ -451,7 +454,8 @@ class TestRunUnprotectOutsSymlink(TestDvc):
         self.assertEqual(ret, 0)
         self.assertFalse(os.access(self.FOO, os.W_OK))
         self.assertTrue(System.is_symlink(self.FOO))
-        self.assertEqual(open(self.FOO, "r").read(), "foo")
+        with open(self.FOO, "r") as fd:
+            self.assertEqual(fd.read(), "foo")
 
 
 class TestRunUnprotectOutsHardlink(TestDvc):
@@ -484,7 +488,8 @@ class TestRunUnprotectOutsHardlink(TestDvc):
         self.assertEqual(ret, 0)
         self.assertFalse(os.access(self.FOO, os.W_OK))
         self.assertTrue(System.is_hardlink(self.FOO))
-        self.assertEqual(open(self.FOO, "r").read(), "foo")
+        with open(self.FOO, "r") as fd:
+            self.assertEqual(fd.read(), "foo")
 
         ret = main(
             [
@@ -503,7 +508,8 @@ class TestRunUnprotectOutsHardlink(TestDvc):
         self.assertEqual(ret, 0)
         self.assertFalse(os.access(self.FOO, os.W_OK))
         self.assertTrue(System.is_hardlink(self.FOO))
-        self.assertEqual(open(self.FOO, "r").read(), "foo")
+        with open(self.FOO, "r") as fd:
+            self.assertEqual(fd.read(), "foo")
 
 
 class TestCmdRunOverwrite(TestDvc):
@@ -603,12 +609,14 @@ class TestCmdRunCliMetrics(TestDvc):
     def test_cached(self):
         ret = main(["run", "-m", "metrics.txt", "echo test > metrics.txt"])
         self.assertEqual(ret, 0)
-        self.assertEqual(open("metrics.txt", "r").read().rstrip(), "test")
+        with open("metrics.txt", "r") as fd:
+            self.assertEqual(fd.read().rstrip(), "test")
 
     def test_not_cached(self):
         ret = main(["run", "-M", "metrics.txt", "echo test > metrics.txt"])
         self.assertEqual(ret, 0)
-        self.assertEqual(open("metrics.txt", "r").read().rstrip(), "test")
+        with open("metrics.txt", "r") as fd:
+            self.assertEqual(fd.read().rstrip(), "test")
 
 
 class TestCmdRunWorkingDirectory(TestDvc):

tests/func/test_system.py

@@ -1,19 +1,16 @@
 import os
+import pytest
 
 from dvc.system import System
-from tests.basic_env import TestDir
 
 
-class TestSystem(TestDir):
-    def test_getdirinfo(self):
-        if os.name != "nt":
-            return
-
-        # simulate someone holding an exclussive access
-        locked = "locked"
-        fd = os.open(locked, os.O_WRONLY | os.O_CREAT | os.O_EXCL)
-
+@pytest.mark.skipif(os.name != "nt", reason="Windows specific")
+def test_getdirinfo(tmp_path):
+    # simulate someone holding an exclussive access
+    locked = str(tmp_path / "locked")
+    fd = os.open(locked, os.O_WRONLY | os.O_CREAT | os.O_EXCL)
+    try:
         # should not raise WinError
         System.getdirinfo(locked)
-
+    finally:
         os.close(fd)

tests/func/test_tree.py

@@ -17,10 +17,10 @@ class TestWorkingTree(TestDir):
         self.tree = WorkingTree()
 
     def test_open(self):
-        self.assertEqual(self.tree.open(self.FOO).read(), self.FOO_CONTENTS)
-        self.assertEqual(
-            self.tree.open(self.UNICODE).read(), self.UNICODE_CONTENTS
-        )
+        with self.tree.open(self.FOO) as fd:
+            self.assertEqual(fd.read(), self.FOO_CONTENTS)
+        with self.tree.open(self.UNICODE) as fd:
+            self.assertEqual(fd.read(), self.UNICODE_CONTENTS)
 
     def test_exists(self):
         self.assertTrue(self.tree.exists(self.FOO))
@@ -47,10 +47,10 @@ class TestGitTree(TestGit):
     def test_open(self):
         self.scm.add([self.FOO, self.UNICODE, self.DATA_DIR])
         self.scm.commit("add")
-        self.assertEqual(self.tree.open(self.FOO).read(), self.FOO_CONTENTS)
-        self.assertEqual(
-            self.tree.open(self.UNICODE).read(), self.UNICODE_CONTENTS
-        )
+        with self.tree.open(self.FOO) as fd:
+            self.assertEqual(fd.read(), self.FOO_CONTENTS)
+        with self.tree.open(self.UNICODE) as fd:
+            self.assertEqual(fd.read(), self.UNICODE_CONTENTS)
         with self.assertRaises(IOError):
             self.tree.open("not-existing-file")
         with self.assertRaises(IOError):