diff: fix missing key bug (#6720)

[dberenbaum]

• ❗ I have followed the Contributing to DVC checklist.

• [N/A] 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

dvc diff fails when there is no diff and hide_missing is true (hide_missing is implied when comparing two revisions) because CmdDiff.run tries to delete the not in cache info from an empty dict. It seemed easy enough to fix.

Not sure if we need a test for this. Couldn't find any relevant ones specific to the CmdDiff logic.

[daavoo]

Not sure if we need a test for this. Couldn't find any relevant ones specific to the CmdDiff logic.

Could be something like:

https://github.com/iterative/dvc/blob/main/tests/unit/command/test_diff.py#L163

[dberenbaum]

Thanks @daavoo! Added a test.

[skshetry]

Please use pytest.mark.parametrize in test_no_changes.

[skshetry]

You can change the above test test_no_changes to something like following:

@pytest.mark.parametrize("opts", ([], ["a_rev", "b_rev"], ["--hide-missing"]))
def test_no_changes(mocker, capsys, opts):
    args = parse_args(["diff", "--json", *opts])
    cmd = args.func(args)
    mocker.patch("dvc.repo.Repo.diff", return_value={})

    assert 0 == cmd.run()
    out, _ = capsys.readouterr()
    assert "{}" in out

    args = parse_args(["diff", *opts])
    cmd = args.func(args)
    assert 0 == cmd.run()
    out, _ = capsys.readouterr()
    assert not out

[dberenbaum]

Sorry, just got a little lazy/sloppy here trying to fix this as quickly as possible.

[dberenbaum]

@skshetry Updated the tests with parametrized options and output formats.

[dberenbaum]

@skshetry Pinging on this. Could you review the updated tests when you have a chance? 🙏

[skshetry]

I'd prefer fixing this in repo.diff() to always return the same schema even if things didn't change.
So, instead of an empty dictionary, it should return this:

>>> repo.diff()
{
    'added': [],
    'deleted': [],
    'modified': [],
    'renamed': [],
    'not in cache': []
}

[dberenbaum]

@skshetry Up to you. I went with the quickest fix since it seemed like a small enough issue to resolve quickly in CmdDiff without impacting repo.diff. Future changes to repo.diff could implement a consistent schema, but any reason not to handle this in CmdDiff also?