From 7c94872bf42981e2e87f3e126af6bc07281cf7d8 Mon Sep 17 00:00:00 2001 From: Charles Baynham Date: Wed, 22 Apr 2020 11:25:47 +0100 Subject: [PATCH 1/8] Convert all paths in config to posix-style --- dvc/config.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/dvc/config.py b/dvc/config.py index 1fc63acbb3..e7f3dacbb6 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -1,14 +1,24 @@ """DVC config objects.""" -from contextlib import contextmanager import logging import os import re +from contextlib import contextmanager +from pathlib import Path, PurePosixPath from urllib.parse import urlparse -from funcy import cached_property, re_find, walk_values, compact import configobj -from voluptuous import Schema, Optional, Invalid, ALLOW_EXTRA -from voluptuous import All, Any, Lower, Range, Coerce +from funcy import cached_property, compact, re_find, walk_values +from voluptuous import ( + ALLOW_EXTRA, + All, + Any, + Coerce, + Invalid, + Lower, + Optional, + Range, + Schema, +) from dvc.exceptions import DvcException, NotDvcRepoError from dvc.utils import relpath @@ -320,8 +330,10 @@ def rel(path): return path if isinstance(path, RelPath) or not os.path.isabs(path): - return relpath(path, conf_dir) - return path + path = relpath(path, conf_dir) + + posix_path = str(PurePosixPath(Path(path))) + return posix_path return Config._map_dirs(conf, rel) From c3c5313970a9393d87560230a47017366b2f0897 Mon Sep 17 00:00:00 2001 From: Charles Baynham Date: Wed, 22 Apr 2020 12:46:29 +0100 Subject: [PATCH 2/8] Use PathInfo for consistancy, add unit test and change functional tests --- dvc/config.py | 5 ++--- tests/func/test_cache.py | 8 ++++---- tests/func/test_remote.py | 5 +++-- tests/unit/command/test_cache.py | 22 ++++++++++++++++++++++ 4 files changed, 31 insertions(+), 9 deletions(-) create mode 100644 tests/unit/command/test_cache.py diff --git a/dvc/config.py b/dvc/config.py index e7f3dacbb6..26da6bacaf 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -3,7 +3,6 @@ import os import re from contextlib import contextmanager -from pathlib import Path, PurePosixPath from urllib.parse import urlparse import configobj @@ -21,6 +20,7 @@ ) from dvc.exceptions import DvcException, NotDvcRepoError +from dvc.path_info import PathInfo from dvc.utils import relpath logger = logging.getLogger(__name__) @@ -332,8 +332,7 @@ def rel(path): if isinstance(path, RelPath) or not os.path.isabs(path): path = relpath(path, conf_dir) - posix_path = str(PurePosixPath(Path(path))) - return posix_path + return PathInfo(path).as_posix() return Config._map_dirs(conf, rel) diff --git a/tests/func/test_cache.py b/tests/func/test_cache.py index 8d692938d2..7b798478e6 100644 --- a/tests/func/test_cache.py +++ b/tests/func/test_cache.py @@ -1,5 +1,6 @@ import os import stat +from pathlib import Path import configobj import pytest @@ -8,8 +9,7 @@ from dvc.main import main from dvc.remote.base import DirCacheError from dvc.utils import relpath -from tests.basic_env import TestDir -from tests.basic_env import TestDvc +from tests.basic_env import TestDir, TestDvc class TestCache(TestDvc): @@ -155,7 +155,7 @@ def test_abs_path(self): self.assertEqual(ret, 0) config = configobj.ConfigObj(self.dvc.config.files["repo"]) - self.assertEqual(config["cache"]["dir"], dname) + self.assertEqual(Path(config["cache"]["dir"]), Path(dname)) def test_relative_path(self): tmpdir = self.mkdtemp() @@ -167,7 +167,7 @@ def test_relative_path(self): # dir path written to config should be just one level above. rel = os.path.join("..", dname) config = configobj.ConfigObj(self.dvc.config.files["repo"]) - self.assertEqual(config["cache"]["dir"], rel) + self.assertEqual(Path(config["cache"]["dir"]), Path(rel)) ret = main(["add", self.FOO]) self.assertEqual(ret, 0) diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 4369203dd7..decda1fece 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -1,17 +1,18 @@ import errno import os +from pathlib import Path import configobj import pytest from mock import patch +from dvc.compat import fspath from dvc.config import Config from dvc.exceptions import DownloadError, UploadError from dvc.main import main from dvc.path_info import PathInfo from dvc.remote import RemoteLOCAL from dvc.remote.base import RemoteBASE, RemoteCacheRequiredError -from dvc.compat import fspath from dvc.utils.fs import remove from tests.basic_env import TestDvc from tests.remotes import Local @@ -47,7 +48,7 @@ def test_relative_path(self): # dir path written to config should be just one level above. rel = os.path.join("..", dname) config = configobj.ConfigObj(self.dvc.config.files["repo"]) - self.assertEqual(config['remote "mylocal"']["url"], rel) + self.assertEqual(Path(config['remote "mylocal"']["url"]), Path(rel)) def test_overwrite(self): remote_name = "a" diff --git a/tests/unit/command/test_cache.py b/tests/unit/command/test_cache.py new file mode 100644 index 0000000000..c00ece627c --- /dev/null +++ b/tests/unit/command/test_cache.py @@ -0,0 +1,22 @@ +from mock import MagicMock + +from dvc.config import Config + + +def test_path_replacement(): + + config_filename = "./config/file" + + # Extract the "rel()" function from Config._save_paths + Config._map_dirs = MagicMock() + Config._save_paths(None, config_filename) + _, rel_func = Config._map_dirs.call_args[0] + + assert rel_func("cache") == "../cache" + assert rel_func("../cache") == "../../cache" + assert rel_func("/path/to/cache") == "/path/to/cache" + + assert rel_func("..\\cache") == "../../cache" + assert rel_func("c:\\path\\to\\cache") == "c:/path/to/cache" + + assert rel_func("ssh://something") == "ssh://something" From c66ade371310fb87ed40d16e10fb2e45709e804c Mon Sep 17 00:00:00 2001 From: Charles Baynham Date: Wed, 22 Apr 2020 13:39:31 +0100 Subject: [PATCH 3/8] Exclude Windows test on Linux --- tests/unit/command/test_cache.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/unit/command/test_cache.py b/tests/unit/command/test_cache.py index c00ece627c..2cfdcba1f8 100644 --- a/tests/unit/command/test_cache.py +++ b/tests/unit/command/test_cache.py @@ -1,3 +1,5 @@ +import os +import pytest from mock import MagicMock from dvc.config import Config @@ -20,3 +22,17 @@ def test_path_replacement(): assert rel_func("c:\\path\\to\\cache") == "c:/path/to/cache" assert rel_func("ssh://something") == "ssh://something" + + +@pytest.mark.skipif(os.name != "nt", reason="Windows only") +def test_path_replacement_windows(): + + config_filename = "./config/file" + + # Extract the "rel()" function from Config._save_paths + Config._map_dirs = MagicMock() + Config._save_paths(None, config_filename) + _, rel_func = Config._map_dirs.call_args[0] + + assert rel_func("..\\cache") == "../../cache" + assert rel_func("c:\\path\\to\\cache") == "c:/path/to/cache" From 2c28724a564f3e228e194f94c8b82ba17da851e0 Mon Sep 17 00:00:00 2001 From: Charles Baynham Date: Wed, 22 Apr 2020 15:12:58 +0100 Subject: [PATCH 4/8] Don't patch Config for all the tests! --- tests/unit/command/test_cache.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/unit/command/test_cache.py b/tests/unit/command/test_cache.py index 2cfdcba1f8..6759ecaad8 100644 --- a/tests/unit/command/test_cache.py +++ b/tests/unit/command/test_cache.py @@ -10,9 +10,10 @@ def test_path_replacement(): config_filename = "./config/file" # Extract the "rel()" function from Config._save_paths - Config._map_dirs = MagicMock() - Config._save_paths(None, config_filename) - _, rel_func = Config._map_dirs.call_args[0] + PatchedConfig = Config + PatchedConfig._map_dirs = MagicMock() + PatchedConfig._save_paths(None, config_filename) + _, rel_func = PatchedConfig._map_dirs.call_args[0] assert rel_func("cache") == "../cache" assert rel_func("../cache") == "../../cache" @@ -30,9 +31,10 @@ def test_path_replacement_windows(): config_filename = "./config/file" # Extract the "rel()" function from Config._save_paths - Config._map_dirs = MagicMock() - Config._save_paths(None, config_filename) - _, rel_func = Config._map_dirs.call_args[0] + PatchedConfig = Config + PatchedConfig._map_dirs = MagicMock() + PatchedConfig._save_paths(None, config_filename) + _, rel_func = PatchedConfig._map_dirs.call_args[0] assert rel_func("..\\cache") == "../../cache" assert rel_func("c:\\path\\to\\cache") == "c:/path/to/cache" From 45d34062e556e99c51da784f7c6daa667a87a451 Mon Sep 17 00:00:00 2001 From: Charles Baynham Date: Wed, 22 Apr 2020 15:16:01 +0100 Subject: [PATCH 5/8] Don't do the linux test on Windows either --- tests/unit/command/test_cache.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/command/test_cache.py b/tests/unit/command/test_cache.py index 6759ecaad8..7a0b4fcee6 100644 --- a/tests/unit/command/test_cache.py +++ b/tests/unit/command/test_cache.py @@ -5,6 +5,7 @@ from dvc.config import Config +@pytest.mark.skipif(os.name == "nt", reason="Linux only") def test_path_replacement(): config_filename = "./config/file" @@ -19,9 +20,6 @@ def test_path_replacement(): assert rel_func("../cache") == "../../cache" assert rel_func("/path/to/cache") == "/path/to/cache" - assert rel_func("..\\cache") == "../../cache" - assert rel_func("c:\\path\\to\\cache") == "c:/path/to/cache" - assert rel_func("ssh://something") == "ssh://something" @@ -38,3 +36,5 @@ def test_path_replacement_windows(): assert rel_func("..\\cache") == "../../cache" assert rel_func("c:\\path\\to\\cache") == "c:/path/to/cache" + + assert rel_func("ssh://something") == "ssh://something" From f1a413d94b4a2e234e91fc48820d97e039a5be01 Mon Sep 17 00:00:00 2001 From: Charles Baynham Date: Wed, 22 Apr 2020 15:58:06 +0100 Subject: [PATCH 6/8] De-hackify the unit test, make the functional test more explicit --- dvc/config.py | 19 ++++++++------- tests/func/test_remote.py | 9 +++---- tests/unit/command/test_cache.py | 40 ------------------------------- tests/unit/command/test_config.py | 39 ++++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 52 deletions(-) delete mode 100644 tests/unit/command/test_cache.py create mode 100644 tests/unit/command/test_config.py diff --git a/dvc/config.py b/dvc/config.py index 26da6bacaf..224e1ac28a 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -3,6 +3,7 @@ import os import re from contextlib import contextmanager +from functools import partial from urllib.parse import urlparse import configobj @@ -322,17 +323,19 @@ def resolve(path): return Config._map_dirs(conf, resolve) @staticmethod - def _save_paths(conf, filename): - conf_dir = os.path.dirname(filename) + def _to_relpath(conf_dir, path): + if re.match(r"\w+://", path): + return path - def rel(path): - if re.match(r"\w+://", path): - return path + if isinstance(path, RelPath) or not os.path.isabs(path): + path = relpath(path, conf_dir) - if isinstance(path, RelPath) or not os.path.isabs(path): - path = relpath(path, conf_dir) + return PathInfo(path).as_posix() - return PathInfo(path).as_posix() + @staticmethod + def _save_paths(conf, filename): + conf_dir = os.path.dirname(filename) + rel = partial(Config._to_relpath, conf_dir) return Config._map_dirs(conf, rel) diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index decda1fece..1393b5aaa1 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -1,6 +1,6 @@ import errno import os -from pathlib import Path +import posixpath import configobj import pytest @@ -40,15 +40,16 @@ def test(self): self.assertEqual(main(["remote", "list"]), 0) def test_relative_path(self): - dname = os.path.join("..", "path", "to", "dir") + dname_parts = ["..", "path", "to", "dir"] + dname = os.path.join(*dname_parts) ret = main(["remote", "add", "mylocal", dname]) self.assertEqual(ret, 0) # NOTE: we are in the repo's root and config is in .dvc/, so # dir path written to config should be just one level above. - rel = os.path.join("..", dname) + rel = posixpath.join("..", *dname_parts) config = configobj.ConfigObj(self.dvc.config.files["repo"]) - self.assertEqual(Path(config['remote "mylocal"']["url"]), Path(rel)) + self.assertEqual(config['remote "mylocal"']["url"], rel) def test_overwrite(self): remote_name = "a" diff --git a/tests/unit/command/test_cache.py b/tests/unit/command/test_cache.py deleted file mode 100644 index 7a0b4fcee6..0000000000 --- a/tests/unit/command/test_cache.py +++ /dev/null @@ -1,40 +0,0 @@ -import os -import pytest -from mock import MagicMock - -from dvc.config import Config - - -@pytest.mark.skipif(os.name == "nt", reason="Linux only") -def test_path_replacement(): - - config_filename = "./config/file" - - # Extract the "rel()" function from Config._save_paths - PatchedConfig = Config - PatchedConfig._map_dirs = MagicMock() - PatchedConfig._save_paths(None, config_filename) - _, rel_func = PatchedConfig._map_dirs.call_args[0] - - assert rel_func("cache") == "../cache" - assert rel_func("../cache") == "../../cache" - assert rel_func("/path/to/cache") == "/path/to/cache" - - assert rel_func("ssh://something") == "ssh://something" - - -@pytest.mark.skipif(os.name != "nt", reason="Windows only") -def test_path_replacement_windows(): - - config_filename = "./config/file" - - # Extract the "rel()" function from Config._save_paths - PatchedConfig = Config - PatchedConfig._map_dirs = MagicMock() - PatchedConfig._save_paths(None, config_filename) - _, rel_func = PatchedConfig._map_dirs.call_args[0] - - assert rel_func("..\\cache") == "../../cache" - assert rel_func("c:\\path\\to\\cache") == "c:/path/to/cache" - - assert rel_func("ssh://something") == "ssh://something" diff --git a/tests/unit/command/test_config.py b/tests/unit/command/test_config.py new file mode 100644 index 0000000000..b1d078ed21 --- /dev/null +++ b/tests/unit/command/test_config.py @@ -0,0 +1,39 @@ +import os +import pytest + +from dvc.config import Config + + +@pytest.mark.skipif(os.name == "nt", reason="Linux only") +def test_path_replacement(): + + config_dirname = "./config" + + assert Config._to_relpath(config_dirname, "cache") == "../cache" + assert Config._to_relpath(config_dirname, "../cache") == "../../cache" + assert ( + Config._to_relpath(config_dirname, "/path/to/cache") + == "/path/to/cache" + ) + + assert ( + Config._to_relpath(config_dirname, "ssh://something") + == "ssh://something" + ) + + +@pytest.mark.skipif(os.name != "nt", reason="Windows only") +def test_path_replacement_windows(): + + config_dirname = ".\\config" + + assert Config._to_relpath(config_dirname, "..\\cache") == "../../cache" + assert ( + Config._to_relpath(config_dirname, "c:\\path\\to\\cache") + == "c:/path/to/cache" + ) + + assert ( + Config._to_relpath(config_dirname, "ssh://something") + == "ssh://something" + ) From 45e0285d31b292ec54a040b241c701ae79d77e9b Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Fri, 24 Apr 2020 02:15:13 +0300 Subject: [PATCH 7/8] tests: adjust tests --- tests/func/test_cache.py | 5 ++-- tests/func/test_remote.py | 10 ++++---- tests/unit/command/test_config.py | 39 ------------------------------- tests/unit/test_config.py | 16 +++++++++++++ 4 files changed, 22 insertions(+), 48 deletions(-) delete mode 100644 tests/unit/command/test_config.py create mode 100644 tests/unit/test_config.py diff --git a/tests/func/test_cache.py b/tests/func/test_cache.py index 7b798478e6..841e06f1cd 100644 --- a/tests/func/test_cache.py +++ b/tests/func/test_cache.py @@ -1,6 +1,5 @@ import os import stat -from pathlib import Path import configobj import pytest @@ -155,7 +154,7 @@ def test_abs_path(self): self.assertEqual(ret, 0) config = configobj.ConfigObj(self.dvc.config.files["repo"]) - self.assertEqual(Path(config["cache"]["dir"]), Path(dname)) + self.assertEqual(config["cache"]["dir"], dname.replace("\\", "/")) def test_relative_path(self): tmpdir = self.mkdtemp() @@ -167,7 +166,7 @@ def test_relative_path(self): # dir path written to config should be just one level above. rel = os.path.join("..", dname) config = configobj.ConfigObj(self.dvc.config.files["repo"]) - self.assertEqual(Path(config["cache"]["dir"]), Path(rel)) + self.assertEqual(config["cache"]["dir"], rel.replace("\\", "/")) ret = main(["add", self.FOO]) self.assertEqual(ret, 0) diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 1393b5aaa1..396e50f8ce 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -1,18 +1,17 @@ import errno import os -import posixpath import configobj import pytest from mock import patch -from dvc.compat import fspath from dvc.config import Config from dvc.exceptions import DownloadError, UploadError from dvc.main import main from dvc.path_info import PathInfo from dvc.remote import RemoteLOCAL from dvc.remote.base import RemoteBASE, RemoteCacheRequiredError +from dvc.compat import fspath from dvc.utils.fs import remove from tests.basic_env import TestDvc from tests.remotes import Local @@ -40,16 +39,15 @@ def test(self): self.assertEqual(main(["remote", "list"]), 0) def test_relative_path(self): - dname_parts = ["..", "path", "to", "dir"] - dname = os.path.join(*dname_parts) + dname = os.path.join("..", "path", "to", "dir") ret = main(["remote", "add", "mylocal", dname]) self.assertEqual(ret, 0) # NOTE: we are in the repo's root and config is in .dvc/, so # dir path written to config should be just one level above. - rel = posixpath.join("..", *dname_parts) + rel = os.path.join("..", dname) config = configobj.ConfigObj(self.dvc.config.files["repo"]) - self.assertEqual(config['remote "mylocal"']["url"], rel) + self.assertEqual(config['remote "mylocal"']["url"], rel.replace("\\", "/")) def test_overwrite(self): remote_name = "a" diff --git a/tests/unit/command/test_config.py b/tests/unit/command/test_config.py deleted file mode 100644 index b1d078ed21..0000000000 --- a/tests/unit/command/test_config.py +++ /dev/null @@ -1,39 +0,0 @@ -import os -import pytest - -from dvc.config import Config - - -@pytest.mark.skipif(os.name == "nt", reason="Linux only") -def test_path_replacement(): - - config_dirname = "./config" - - assert Config._to_relpath(config_dirname, "cache") == "../cache" - assert Config._to_relpath(config_dirname, "../cache") == "../../cache" - assert ( - Config._to_relpath(config_dirname, "/path/to/cache") - == "/path/to/cache" - ) - - assert ( - Config._to_relpath(config_dirname, "ssh://something") - == "ssh://something" - ) - - -@pytest.mark.skipif(os.name != "nt", reason="Windows only") -def test_path_replacement_windows(): - - config_dirname = ".\\config" - - assert Config._to_relpath(config_dirname, "..\\cache") == "../../cache" - assert ( - Config._to_relpath(config_dirname, "c:\\path\\to\\cache") - == "c:/path/to/cache" - ) - - assert ( - Config._to_relpath(config_dirname, "ssh://something") - == "ssh://something" - ) diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py new file mode 100644 index 0000000000..86f9e5a511 --- /dev/null +++ b/tests/unit/test_config.py @@ -0,0 +1,16 @@ +import os +import pytest + +from dvc.config import Config + + +@pytest.mark.parametrize("path, expected", + [ + ("cache", "../cache"), + (os.path.join("..", "cache"), "../../cache"), + (os.getcwd(), os.getcwd().replace("\\", "/")), + ("ssh://some/path", "ssh://some/path"), + ] +) +def test_to_relpath(path, expected): + assert Config._to_relpath(os.path.join(".", "config"), path) == expected From c90ac5d6c0e3af9df8309136c17175ee3249d39f Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Thu, 23 Apr 2020 23:15:28 +0000 Subject: [PATCH 8/8] Restyled by black --- tests/func/test_remote.py | 4 +++- tests/unit/test_config.py | 5 +++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 396e50f8ce..0d3f9e9f54 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -47,7 +47,9 @@ def test_relative_path(self): # dir path written to config should be just one level above. rel = os.path.join("..", dname) config = configobj.ConfigObj(self.dvc.config.files["repo"]) - self.assertEqual(config['remote "mylocal"']["url"], rel.replace("\\", "/")) + self.assertEqual( + config['remote "mylocal"']["url"], rel.replace("\\", "/") + ) def test_overwrite(self): remote_name = "a" diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 86f9e5a511..e0899cbf3e 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -4,13 +4,14 @@ from dvc.config import Config -@pytest.mark.parametrize("path, expected", +@pytest.mark.parametrize( + "path, expected", [ ("cache", "../cache"), (os.path.join("..", "cache"), "../../cache"), (os.getcwd(), os.getcwd().replace("\\", "/")), ("ssh://some/path", "ssh://some/path"), - ] + ], ) def test_to_relpath(path, expected): assert Config._to_relpath(os.path.join(".", "config"), path) == expected