tests: several tests migrated unittest to pytest

[nik123]

Part of #1819

Changes:

1. Removed TestGit in basic_env.py.
2. Several unittest.TestCase subclasses in func/test_odb.py migrated to pytest: TestCacheLoadBadDirCache, TestExternalCacheDir, TestSharedCacheDir, TestCacheLinkType, TestCmdCacheDir
3. Several unittest.TestCase subclasses in func/test_fs.py migrated to pytest: TestMtimeAndSize, TestContainsLink,

• ❗ I have followed the Contributing to DVC checklist.

I have followed the checklit.

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

This PR doesn't require documentation updates.

[pmrowla]

Thanks for the PR @nik123

The changes relating to moving from unittest to pytest look good, but a lot of the tests should also be updated to use the tmp_dir/dvc/scm fixtures correctly. Basically, in the old tests there are a lot of places where main() is used, but we can now just use the pytest fixtures instead.

I noticed that you did replace the TestDvc.create() calls with tmp_dir.gen() which is a good start, but in a lot of cases there are more lines that can also be replaced.

For example

tmp_dir.gen("foo", "foo")
ret = main(["add", "foo"])
assert 0 == ret

can be replaced with just

tmp_dir.dvc_gen("foo", "foo")

(dvc_gen is tmp_dir.gen() that automatically dvc adds everything as well

I marked a few cases that I noticed during the review, but please double check the tests you've migrated for similar changes that could be made.

[pmrowla]

This should no longer be needed with the dvc fixture

[nik123]

@pmrowla - I've removed dvc.__init__ call and main calls in this test. But I had to also add dvc.odb = ODBManager(dvc) because dvc.config updates are not applied to dvc.odb.config. If there is a better way to update odb, please tell me.

[pmrowla]

This test should really be re-written to use the tmp_dir fixture properly. It also does not need the scm or dvc fixtures, since the test itself is initializing DVC repos with --no-scm.

So the test should be something like:

def test_shared_cache_dir(tmp_dir):
    cache_dir = tmp_dir / "cache"
    for d in ["dir1", "dir2"]:
        makedirs(tmp_dir / d)
        with (tmp_dir / d).chdir():
            # do the rest of the test operations
            ...
    assert not (tmp_dir / "dir1" /  ".dvc" / "cache").exists()
    assert not (tmp_dir / "dir2" /  ".dvc" / "cache").exists()
    ...

We can also just use (tmp_dir / d).gen(...) instead of the existing with open(...): write(...) calls as well

[nik123]

The changes relating to moving from unittest to pytest look good, but a lot of the tests should also be updated to use the tmp_dir/dvc/scm fixtures correctly. Basically, in the old tests there are a lot of places where main() is used, but we can now just use the pytest fixtures instead.

@pmrowla I've replaced main calls with fixtures with two exceptions:

1. test_shared_cache_dir
2. Tests which explicitly declare cmd call in their name: i.e. test_cmd_cache_dir, test_cmd_cache_abs_path because their names suggest that command (i.e. subclass of CmdBase) should be called.

[pmrowla]

@nik123 thanks for making the changes.

I think it looks ok now, could you please squash your commits together? We have started merging rebased/linear history into the main DVC branch (rather than squashing all PRs into single commits).

In this case, it should be fine to squash down to a single tests: migrate unittest to pytest commit (instead of the several repeated Update tests/func/test_odb.py etc. commits).

[nik123]

I think it looks ok now, could you please squash your commits together? We have started merging rebased/linear history into the main DVC branch (rather than squashing all PRs into single commits).

In this case, it should be fine to squash down to a single tests: migrate unittest to pytest commit (instead of the several repeated Update tests/func/test_odb.py etc. commits).

@pmrowla Not sure if I understood you right but I've squashed all commits in unittest-to-pytest into single commit, rebased unittest-to-pytest on main and then force-pushed my branch.

BTW could you please elaborate why you stopped using squashing?

[pmrowla]

@pmrowla Not sure if I understood you right but I've squashed all commits in unittest-to-pytest into single commit, rebased unittest-to-pytest on main and then force-pushed my branch.

That's fine, thanks

BTW could you please elaborate why you stopped using squashing?

Because for larger PR's, it makes sense to merge multiple commits from the PR as separate commits instead of one giant squashed commit. This makes things like doing git bisect on main more useful since if a PR breaks something it is easier to narrow down what specific change from a PR introduced the bug.

Basically, for small/straightforward PR's, like this one, we will generally still want to squash them into a single commit, but it's now on the devs to do it selectively when it makes sense, as opposed to always squashing PR into a single commit by default.

[pmrowla]

Thanks again @nik123 🙏