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

test: refactor tmp dir helper fixtures #2868

Merged
merged 3 commits into from
Dec 5, 2019
Merged

Conversation

Suor
Copy link
Contributor

@Suor Suor commented Nov 29, 2019

  • tmp_dir descends from Path
  • convenient template methods to generate files and dirs, add them to
    dvc and git and commit to git
  • independent of old basic_env structures
  • can go in any order
  • modular: start with empty tmp_dir, initialize git with scm, dvc
    repo with dvc add "repo template" with repo_template, ...

Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

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

Great stuff @Suor. Some minor comments.

Additionaly,
what do you think about removing
repo_template in favor of more granular elements?
For example:

FOO_TEMPLATE = {"foo": "foo"}
@pytest.fixture
def foo(tmp_dir):
    tmp_dir.gen(FOO_TEMPLATE)
    return "foo"

and in test:

def test_add_file(dvc, foo):
    stage, =dvc.add(foo)
    assert os.path.exists(stage.relpath)

also,
using name dvc prevents us from general import dvc. I don't think it should be bloker in any test, as it seems to me that all usages of import dvc are related to patching, and can be replaced with more detailed import. So it is just a comment.

tests/func/test_add.py Outdated Show resolved Hide resolved
tests/func/test_add.py Show resolved Hide resolved
tests/func/test_api.py Outdated Show resolved Hide resolved
tests/func/test_api.py Outdated Show resolved Hide resolved
tests/func/test_api.py Show resolved Hide resolved
tests/func/test_api.py Show resolved Hide resolved
@Suor
Copy link
Contributor Author

Suor commented Dec 2, 2019

@pared The idea is that helpers will make generating a file so easy that you won't require files precreated with fixtures, which distribute logic over code. In your foo example:

def test_add_file(tmp_dir, dvc):
    stage, = temp_dir.dvc_gen("foo")
    assert os.path.exists(stage.relpath)

So foo fixture saves 0 lines.

tests/dir_helpers.py Outdated Show resolved Hide resolved
Copy link
Member

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Neat!

@Suor
Copy link
Contributor Author

Suor commented Dec 2, 2019

... (sorry for breaking this)...

This way you may use specific telling names for files, e.g. not reuse code.py as some pregitadded file. Bootstrap specific file structure and play with it in a single place. This should make tests more self-contained and hence more readable.

Getting rid of repo_template might be even better, but it looks like it makes harder to migrate. I also wanted to show it to you, guys. So this remains a design question, do we need to get rid of template?

Suor and others added 3 commits December 2, 2019 23:31
- `tmp_dir` descends from Path
- convenient template methods to generate files and dirs, add them to
dvc and git and commit to git
- independent of old basic_env structures
- can go in any order
- modular: start with empty `tmp_dir`, initialize git with `scm`, dvc
repo with `dvc` add "repo template" with `repo_template`, ...
Co-Authored-By: Ruslan Kuprieiev <kupruser@gmail.com>
Co-Authored-By: Paweł Redzyński <pawelredzynski@gmail.com>
@Suor
Copy link
Contributor Author

Suor commented Dec 2, 2019

@MrOutis may you take a look?

'Did you forget to use "{name}" fixture?'.format(name=name)
)

def gen(self, struct, text=""):
Copy link

Choose a reason for hiding this comment

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

really neat; hard to read at the beginning but the interface is awesome 😍
would you mind documenting the intended usage, @Suor? (a quick summary on top of the module is enough))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good idea.

@ghost
Copy link

ghost commented Dec 3, 2019

@Suor , what's currently missing from the implementation? (asking because of WIP in title)

@Suor
Copy link
Contributor Author

Suor commented Dec 3, 2019

@MrOutis It could be merged like this. We will, however, get 3 types of tests this way: unittests, pytest old, pytest new. I thought migrating the rest of the pytes tests first. The thing is the longer this PR lingers here the harder it would be to rebase it. So maybe merging it as is and migrating the rest of the pytest-style tests in a subsequent PR would be easier. What do you think @efiop?

@efiop
Copy link
Member

efiop commented Dec 3, 2019

@Suor A bit hesitant because of those reasons, but it is indeed reasonable. Let's do that, but let's also put the converting on the next sprint, so that we don't leave the mess for long. Are you willing to give it shot? 🙂

@Suor
Copy link
Contributor Author

Suor commented Dec 3, 2019

@efiop I am in.

@efiop
Copy link
Member

efiop commented Dec 3, 2019

@Suor Sounds good :)

@Suor
Copy link
Contributor Author

Suor commented Dec 5, 2019

So, should we merge it? I can add a usage module doc-string in a separate PR.

@efiop
Copy link
Member

efiop commented Dec 5, 2019

@Suor At least need to remove WIP suffix, I'm mostly waiting for that.

@Suor Suor changed the title [WIP] test: refactor tmp dir helper fixtures test: refactor tmp dir helper fixtures Dec 5, 2019
@Suor
Copy link
Contributor Author

Suor commented Dec 5, 2019

Done;

@Suor Suor mentioned this pull request Dec 5, 2019
3 tasks
@efiop efiop merged commit c328075 into iterative:master Dec 5, 2019
@efiop
Copy link
Member

efiop commented Dec 5, 2019

@Suor Please create a ticket for the further steps and add it to our sprint, so we can keep track of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants