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

@@ -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

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

tests/func/metrics/conftest.py

@@ -1 +0,0 @@
-from tests.func.plots import run_copy_metrics  # noqa: F401

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

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

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"],
+        )

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)]),