diff --git a/dvc/command/plots.py b/dvc/command/plots.py index 8deb5aff2f..0e28162a11 100644 --- a/dvc/command/plots.py +++ b/dvc/command/plots.py @@ -1,6 +1,5 @@ import argparse import logging -import os from dvc.command import completion from dvc.command.base import CmdBase, append_doc_link, fix_subparsers @@ -23,6 +22,8 @@ def _props(self): return {k: v for k, v in props.items() if v is not None} def run(self): + from pathlib import Path + if self.args.show_vega: if not self.args.targets: logger.error("please specify a target for `--show-vega`") @@ -41,20 +42,20 @@ def run(self): logger.info(plots[target]) return 0 - path = self.args.out or "plots.html" - path = os.path.join(os.getcwd(), path) - + rel: str = self.args.out or "plots.html" + path = (Path.cwd() / rel).resolve() write(path, plots) - url = f"file://{path}" except DvcException: logger.exception("") return 1 + assert path.is_absolute() # as_uri throws ValueError if not absolute + url = path.as_uri() logger.info(url) if self.args.open: import webbrowser - opened = webbrowser.open(url) + opened = webbrowser.open(rel) if not opened: logger.error("Failed to open. Please try opening it manually.") return 1 diff --git a/tests/func/plots/test_show.py b/tests/func/plots/test_show.py index f7c378a25a..59efab8187 100644 --- a/tests/func/plots/test_show.py +++ b/tests/func/plots/test_show.py @@ -708,7 +708,7 @@ def test_show_from_subdir(tmp_dir, dvc, caplog): with subdir.chdir(), caplog.at_level(logging.INFO, "dvc"): assert main(["plots", "show", "metric.json"]) == 0 - assert f"file://{str(subdir)}" in caplog.text + assert subdir.as_uri() in caplog.text assert (subdir / "plots.html").exists() diff --git a/tests/unit/command/test_plots.py b/tests/unit/command/test_plots.py index a2f5b1cc60..510d699b1d 100644 --- a/tests/unit/command/test_plots.py +++ b/tests/unit/command/test_plots.py @@ -1,4 +1,8 @@ import logging +import os +import posixpath + +import pytest from dvc.cli import parse_args from dvc.command.plots import CmdPlotsDiff, CmdPlotsShow @@ -116,12 +120,11 @@ def test_plots_diff_open(tmp_dir, dvc, mocker, caplog): ) assert cmd.run() == 0 + mocked_open.assert_called_once_with("plots.html") - expected_url = f"file://{tmp_dir / 'plots.html'}" + expected_url = posixpath.join(tmp_dir.as_uri(), "plots.html") assert expected_url in caplog.text - mocked_open.assert_called_once_with(expected_url) - def test_plots_diff_open_failed(tmp_dir, dvc, mocker, caplog): mocked_open = mocker.patch("webbrowser.open", return_value=False) @@ -132,12 +135,36 @@ def test_plots_diff_open_failed(tmp_dir, dvc, mocker, caplog): ) assert cmd.run() == 1 - - expected_url = f"file://{tmp_dir / 'plots.html'}" - mocked_open.assert_called_once_with(expected_url) + mocked_open.assert_called_once_with("plots.html") error_message = "Failed to open. Please try opening it manually." + expected_url = posixpath.join(tmp_dir.as_uri(), "plots.html") + assert caplog.record_tuples == [ ("dvc.command.plots", logging.INFO, expected_url), ("dvc.command.plots", logging.ERROR, error_message), ] + + +@pytest.mark.parametrize( + "output, expected_url_path", + [ + ("plots file with spaces.html", "plots%20file%20with%20spaces.html"), + (os.path.join("dir", "..", "plots.html"), "plots.html"), + ], + ids=["quote", "resolve"], +) +def test_plots_path_is_quoted_and_resolved_properly( + tmp_dir, dvc, mocker, caplog, output, expected_url_path +): + cli_args = parse_args( + ["plots", "diff", "--targets", "datafile", "--out", output] + ) + cmd = cli_args.func(cli_args) + mocker.patch( + "dvc.repo.plots.diff.diff", return_value={"datafile": "filledtemplate"} + ) + + assert cmd.run() == 0 + expected_url = posixpath.join(tmp_dir.as_uri(), expected_url_path) + assert expected_url in caplog.text