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

Use PyTest tmpdir fixture instead of manually handling them #123

Merged
merged 1 commit into from
Mar 13, 2018

Conversation

poush
Copy link
Contributor

@poush poush commented Mar 13, 2018

Fixes #70

@@ -10,20 +9,20 @@ def test_get_bugs():
bugs = utils.get_bugs()
assert(isinstance(bugs, list))

@pytest.fixture(scope='session')
Copy link
Owner

Choose a reason for hiding this comment

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

The tests are not fixtures

@@ -10,20 +9,20 @@ def test_get_bugs():
bugs = utils.get_bugs()
assert(isinstance(bugs, list))

@pytest.fixture(scope='session')
def test_mkdir(tmpdir_factory):
Copy link
Owner

Choose a reason for hiding this comment

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

Use tmpdir and the path as a string is tmpdir.strpath

Copy link
Owner

@marco-c marco-c left a comment

Choose a reason for hiding this comment

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

See review comments.

@poush
Copy link
Contributor Author

poush commented Mar 13, 2018

Updated. Please check.

utils.mkdir(direc_path)
def test_mkdir(tmpdir):
dir_path = tmpdir.strpath + "/test"
utils.mkdir(dir_path)
Copy link
Owner

Choose a reason for hiding this comment

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

Changing the name of the variable is unnecessary and leads to changes everywhere, it's better to leave it as it was

@@ -47,12 +45,12 @@ def test_read_labels():
assert(isinstance(labels, dict))


def test_write_labels():
@pytest.fixture(scope='session')
Copy link
Owner

Choose a reason for hiding this comment

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

Again, tests are not fixtures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

missed it. updated now. Please check.

file_path = d.name + "/test.csv"
utils.write_labels(label, file_name=file_path)
assert(os.path.exists(file_path))
fn = tmpdir.mkdir("data").join("test.csv")
Copy link
Owner

Choose a reason for hiding this comment

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

No need to create a new directory, use tmpdir directly.
Also, don't change the name of the variable



def test_load_image(tmpdir):
d = TemporaryDirectory()
fn = tmpdir.mkdir("data").join("Image.jpg")
Copy link
Owner

Choose a reason for hiding this comment

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

No need to create another directory, use tmpdir directly

@codecov-io
Copy link

codecov-io commented Mar 13, 2018

Codecov Report

Merging #123 into master will decrease coverage by 0.32%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
- Coverage   24.24%   23.91%   -0.33%     
==========================================
  Files          10       10              
  Lines         693      690       -3     
  Branches       84       84              
==========================================
- Hits          168      165       -3     
  Misses        524      524              
  Partials        1        1
Impacted Files Coverage Δ
tests/test_utils.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0528edf...d6fdf8a. Read the comment docs.

@poush poush force-pushed the fix70 branch 3 times, most recently from 96f3d4c to d6fdf8a Compare March 13, 2018 17:40
Copy link
Owner

@marco-c marco-c left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good now!

@marco-c marco-c merged commit 1e864c7 into marco-c:master Mar 13, 2018
@propr
Copy link

propr bot commented Mar 13, 2018

Please provide your feedback on this pull request here.

Privacy statement: We don't store any personal information such as your email address or name. We ask for GitHub authentication as an anonymous identifier to account for duplicate feedback entries and to see people specific preferences.

@poush poush deleted the fix70 branch March 13, 2018 19:30
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