From 0c724d28b7b9547b5826d00c180453758a4c789f Mon Sep 17 00:00:00 2001 From: Alexander Schepanovski Date: Thu, 18 Apr 2019 17:54:17 +0700 Subject: [PATCH 1/3] test: clean up test dirs Closes #1770. Partially does #1888. --- tests/basic_env.py | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/tests/basic_env.py b/tests/basic_env.py index 0198dc7f0f..d410293204 100644 --- a/tests/basic_env.py +++ b/tests/basic_env.py @@ -7,6 +7,7 @@ import uuid import tempfile import logging +import warnings from git import Repo from git.exc import GitCommandNotFound @@ -60,7 +61,8 @@ def __init__(self, root_dir=None): 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 @@ def setUp(self): 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): @@ -196,6 +213,9 @@ class MockConfig: ) 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. From 26b398e203437334a0c10bd4360ab20b38430b8d Mon Sep 17 00:00:00 2001 From: Alexander Schepanovski Date: Fri, 19 Apr 2019 22:04:47 +0700 Subject: [PATCH 2/3] dvc: close anything opened --- dvc/system.py | 21 ++++++--------------- dvc/utils/__init__.py | 28 +++++++++++----------------- tests/func/test_checkout.py | 5 ++--- tests/func/test_import.py | 3 ++- tests/func/test_run.py | 24 ++++++++++++++++-------- tests/func/test_system.py | 19 ++++++++----------- tests/func/test_tree.py | 16 ++++++++-------- 7 files changed, 53 insertions(+), 63 deletions(-) diff --git a/dvc/system.py b/dvc/system.py index 8ca0d53de0..77f622cbfb 100644 --- a/dvc/system.py +++ b/dvc/system.py @@ -102,22 +102,13 @@ def _reflink_linux(src, dst): 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 diff --git a/dvc/utils/__init__.py b/dvc/utils/__init__.py index 6fa901bc00..784ba1ab4d 100644 --- a/dvc/utils/__init__.py +++ b/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+") - - 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) + dest = os.path.join(dest, os.path.basename(src)) + + 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) diff --git a/tests/func/test_checkout.py b/tests/func/test_checkout.py index 1d690c5d1b..a384cd5f3e 100644 --- a/tests/func/test_checkout.py +++ b/tests/func/test_checkout.py @@ -126,9 +126,8 @@ def commit_data_file(self, fname, content="random text"): 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") diff --git a/tests/func/test_import.py b/tests/func/test_import.py index db908cf5ab..ede967f596 100644 --- a/tests/func/test_import.py +++ b/tests/func/test_import.py @@ -41,7 +41,8 @@ def test(self): 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): diff --git a/tests/func/test_run.py b/tests/func/test_run.py index 9015e4eab1..cebca9d8ad 100644 --- a/tests/func/test_run.py +++ b/tests/func/test_run.py @@ -380,7 +380,8 @@ def test(self): ) 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( [ @@ -398,7 +399,8 @@ def test(self): ) 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): @@ -431,7 +433,8 @@ def test(self): 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( [ @@ -450,7 +453,8 @@ def test(self): 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): @@ -483,7 +487,8 @@ def test(self): 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( [ @@ -502,7 +507,8 @@ def test(self): 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): @@ -602,12 +608,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): diff --git a/tests/func/test_system.py b/tests/func/test_system.py index 45b79cebd0..432493fb3b 100644 --- a/tests/func/test_system.py +++ b/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) diff --git a/tests/func/test_tree.py b/tests/func/test_tree.py index 9792bd8e66..8e937d4161 100644 --- a/tests/func/test_tree.py +++ b/tests/func/test_tree.py @@ -17,10 +17,10 @@ def setUp(self): 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 @@ def setUp(self): 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): From d3a8d1594fb7b8986c9b7b413b455bde36429840 Mon Sep 17 00:00:00 2001 From: Alexander Schepanovski Date: Wed, 24 Apr 2019 14:49:59 +0700 Subject: [PATCH 3/3] test: finalize git repo explicitly to remove test dir later This was previously an issue on Windows. --- tests/basic_env.py | 6 ++++++ tests/conftest.py | 18 ++++++++++++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/basic_env.py b/tests/basic_env.py index d410293204..831875b50e 100644 --- a/tests/basic_env.py +++ b/tests/basic_env.py @@ -139,6 +139,9 @@ def setUp(self): 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): @@ -162,6 +165,9 @@ def setUp(self): 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): diff --git a/tests/conftest.py b/tests/conftest.py index 733a1bb2f3..2a62fe1c5e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -40,13 +40,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()