plots/metrics: fix some communication disrepancies #4837
Conversation
pared
force-pushed
the
4446_malformed_parse
branch
from
e995b29 to
896a49c
Compare
There was a problem hiding this comment.
Removed tests have been checking the type of error thrown. Considering the whole unification issue from the user perspective, it makes more sense to me to check whether interaction with the user is properly structured, and not whether the proper exception type has been thrown. That is why I decided to use main in test_common tests and check for particular error messages in caplog.
| def test_show_empty(dvc): | ||
| with pytest.raises(NoMetricsError): | ||
| dvc.metrics.show() | ||
|
|
||
|
|
There was a problem hiding this comment.
Moved to test_common.py::test_show_no_metrics_files
| def test_no_plots(tmp_dir, dvc): | ||
| from dvc.exceptions import NoPlotsError | ||
|
|
||
| with pytest.raises(NoPlotsError): | ||
| dvc.plots.show() |
There was a problem hiding this comment.
Moved to test_common.py::test_show_no_metrics_files
| def test_show_invalid_metric(tmp_dir, dvc, run_copy_metrics): | ||
| tmp_dir.gen("metrics_temp.yaml", "foo:\n- bar\n- baz\nxyz: string") | ||
| run_copy_metrics( | ||
| "metrics_temp.yaml", "metrics.yaml", metrics=["metrics.yaml"] | ||
| ) | ||
| with pytest.raises(NoMetricsError): | ||
| dvc.metrics.show() |
There was a problem hiding this comment.
Moved to test_common.py::test_show_malformed_metric.
pared
changed the title
| @@ -73,7 +73,7 @@ def run(self): | |||
| ) | |||
|
|
|||
| except DvcException: | |||
| logger.exception("") | |||
| logger.exception("failed to show plots") | |||
There was a problem hiding this comment.
Sorry for a post-merge review, but we are going away from these obvious messages and deleting them everywhere. Are you sure we need it here?
There was a problem hiding this comment.
not necessarily, I added it only to make it consistent with metrics - I guess we could remove it in both places. How about that?
There was a problem hiding this comment.
@pared Yeah, better to remove in both cases.
| assert ( | ||
| f"No {command} files in this repository. " | ||
| f"Use `{run_options}` options for " | ||
| f"`dvc run` to mark stage outputs as {command}." | ||
| ) in caplog.text |
There was a problem hiding this comment.
Since this is really an exception that you are testing, it would be better to test it without going through CLI, as we did before. Just a bit cleaner. Same with tests above.
skshetry
added
refactoring
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
Related to #4446