From c9391df3133b4019dbe46e078acf54ce39a5b44a Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Mon, 15 Jun 2020 20:31:08 +0300 Subject: [PATCH] plots: modify: handle --unset for nonexistent options (#4044) * tests: plots: reorginize the structure * plots: modify: handle --unset for nonexistent options Fixes #3933 --- dvc/repo/plots/__init__.py | 20 +++++++++- tests/func/conftest.py | 30 ++++++++++++++ tests/func/metrics/conftest.py | 1 - tests/func/plots/__init__.py | 30 -------------- tests/func/plots/conftest.py | 13 +++++- tests/func/plots/test_modify.py | 70 +++++++++++++++++++++++++++++++++ tests/func/plots/test_plots.py | 56 -------------------------- 7 files changed, 130 insertions(+), 90 deletions(-) create mode 100644 tests/func/conftest.py delete mode 100644 tests/func/metrics/conftest.py create mode 100644 tests/func/plots/test_modify.py diff --git a/dvc/repo/plots/__init__.py b/dvc/repo/plots/__init__.py index 07d144a1a..8e873e915 100644 --- a/dvc/repo/plots/__init__.py +++ b/dvc/repo/plots/__init__.py @@ -17,6 +17,10 @@ class NotAPlotError(DvcException): ) +class PropsNotFoundError(DvcException): + pass + + class Plots: def __init__(self, repo): self.repo = repo @@ -93,6 +97,17 @@ class Plots: return diff(self.repo, *args, **kwargs) + @staticmethod + def _unset(out, props): + missing = list(set(props) - set(out.plot.keys())) + if missing: + raise PropsNotFoundError( + f"display properties {missing} not found in plot '{out}'" + ) + + for prop in props: + out.plot.pop(prop) + def modify(self, path, props=None, unset=None): from dvc.dvcfile import Dvcfile @@ -109,8 +124,9 @@ class Plots: if not isinstance(out.plot, dict): out.plot = {} - for field in unset or (): - out.plot.pop(field, None) + if unset: + self._unset(out, unset) + out.plot.update(props) # Empty dict will move it to non-plots diff --git a/tests/func/conftest.py b/tests/func/conftest.py new file mode 100644 index 000000000..41851900f --- /dev/null +++ b/tests/func/conftest.py @@ -0,0 +1,30 @@ +import os + +import pytest + + +@pytest.fixture +def run_copy_metrics(tmp_dir, run_copy): + def run(file1, file2, commit=None, tag=None, single_stage=True, **kwargs): + stage = tmp_dir.dvc.run( + cmd=f"python copy.py {file1} {file2}", + deps=[file1], + single_stage=single_stage, + **kwargs, + ) + + if hasattr(tmp_dir.dvc, "scm"): + files = [stage.path] + files += [ + os.fspath(out.path_info) + for out in stage.outs + if not out.use_cache + ] + tmp_dir.dvc.scm.add(files) + if commit: + tmp_dir.dvc.scm.commit(commit) + if tag: + tmp_dir.dvc.scm.tag(tag) + return stage + + return run diff --git a/tests/func/metrics/conftest.py b/tests/func/metrics/conftest.py deleted file mode 100644 index 2e482d908..000000000 --- a/tests/func/metrics/conftest.py +++ /dev/null @@ -1 +0,0 @@ -from tests.func.plots import run_copy_metrics # noqa: F401 diff --git a/tests/func/plots/__init__.py b/tests/func/plots/__init__.py index 41851900f..e69de29bb 100644 --- a/tests/func/plots/__init__.py +++ b/tests/func/plots/__init__.py @@ -1,30 +0,0 @@ -import os - -import pytest - - -@pytest.fixture -def run_copy_metrics(tmp_dir, run_copy): - def run(file1, file2, commit=None, tag=None, single_stage=True, **kwargs): - stage = tmp_dir.dvc.run( - cmd=f"python copy.py {file1} {file2}", - deps=[file1], - single_stage=single_stage, - **kwargs, - ) - - if hasattr(tmp_dir.dvc, "scm"): - files = [stage.path] - files += [ - os.fspath(out.path_info) - for out in stage.outs - if not out.use_cache - ] - tmp_dir.dvc.scm.add(files) - if commit: - tmp_dir.dvc.scm.commit(commit) - if tag: - tmp_dir.dvc.scm.tag(tag) - return stage - - return run diff --git a/tests/func/plots/conftest.py b/tests/func/plots/conftest.py index 7b4cf09a0..9f94dcff5 100644 --- a/tests/func/plots/conftest.py +++ b/tests/func/plots/conftest.py @@ -1 +1,12 @@ -from . import run_copy_metrics # noqa: F401 +import shutil + +import pytest + + +@pytest.fixture() +def custom_template(tmp_dir, dvc): + custom_template = tmp_dir / "custom_template.json" + shutil.copy( + tmp_dir / ".dvc" / "plots" / "default.json", custom_template, + ) + return custom_template diff --git a/tests/func/plots/test_modify.py b/tests/func/plots/test_modify.py new file mode 100644 index 000000000..ad11f1bee --- /dev/null +++ b/tests/func/plots/test_modify.py @@ -0,0 +1,70 @@ +import pytest + +from dvc.dvcfile import PIPELINE_LOCK +from dvc.repo.plots import PropsNotFoundError +from dvc.repo.plots.template import TemplateNotFoundError +from dvc.utils import relpath + +from .test_plots import _write_json + + +def test_plots_modify_existing_template( + tmp_dir, dvc, run_copy_metrics, custom_template +): + metric = [{"a": 1, "b": 2}, {"a": 2, "b": 3}] + _write_json(tmp_dir, metric, "metric_t.json") + stage = run_copy_metrics( + "metric_t.json", + "metric.json", + plots_no_cache=["metric.json"], + name="copy-metrics", + single_stage=False, + ) + dvc.plots.modify( + "metric.json", props={"template": relpath(custom_template)} + ) + stage = stage.reload() + assert stage.outs[0].plot == {"template": relpath(custom_template)} + + +def test_plots_modify_should_not_change_lockfile( + tmp_dir, dvc, run_copy_metrics, custom_template +): + _write_json(tmp_dir, [{"a": 1, "b": 2}], "metric_t.json") + run_copy_metrics( + "metric_t.json", + "metric.json", + plots_no_cache=["metric.json"], + name="copy-metrics", + single_stage=False, + ) + + (tmp_dir / PIPELINE_LOCK).unlink() + dvc.plots.modify( + "metric.json", props={"template": relpath(custom_template)} + ) + assert not (tmp_dir / PIPELINE_LOCK).exists() + + +def test_plots_modify_not_existing_template(dvc): + with pytest.raises(TemplateNotFoundError): + dvc.plots.modify( + "metric.json", props={"template": "not-existing-template.json"} + ) + + +def test_unset_nonexistent(tmp_dir, dvc, run_copy_metrics, custom_template): + metric = [{"a": 1, "b": 2}, {"a": 2, "b": 3}] + _write_json(tmp_dir, metric, "metric_t.json") + run_copy_metrics( + "metric_t.json", + "metric.json", + plots_no_cache=["metric.json"], + name="copy-metrics", + single_stage=False, + ) + + with pytest.raises(PropsNotFoundError): + dvc.plots.modify( + "metric.json", unset=["nonexistent"], + ) diff --git a/tests/func/plots/test_plots.py b/tests/func/plots/test_plots.py index 294515a8d..33a910026 100644 --- a/tests/func/plots/test_plots.py +++ b/tests/func/plots/test_plots.py @@ -9,7 +9,6 @@ import pytest import yaml from funcy import first -from dvc.dvcfile import PIPELINE_LOCK from dvc.repo.plots.data import ( JSONPlotData, NoMetricInHistoryError, @@ -18,7 +17,6 @@ from dvc.repo.plots.data import ( YAMLPlotData, ) from dvc.repo.plots.template import NoFieldInDataError, TemplateNotFoundError -from dvc.utils import relpath def _write_csv(metric, filename, header=True): @@ -358,15 +356,6 @@ def test_throw_on_no_metric_at_all(tmp_dir, scm, dvc, caplog): assert str(error.value) == "Could not find 'metric.json'." -@pytest.fixture() -def custom_template(tmp_dir, dvc): - custom_template = tmp_dir / "custom_template.json" - shutil.copy( - tmp_dir / ".dvc" / "plots" / "default.json", custom_template, - ) - return custom_template - - def test_custom_template(tmp_dir, scm, dvc, custom_template, run_copy_metrics): metric = [{"a": 1, "b": 2}, {"a": 2, "b": 3}] _write_json(tmp_dir, metric, "metric_t.json") @@ -542,51 +531,6 @@ def test_load_metric_from_dict_yaml(tmp_dir): assert list(map(dict, plot_data.to_datapoints())) == expected -def test_plots_modify_existing_template( - tmp_dir, dvc, run_copy_metrics, custom_template -): - metric = [{"a": 1, "b": 2}, {"a": 2, "b": 3}] - _write_json(tmp_dir, metric, "metric_t.json") - stage = run_copy_metrics( - "metric_t.json", - "metric.json", - plots_no_cache=["metric.json"], - name="copy-metrics", - single_stage=False, - ) - dvc.plots.modify( - "metric.json", props={"template": relpath(custom_template)} - ) - stage = stage.reload() - assert stage.outs[0].plot == {"template": relpath(custom_template)} - - -def test_plots_modify_should_not_change_lockfile( - tmp_dir, dvc, run_copy_metrics, custom_template -): - _write_json(tmp_dir, [{"a": 1, "b": 2}], "metric_t.json") - run_copy_metrics( - "metric_t.json", - "metric.json", - plots_no_cache=["metric.json"], - name="copy-metrics", - single_stage=False, - ) - - (tmp_dir / PIPELINE_LOCK).unlink() - dvc.plots.modify( - "metric.json", props={"template": relpath(custom_template)} - ) - assert not (tmp_dir / PIPELINE_LOCK).exists() - - -def test_plots_modify_not_existing_template(dvc): - with pytest.raises(TemplateNotFoundError): - dvc.plots.modify( - "metric.json", props={"template": "not-existing-template.json"} - ) - - def test_multiple_plots(tmp_dir, scm, dvc, run_copy_metrics): metric1 = [ OrderedDict([("first_val", 100), ("second_val", 100), ("val", 2)]),