[MRG] Refactor test_disk.py as per pytest design. by kdexd · Pull Request #446 · joblib/joblib

kdexd · 2016-12-07T15:57:00Z

Third Phase PR on #411 (Succeeding PR #441)

This PR deals with refactor of tests in test_disk.py to adopt the pytest flavor.

assert_raises now derived from pytest because it removes two redundant layers from traceback. In a deliberately created failure, pytest.raises only highlights the last two lines in this traceback as opposed to unittests traceback (the complete one)...

/usr/lib/python3.5/unittest/case.py:727: in assertRaises
    return context.handle('assertRaises', args, kwargs)
/usr/lib/python3.5/unittest/case.py:176: in handle
    callable_obj(*args, **kwargs)
joblib/disk.py:53: in mkdirp
    os.makedirs(d)

Test cases have been separated out and parametrized using a decorator.

kdexd · 2016-12-07T15:59:16Z

conftest.py will grow over time, probably we may end up emptying up joblib/test/common.py as well, if need be..

kdexd · 2016-12-08T11:55:12Z

@lesteve Please review this code whenever you feel convenient 😄

lesteve · 2016-12-08T15:37:41Z

joblib/test/test_disk.py

+              ('1.4M', int(1.4 * 1024 ** 2), None),
+              ('120M', 120 * 1024 ** 2, None),
+              ('53K', 53 * 1024, None),
+              ('fooG', None, ValueError)])


Put the exception in a separate test function this way test_mem_str_to_bytes can be simpler.

lesteve · 2016-12-08T15:38:48Z

joblib/testing.py

 assert_equal = _dummy.assertEqual
 assert_not_equal = _dummy.assertNotEqual
-assert_raises = _dummy.assertRaises
+assert_raises = pytest.raises


I think I said somewhere else that I would have a different name to make it clear that it is not the stldlib assert_raises, maybe pytest_assert_raises.

Is there an advantage of using pytest.raises in the tests affected by this PR?

For the record, my personal preference is to always use assert_raises_regex in order to check the error message. I have seen cases where the test pass but the exception is raised for a completely different reason.

pytest has some way with a context manager of checking the error message by the way.

pytest has some way with a context manager of checking the error message by the way.

Yeah quite convenient, have a look

lesteve · 2016-12-08T15:50:52Z

joblib/test/conftest.py

+from joblib.testing import fixture
+
+
+@fixture(scope='function')


Just checking we don't actually need to write a fixture, provided we use tmpdir as the test function arguments and tmpdir.strpath in the test function body right?

Maybe this solution would have my preference.

Sure, I was fine the either way, although at one point I thought tmpdir.strpath would be a bit clumsy everywhere.

I think what this is is me, as a new pytest user, being a bit wary about pytest magic ... Leave it like this is fine.

lesteve · 2016-12-08T16:18:07Z

joblib/test/test_disk.py

-    mkdirp(os.path.join(tmpdir_path, 'ham'))
-    mkdirp(os.path.join(tmpdir_path, 'ham'))
-    mkdirp(os.path.join(tmpdir_path, 'spam', 'spam'))
+@parametrize(['text', 'exception', 'exc_substr'],


You don't need the parametrize here anymore, right?

I mean you are parametrizing over one item so there is no need to parametrize and you can use local variables into a standard test function.

Please refer this thread Coincidentially we wrote our comments at the same moment 😄

kdexd · 2016-12-08T16:18:42Z

joblib/test/test_disk.py

+def test_memstr_to_bytes_exception(text, exception, exc_substr):
+    with pytest_assert_raises(exception) as excinfo:
+        memstr_to_bytes(text)
+    assert exc_substr in str(excinfo.value)


The "pytest" way of assert_raises_regex. Also there's just one test case but it looks better this way in my opinion.

You don't need the parametrize here anymore, right?

@lesteve I was writing this comment simultaneously, well in variable names exception, text and exc_substr make the test body very generic. I chose to write it this way because of the way the method just above this was written in a similar manner. No objections in modifying it though, shall I ?

Also, looking at the code of memstr_to_bytes it looks like the ValueError can be raised both by a prior KeyError as well as a ValueError. Maybe a good time to test it properly by corresponding two test cases ? Or is it not that significant ?

lesteve · 2016-12-08T16:27:44Z

joblib/test/test_disk.py

+def test_memstr_to_bytes_exception(text, exception, exc_substr):
+    with pytest_assert_raises(exception) as excinfo:
+        memstr_to_bytes(text)
+    assert exc_substr in str(excinfo.value)


You can use excinfo.match(r'.* 123 .*') according to http://doc.pytest.org/en/latest/assert.html. It is general a good idea to make sure that the value the user provided is given in the error message. Here you could check 'Invalid literal for size.*fooG'.

kdexd · 2016-12-08T16:37:14Z

joblib/test/test_disk.py

+
+@parametrize(['text', 'exception', 'regex'],
+             [('fooG', ValueError, r'Invalid literal for size.*fooG.*'),
+              ('1.4N', ValueError, r'Invalid literal for size.*1.4N.*')])


try: size = int(units[text[-1]] * float(text[:-1])) except (KeyError, ValueError): raise ValueError( "Invalid literal for size give: %s (type %s) should be " "alike '10G', '500M', '50K'." % (text, type(text)))

So to test this block of memstr_to_bytes I added one more test case. First case causes ValueError before landing in except block while second causes KeyError.

Just a nitpick from my side...

I can tell you were just looking for an excuse to keep the parametrize ;-).

Haha you got me xD anyways jokes apart, I can revert back if you wish..

kdexd · 2016-12-09T02:44:56Z

@lesteve The failing job on Travis CI uses pytest==2.7.2 and raises an AttributeError that ExceptionInfo has no attribute named match because that feature was introduced in pytest==3.0.0 as per their CHANGELOG. I guess there's one solution, pin pytest to one version in both packaging as well as testing.

lesteve · 2016-12-09T11:22:17Z

@lesteve The failing job on Travis CI uses pytest==2.7.2 and raises an AttributeError that ExceptionInfo has no attribute named match because that feature was introduced in pytest==3.0.0 as per their CHANGELOG.

The reason for that is that conda has some dependency constraints that are not up-to-date for python 3.3. Pytest 3 works perfectly well with python 3.3 AFAIK. I am going to install pytest via pip and that should fix this problem.

to make sure we have a pytest version >=3 even on Python 3.3

lesteve · 2016-12-09T11:31:23Z

Side note we will probably drop support for Python 3.3 in the not too distant future.

kdexd · 2016-12-09T12:32:41Z

The reason for that is that conda has some dependency constraints that are not up-to-date for python 3.3. Pytest 3 works perfectly well with python 3.3 AFAIK.

Yes you are correct, pytest works fine with python 3.3. I was puzzled by the fact that always pytest==2.7.2 was installed by default in that particular job. Never hit me that I might have been conda...

I am going to install pytest via pip and that should fix this problem.

Yeah that fixed it, great ! Thanks for the help 😄

lesteve · 2016-12-09T14:47:10Z

OK merging this one, thanks a lot!

Feel free to reintroduce the tmpdir_path fixture in conftest.py in your next PR.

kdexd · 2016-12-09T16:53:22Z

Great, thanks for the continuous review feedbacks ! As usual, next PR on the way...

GaelVaroquaux · 2016-12-09T17:01:13Z

Thanks Karan, it's great to have you on board.

Karan Desai added 4 commits December 7, 2016 20:48

Use pytests's tmpdir fixture in test_disk_used.

cde9ad1

Parametrize test_memstr_to_bytes.

e15ce46

Use pytest's tmpdir fixture in test_mkdirp.

495b5b0

Move tmpdir_path fixture from test_hashing.py to conftest.py

52424fe

kdexd changed the title ~~Refactor test_disk.py as per pytest design.~~ [MRG] Refactor test_disk.py as per pytest design. Dec 7, 2016

lesteve reviewed Dec 8, 2016

View reviewed changes

Karan Desai added 2 commits December 8, 2016 21:43

Separate exception assertion out from test_memstr_to_bytes.

357ae20

Use tmpdir fixture instead of tmpdir_path in test_disk.py

14709e4

lesteve reviewed Dec 8, 2016

View reviewed changes

kdexd commented Dec 8, 2016

View reviewed changes

lesteve reviewed Dec 8, 2016

View reviewed changes

kdexd force-pushed the pytestify-test-disk branch from dfad634 to 991e36d Compare December 8, 2016 16:34

Modify test_memstr_to_bytes_exception.

d3cae30

kdexd force-pushed the pytestify-test-disk branch from 991e36d to d3cae30 Compare December 8, 2016 16:35

kdexd commented Dec 8, 2016

View reviewed changes

TRAVIS always install pytest through pip

9d51d38

to make sure we have a pytest version >=3 even on Python 3.3

lesteve merged commit d3b5388 into joblib:master Dec 9, 2016

kdexd deleted the pytestify-test-disk branch December 9, 2016 16:52

kdexd mentioned this pull request Dec 9, 2016

[MRG] Refactor test_func_inspect.py as per pytest design. #450

Merged

		from joblib.testing import fixture


		@fixture(scope='function')

Conversation

kdexd commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Third Phase PR on #411 (Succeeding PR #441)

Uh oh!

kdexd commented Dec 7, 2016

Uh oh!

kdexd commented Dec 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdexd commented Dec 9, 2016

Uh oh!

lesteve commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kdexd commented Dec 9, 2016

Uh oh!

lesteve commented Dec 9, 2016

Uh oh!

kdexd commented Dec 9, 2016

Uh oh!

GaelVaroquaux commented Dec 9, 2016 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kdexd commented Dec 7, 2016 •

edited

Loading

lesteve commented Dec 9, 2016 •

edited

Loading

lesteve commented Dec 9, 2016 •

edited

Loading