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

kdexd · 2016-12-09T17:15:21Z

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

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

Prior to this PR, each method had several yield based statements where the test cases comprised of a set of functions with args and kwargs as well. I have proposed a fixture which returns a dictionary of functions to make parametrization easier.
with_numpy decorator is now a pytest.skipif marker [Commented for now, will be uncommented after test_hashing.py is pytestified. Because this skipif marker does not work well with yield based tests.]
The function names g and f2 have been interchanged. Earlier g was decorated with @mem.cache earlier, and it was the only one function. All other functions - f, f2, h, i, j, k were normal undecorated ones. This name interchanging is simply a cosmetic change.

kdexd · 2016-12-09T17:16:09Z

@lesteve It is WIP but since the diff is small, it might be easier for review, feel free to review it whenever you wish. 😄

lesteve · 2016-12-09T17:44:05Z

joblib/test/test_func_inspect.py

-    yield assert_equal, filter_args(i, [], (2, )), {'x': 2}
-    yield assert_equal, filter_args(f2, [], (), dict(x=1)), {'x': 1}
+@parametrize(['funcname', 'args', 'filtered_args'],
+             [('f', [[], (1, )], {'x': 1, 'y': 0}),


Why not use functions in parametrize?

@parametrize(['func', 'args', 'filtered_args'], [(f, [[], (1, )], {'x': 1, 'y': 0}), ...

I guess the only good reason to use a fixture for funcs is actually the cached function f2, because it depends on tmpdir, right?

Yes, because f2 depends on tmpdir. So would it be great if f2 is enclosed in a fixture while other functions just stay there in global scope ?

kdexd · 2016-12-09T18:05:08Z

I am rolling back to old with_numpy because the skipif marker will not work with yield based tests yet - as in in failing job of Travis CI. But only four such tests exist in test_hashing.py so I will bring this back in immediately next PR.

lesteve · 2016-12-12T10:51:21Z

joblib/test/test_func_inspect.py

-@mem.cache
 def g(x):
-    return x
+    pass


Why this change?

I interchanged names g and f2 just because f2 was the odd one out. Maybe it got missed out.

Nope, this one is correct. Earlier it was:

def f2(x): pass @mem.cache def g(x): return x

Just the names g and f2 were interchanged because f2 was the odd one out (decorated).

I see. In general the less you change that is not directly related to the PR the easier it is to review. When you notice unrelated shortcomings/problems while working on a PR, it is a good habit to change them in separate PRs.

Since you are changing it anyway, can you use a better name than f2 then to make it clearer than it is a cached function? Maybe something like cached_func?

Since you are changing it anyway, can you use a better name than f2 then to make it clearer than it is a cached function? Maybe something like cached_func?

I think this is the last thing remaining and then this will be good to merge.

When you notice unrelated shortcomings/problems while working on a PR, it is a good habit to change them in separate PRs.

I agree with this fact.
Okay, I'll make it cached_func.

lesteve · 2016-12-12T10:51:46Z

joblib/test/test_func_inspect.py

 from joblib.memory import Memory
 from joblib.test.common import with_numpy
-from joblib.testing import assert_equal, assert_raises_regex, assert_raises
+from joblib.testing import (fixture, parametrize, pytest_assert_raises)


You don't need the parentheses if the imports fit on one line

lesteve · 2016-12-12T10:53:05Z

joblib/test/test_func_inspect.py

+               {'x': 1, 'y': 0, '*': [], '**': {}}),
+              (h, [[], (1, 2, 3, 4)],
+               {'x': 1, 'y': 2, '*': [3, 4], '**': {}}),
+              (h, [[], (1, 25), dict(ee=2)],


For consistency you may as well use {'ee': 2}. To be honest I am not sure why dict is used maybe some people find it more explicit.

If you do that do it everywhere in this file.

lesteve · 2016-12-12T10:58:03Z

joblib/test/test_func_inspect.py

+@fixture(scope='module')
+def f2(tmpdir_factory):
+    """
+    This fixture returns a dict of functions which are used as test cases


This docstring is lying.

lesteve · 2016-12-12T10:58:56Z

joblib/test/test_func_inspect.py

+
+
+def test_func_name_on_inner_func(f2):
+    # Check that we are not confused by decoration


I don't understand why the "not confused" test was on g before and is on f2 now.

g was decorated with @mem.cache earlier, and it was the only one function. All other functions - f, f2, h, i, j, k were normal ones. That seemed a bit off to me to I went ahead and made this change.

I added this note in main PR description.

kdexd · 2016-12-12T12:06:32Z

@lesteve All work done here I guess. New PR on the way once this is merged..

lesteve · 2016-12-12T13:25:01Z

joblib/test/test_func_inspect.py

+def f2(tmpdir_factory):
+    # Create a Memory object to test decorated functions.
+    # We should be careful not to call the decorated functions, so that
+    # cache directories are not created in the temp dir.


I have to say I don't really understand this comment but it was there before ...

Agreed. I did not alter it because it was there...

I think I got it eventually: we should only use f2 for testing its signature. If you call it you will pollute the temporary cache directory. The fact that it is a fixture makes it even more complicated to understand the comment I think ...

I may clean this up a bit after this PR is merged.

lesteve · 2016-12-12T13:29:46Z

joblib/test/test_func_inspect.py

+              ])
+def test_filter_args_error_msg(exception, regex, func, args):
+    """
+    Make sure that filter_args returns decent error messages, for the sake


Was this docstring too long? It it was try to follow PEP0258. Short description one the first line and then a blank line and then long description.

Oh okay I did not remember this PEP. I usually write them throughout the project in this manner so I wrote them here in this way unconsciously.. I'll rectify them in a jiffy.

For the record, in general we try to follow the Numpy style for docstrings: see https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt.

Okay, thanks for informing the style guidelines. Will be helpful ! (pins this link to browser ).

I modified the docstring..

lesteve · 2016-12-12T13:31:25Z

joblib/test/test_func_inspect.py

-def test_format_signature():
+@parametrize(['func', 'args', 'kwargs', 'sgn_expected'],
+             [(g, [list(range(5))], {}, 'g([0, 1, 2, 3, 4])'),
+              (k, [1, 2, (3, 4)], {'y': True}, 'k(1, 2, (3, 4), y=True)')])


Your changes are fine, but remember what I said above about not changing unrelated things if there is not a strong motivation behind it.

It was just to keep things within 80 characters. I would remember about the fact, keeping in mind that there might be some piece of code which would be written a long time ago due to some reason. 😄

lesteve · 2016-12-12T14:27:01Z

LGTM, merging, thanks a lot!

kdexd · 2016-12-12T14:29:27Z

Okay I'm leaving a long comment on the issue thread, please refer to it 😄

Here: #411 (comment)

kdexd force-pushed the pytestify-test-func-inspect branch from 95e6adf to 2e86b6b Compare December 9, 2016 17:17

lesteve reviewed Dec 9, 2016

View reviewed changes

Karan Desai added 6 commits December 9, 2016 23:15

Convert f and g functions into module scoped fixtures.

f89a26c

Parametrize all tests about filter args.

82e6e74

Parametrize test_filter_args_error_msg with suitable regexp.

dffe55f

Convert with_numpy decorator func to skipif marker.

9f581c3

Parametrize test_format_signature.

32f9f6a

Replace assert_raises_regex with pytest_assert_raises.

4b36e89

kdexd force-pushed the pytestify-test-func-inspect branch from 4dfa484 to 4b36e89 Compare December 9, 2016 17:45

kdexd changed the title ~~[WIP] Refactor test_disk.py as per pytest design.~~ [WIP] Refactor test_func_inspect.py as per pytest design. Dec 9, 2016

Karan Desai added 3 commits December 9, 2016 23:39

Restore back old with_numpy decorator for a while.

8b88932

Breakdown func fixture to individual functions again.

7627680

Uncomment get_func_name again.

1028b46

kdexd changed the title ~~[WIP] Refactor test_func_inspect.py as per pytest design.~~ [MRG] Refactor test_func_inspect.py as per pytest design. Dec 10, 2016

lesteve reviewed Dec 12, 2016

View reviewed changes

Cosmetic changes in import statements and dict literals.

f3e435c

lesteve reviewed Dec 12, 2016

View reviewed changes

Karan Desai added 2 commits December 12, 2016 19:08

COSMIT small docstring modification.

f8615d8

Rename f2 => cached_func.

5a3fc93

lesteve merged commit 6aa3fa4 into joblib:master Dec 12, 2016

kdexd deleted the pytestify-test-func-inspect branch December 12, 2016 14:29

kdexd mentioned this pull request Dec 12, 2016

[MRG] Refactor test_logger.py as per pytest design. #453

Merged



		def test_func_name_on_inner_func(f2):
		# Check that we are not confused by decoration

Conversation

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

Uh oh!

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

Uh oh!

kdexd commented Dec 9, 2016

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!

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!

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

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 12, 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!

lesteve commented Dec 12, 2016

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kdexd commented Dec 9, 2016 •

edited

Loading

lesteve Dec 12, 2016 •

edited

Loading

kdexd commented Dec 12, 2016 •

edited

Loading