From e510b9b55913cb3c05ea5a0746018329ab2d9807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Saugat=20Pachhai=20=28=E0=A4=B8=E0=A5=8C=E0=A4=97=E0=A4=BE?= =?UTF-8?q?=E0=A4=A4=29?= Date: Mon, 28 Dec 2020 15:52:14 +0545 Subject: [PATCH 1/3] Fix error when the file in `vars` is a directory --- dvc/parsing/context.py | 15 ++++++++++----- tests/unit/test_context.py | 21 ++++++++++++++++----- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/dvc/parsing/context.py b/dvc/parsing/context.py index a9abbf5661..e6cf7f56b8 100644 --- a/dvc/parsing/context.py +++ b/dvc/parsing/context.py @@ -62,7 +62,7 @@ def __init__(self, key, new, into): ) -class ParamsFileNotFound(ContextError): +class ParamsLoadError(ContextError): pass @@ -349,15 +349,20 @@ def select( def load_from(cls, tree, path: PathInfo, select_keys=None) -> "Context": file = relpath(path) if not tree.exists(path): - raise ParamsFileNotFound(f"'{file}' does not exist") + raise ParamsLoadError(f"'{file}' does not exist") _, ext = os.path.splitext(file) loader = LOADERS[ext] - data = loader(path, tree=tree) + try: + data = loader(path, tree=tree) + except IsADirectoryError as exc: + msg = f"Cannot load '{file}', '{file}' is a directory" + raise ParamsLoadError(msg) from exc + if not isinstance(data, Mapping): typ = type(data).__name__ - raise ContextError( + raise ParamsLoadError( f"expected a dictionary, got '{typ}' in file '{file}'" ) @@ -367,7 +372,7 @@ def load_from(cls, tree, path: PathInfo, select_keys=None) -> "Context": data = {key: data[key] for key in select_keys} except KeyError as exc: key, *_ = exc.args - raise ContextError( + raise ParamsLoadError( f"could not find '{key}' in '{file}'" ) from exc diff --git a/tests/unit/test_context.py b/tests/unit/test_context.py index 2cab30d18f..1f8a1c15c3 100644 --- a/tests/unit/test_context.py +++ b/tests/unit/test_context.py @@ -10,7 +10,7 @@ CtxList, KeyNotInContext, MergeError, - ParamsFileNotFound, + ParamsLoadError, Value, recurse_not_a_node, ) @@ -430,7 +430,18 @@ def test_resolve_resolves_boolean_value(): assert context.resolve_str("--flag ${disabled}") == "--flag false" -def test_merge_from_raises_if_file_not_exist(tmp_dir, dvc): - context = Context(foo="bar") - with pytest.raises(ParamsFileNotFound): - context.merge_from(dvc.tree, DEFAULT_PARAMS_FILE, tmp_dir) +def test_load_from_raises_if_file_not_exist(tmp_dir, dvc): + with pytest.raises(ParamsLoadError) as exc_info: + Context.load_from(dvc.tree, tmp_dir / DEFAULT_PARAMS_FILE) + + assert str(exc_info.value) == "'params.yaml' does not exist" + + +def test_load_from_raises_if_file_is_directory(tmp_dir, dvc): + data_dir = tmp_dir / "data" + data_dir.mkdir() + + with pytest.raises(ParamsLoadError) as exc_info: + Context.load_from(dvc.tree, data_dir) + + assert str(exc_info.value) == "Cannot load 'data', 'data' is a directory" From 7e4ed512a5d130359e5548c895dbc2f32e93bdc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Saugat=20Pachhai=20=28=E0=A4=B8=E0=A5=8C=E0=A4=97=E0=A4=BE?= =?UTF-8?q?=E0=A4=A4=29?= Date: Mon, 28 Dec 2020 16:47:09 +0545 Subject: [PATCH 2/3] Fix on windows --- dvc/parsing/context.py | 9 +++------ tests/unit/test_context.py | 10 +++++----- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/dvc/parsing/context.py b/dvc/parsing/context.py index e6cf7f56b8..b911ab8fc4 100644 --- a/dvc/parsing/context.py +++ b/dvc/parsing/context.py @@ -350,16 +350,13 @@ def load_from(cls, tree, path: PathInfo, select_keys=None) -> "Context": file = relpath(path) if not tree.exists(path): raise ParamsLoadError(f"'{file}' does not exist") + if tree.isdir(path): + raise ParamsLoadError(f"'{file}' is a directory") _, ext = os.path.splitext(file) loader = LOADERS[ext] - try: - data = loader(path, tree=tree) - except IsADirectoryError as exc: - msg = f"Cannot load '{file}', '{file}' is a directory" - raise ParamsLoadError(msg) from exc - + data = loader(path, tree=tree) if not isinstance(data, Mapping): typ = type(data).__name__ raise ParamsLoadError( diff --git a/tests/unit/test_context.py b/tests/unit/test_context.py index 1f8a1c15c3..25e3d597be 100644 --- a/tests/unit/test_context.py +++ b/tests/unit/test_context.py @@ -221,12 +221,12 @@ def _yaml_load(*args, **kwargs): mocker.patch("dvc.parsing.context.LOADERS", {".yaml": _yaml_load}) - class tree: - def exists(self, _): - return True + tree = mocker.Mock( + **{"exists.return_value": True, "isdir.return_value": False} + ) file = "params.yaml" - c = Context.load_from(tree(), file) + c = Context.load_from(tree, file) assert asdict(c["x"].meta) == { "source": file, @@ -444,4 +444,4 @@ def test_load_from_raises_if_file_is_directory(tmp_dir, dvc): with pytest.raises(ParamsLoadError) as exc_info: Context.load_from(dvc.tree, data_dir) - assert str(exc_info.value) == "Cannot load 'data', 'data' is a directory" + assert str(exc_info.value) == "'data' is a directory" From 7c7904b03afe0846c3bfb4729b458ee8476feb9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Saugat=20Pachhai=20=28=E0=A4=B8=E0=A5=8C=E0=A4=97=E0=A4=BE?= =?UTF-8?q?=E0=A4=A4=29?= Date: Mon, 28 Dec 2020 17:10:27 +0545 Subject: [PATCH 3/3] mock tree only --- tests/unit/test_context.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/unit/test_context.py b/tests/unit/test_context.py index 25e3d597be..eae4dc056e 100644 --- a/tests/unit/test_context.py +++ b/tests/unit/test_context.py @@ -1,5 +1,6 @@ from dataclasses import asdict from math import pi +from unittest.mock import mock_open import pytest @@ -16,7 +17,7 @@ ) from dvc.tree.local import LocalTree from dvc.utils import relpath -from dvc.utils.serialize import dump_yaml +from dvc.utils.serialize import dump_yaml, dumps_yaml def test_context(): @@ -216,15 +217,11 @@ def test_overwrite_with_setitem(): def test_load_from(mocker): - def _yaml_load(*args, **kwargs): - return {"x": {"y": {"z": 5}, "lst": [1, 2, 3]}, "foo": "foo"} - - mocker.patch("dvc.parsing.context.LOADERS", {".yaml": _yaml_load}) - + d = {"x": {"y": {"z": 5}, "lst": [1, 2, 3]}, "foo": "foo"} tree = mocker.Mock( - **{"exists.return_value": True, "isdir.return_value": False} + open=mock_open(read_data=dumps_yaml(d)), + **{"exists.return_value": True, "isdir.return_value": False}, ) - file = "params.yaml" c = Context.load_from(tree, file)