Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unittest to pytest: moved few unittest tests to pytest #6379

Merged
merged 5 commits into from Aug 4, 2021

Conversation

nik123
Copy link
Contributor

@nik123 nik123 commented Aug 2, 2021

Part of #1819

PR doesn't require documentation updates.

@nik123 nik123 requested a review from a team as a code owner August 2, 2021 17:49
@nik123 nik123 requested a review from isidentical August 2, 2021 17:49
tmp_dir.gen({"foo": "foo"})
scm.add(["foo"])
scm.commit("add")
assert "foo" in Repo(tmp_dir).git.ls_files()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this one.

scm.is_tracked("foo")

instead of

    assert "foo" in Repo(tmp_dir).git.ls_files()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've replaced ls_files with is_tracked. But I'm not sure if test_commit is really needed because it seems that test_is_tracked fully covers test_commit case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the reason that this did not use is_tracked before is that is_tracked is still using our SCM class, and the tests for is_tracked itself also depends on the assumption that scm.commit works (so we want to test scm.commit independently of is_tracked.

To verify that scm.commit works, we were testing it directly using CLI git before (the git.ls_files call spawns an actual CLI git ls-files call via gitpython).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. Now test_commit makes sense.

self._test_init()
def test_api_init(scm):
DvcRepo.init().close()
_check_dirs()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the check for isdir should be enough, so I think it'd be more clearer if we get rid of _check_dirs and replace them with os.path.isdir(repo.DVC_DIR). 1 less indirection.

tests/func/test_scm.py Outdated Show resolved Hide resolved
Co-authored-by: Peter Rowlands (변기호) <peter@pmrowla.com>
@efiop
Copy link
Member

efiop commented Aug 4, 2021

Thank you, @nik123 ! 🙏

@efiop efiop merged commit 3be395b into iterative:master Aug 4, 2021
@efiop efiop added testing Related to the tests and the testing infrastructure maintenance labels Aug 10, 2021
@nik123 nik123 deleted the unittest-to-pytest branch August 31, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance testing Related to the tests and the testing infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants