Skip to content

Conversation

@pared
Copy link
Contributor

@pared pared commented Dec 10, 2019

  • ❗ Have you followed the guidelines in the Contributing to DVC list?

  • πŸ“– Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.

  • ❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

@pared pared requested review from a user and Suor December 10, 2019 21:37
@efiop
Copy link
Contributor

efiop commented Dec 11, 2019

@pared Tests are failing.

@efiop efiop added refactoring Factoring and re-factoring testing Related to the tests and the testing infrastructure labels Dec 11, 2019
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be there already. If we are not then this is a bug in erepo_dir probably.

Copy link
Contributor

@Suor Suor Dec 12, 2019

Choose a reason for hiding this comment

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

erepo_dir is fixed here #2938, not merged yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is merged now.

Comment on lines 22 to 24
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's simply use literals and fspath() not str():

dvc.imp(fspath(erepo_dir), "foo", "foo_imported")

Comment on lines 26 to 27
Copy link
Contributor

Choose a reason for hiding this comment

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

assert os.path.isfile("foo_imported") should be enough, we don't need both.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we really want to check foo contents then it might be easier/more obvious to check text itself:

assert (tmp_dir / "foo").read_text() == "foo text" 

I am fine both ways though.

Comment on lines 54 to 55
Copy link
Contributor

Choose a reason for hiding this comment

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

assert (tmp_dir / "version").read_text() == "branch"

Also why do we need .exists() and .isfile() check?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you create new Repo while previously test used .dvc?

Copy link
Contributor

Choose a reason for hiding this comment

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

    erepo_dir.scm_add([...], commit="use symlinks")

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use scm directly, you also can use tmp_dir.scm_add(..., commit=...)

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@Suor leaved pretty good comments, nothing to add, but please address them

@Suor
Copy link
Contributor

Suor commented Dec 16, 2019

@pared did you forget about this one?

@pared
Copy link
Contributor Author

pared commented Dec 16, 2019

@Suor
Working on it, what blocks me is that
erepo_dir.dvc_gen is failing, due to the fact that at the time of test we are in test dir and not erepo. Trying to come up with a solution

@Suor
Copy link
Contributor

Suor commented Dec 16, 2019

@pared use :

with monkeypatch.context() as m:
    m.chdir(erepo_dir)

    erepo_dir.dvc_gen(...)

for now. I will think how to make it easier later.

@pared pared requested review from a user and Suor December 16, 2019 15:16
@efiop
Copy link
Contributor

efiop commented Dec 16, 2019

@pared Tests are failing.

@efiop
Copy link
Contributor

efiop commented Dec 17, 2019

@pared Tests are still failing

@efiop
Copy link
Contributor

efiop commented Dec 17, 2019

For the record: mac pkg test is failing due to unrelated reasons.

@pared Need to rebase though πŸ™‚

m.chdir(fspath(erepo_dir))
erepo_dir.dvc_gen("foo", "master content", commit="create foo")
erepo_dir.scm.checkout("new_branch", create_new=True)
erepo_dir.dvc_gen("foo", "branch content", commit="modify foo")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit complicated bootstrapping. We will need to come up with something better.

@efiop efiop merged commit 7531694 into iterative:master Dec 17, 2019
@pared pared deleted the 2896_import branch March 24, 2020 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring Factoring and re-factoring testing Related to the tests and the testing infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants