[MRG] Refactor test_memory.py as per pytest design. #466

kdexd · 2016-12-21T03:13:54Z

Third Phase (Extended) PR on #411 ( Replacement of #460 , Succeeding #465 )

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

Create two new fixtures memory and no_memoize_memory as they are used by almost all of the tests.
Remove memory clearing calls at certain places, because earlier temporary directory was created / destroyed manually. Now pytest's tmpdir already ensures isolation with other tests.
Parametrize tests wherever it seems straightforward.

kdexd · 2016-12-21T03:28:09Z

Strange, undefined name 'func_with_kwonly_args - this PEP8 was never raised earlier, I doubt why now ? This has happened in some other module as well.

kdexd

@lesteve: I introduced a new design in 3e2878d . Last time both of us agreed that indirect parametrization looks a little weird so I didn't do it here.

Pytest deals with @parametrize decorator during test collection, while the fixtures are executed and provided when first asked for. Hence using a fixture (custom / provided by pytest ) is not permitted inside @parametrize decorator.

So for such requirements, or any other case where even setting up the test cases need precalculation, all of it can be done in a fixture. This way of parametrization is known as parametrizing through a fixture, where I set params=range(4) showing that any test using this fixture will be invoked 4 times with different setups provided by this fixture.

Each each, this fixture returns one tuple from the list of testcases, all this is handled by pytest's own request fixture. So in four invocations, request.param takes values from params argument (i.e. 0, 1, 2, 3).

I don't know whether such feature existed in nose, so this type of design might look new to people who have been using nose so far. Although pytest users would say this is standard.

Reference: http://doc.pytest.org/en/latest/fixture.html#parametrizing-fixtures

What are your thoughts about introducing such design to different tests ? I am in favour of keeping things uniform everywhere - either this new design or the older, so there are a lot of places where this design can be implemented, and if not then they should be done nowhere.

lesteve · 2016-12-21T10:11:07Z

joblib/test/test_memory.py

+# Module-level functions and fixture, for tests
+@fixture(scope='function')
+def memory(tmpdir):
+    return Memory(cachedir=tmpdir.strpath, mmap_mode=None, verbose=0)


Not convinced by the memory and no_memoize_memory fixture, they only replace a one-line statement and add a layer of indirection. I feel it is better in this case to be explicit and go for simplicity.

@lesteve Reverting back.

lesteve · 2016-12-21T10:11:13Z

joblib/test/test_memory.py

    g = memory.cache(f)
    gp = pickle.loads(pickle.dumps(g))
    gp(1)


-def test_call_and_shelve(tmpdir):
+@fixture(scope='function', params=range(4))


I find it a bit disappointing that there is no better way than this. The thing is that you could actually mess up very easily by adding elements in func and Result and thinking that your test pass although it is just that you forgot to update params.

True that, there's one method through indirect parametrization, but I guess we'll have to wait for a while. Pytest recently introduced a feature related to supporting fixtures that "yield" - but they're yielding only once right now. Multiple yields from a yield, and direct parametrization through multiple yields should arrive in near future.

I'll be opening an issue in pytest later today about this.

@lesteve: Well another obvious solution, till we dont get a better feature in pytest is to manually make a tempdir and remove it at such places. I am quite fond of tmpdir fixture because we don't need to care about isolation, creation and deletion and leakages at any point. Although if it is suitable, we can directly use the @parametrize decorator by not using tmpdir _for this test only. (I can add a TODO comment there at the best. Or is the current design fine ?)

I think in this case, we should give up on parametrize and keep the for loop. I am afraid that the advantages of parametrize (one test for each input value) does not compensate the disadvantages (strong obfuscation of what exactly gets tested + caveat when we want to add one input value to the test).

When I said parametrize I meant rather a fixture with a params parameter, but you get the point ...

lesteve · 2016-12-21T10:14:05Z

joblib/test/test_memory.py

-                 verbose=0)
-
-    @mem.cache()
+def _setup_temporary_cache_folder(memory, num_inputs=10):


Rename this to _setup_toy_cache or something better

kdexd · 2016-12-21T10:49:16Z

I had one more question - I saw a few smoke tests in test_memorized_repr, earlier they used to print directly to stdout and nosetests had a higher verbosity flag (in setup.cfg), so they were simply visible in CI logs. Can we introduce some string comparisons through capsys there ?

lesteve · 2017-01-05T15:10:21Z

joblib/test/test_memory.py

+    result.clear()  # Do nothing if there is no cache.
+
+
+@fixture(scope='function', params=range(2))


Same comment here about using a for loop.

Okay I'll keep for loops until there's something interesting other than indirect parametrization from pytest's side.

lesteve · 2017-01-05T15:24:36Z

The coverage drop is a bit weird here too ...

lesteve · 2017-01-05T15:28:45Z

Why is it WIP by the way?

kdexd · 2017-01-05T17:00:35Z

@lesteve I was experimenting a little bit of monkeypatching at places. I am usually not in favour of tampaering with sys variables when pytest's monkeypatch fixture is at disposal.

lesteve · 2017-01-09T09:21:01Z

Oops sorry I didn't realize I broke the tests with my last commit. This should be fixed now.

lesteve · 2017-01-10T09:03:59Z

@karandesai-96 I think we should try not to be too perfectionist about remaining pytest-related PRs and merge them reasonably soon. Yes there may be pytest functionality that we could use to write the tests in a marginally nicer way, but that is not crucial enough to spend too much time on it.

In your last commit, you need to keep the sys.modules.pop('tmp_joblib_') in order to reload the module. If you want to be perfectionist you can actually use import imp and tmp = imp.reload(tmp) or something like this.

kdexd · 2017-01-10T09:11:53Z

I think we should try not to be too perfectionist about remaining pytest-related PRs and merge them reasonably soon.

@lesteve This perfectly makes sense.

In your last commit, you need to keep the sys.modules.pop('tmp_joblib_') in order to reload the module.

I'll probably restore this. It looks more straightforward to me.

lesteve · 2017-01-10T09:49:14Z

joblib/test/test_memory.py

+    memory.clear()
+
+
+def func_with_kwonly_args(a, b, kw1='kw1', kw2='kw2'):


I am guessing you added these functions only to get rid of some flake8 warnings. This is a bit misleading so I removed them. Basically the test is for python 3 only and defining a function whose name does not match its behaviour can just add confusion.

Yes I added them because that flake8 job repeatedly gave a red at some point of time. I am puzzled why it doesn't, now. Anyways this is good 👍

It gave a red on flake8 job again.

For completeness, the flake8 is done on the diff that the PR introduces. The main reason is historical: there are some flake violations in some files and we don't want to fix them all because it will introduce conflicts in ongoing PRs. On the other hand we don't want that PRs introduce additional flake8 violations.

Because the flake8 is done on the diff, if you don't change a line that already has a flake8 violation, Travis will be green. In this case we were changing a line that had a flake8 violation so it is in the diff and Travis is red.

Okay, I understand now. Thanks for the clear explanation 👍

lesteve · 2017-01-10T10:09:21Z

OK the Travis failure is the expected flake8 failure. I am going to merge this one, thanks a lot @karandesai-96.

kdexd mentioned this pull request Dec 21, 2016

[WIP] Refactor test_memory.py as per pytest design. #460

Closed

kdexd commented Dec 21, 2016

View reviewed changes

lesteve reviewed Dec 21, 2016

View reviewed changes

kdexd force-pushed the pytestify-test-memory branch from 18a9baf to a31b952 Compare December 21, 2016 12:19

kdexd mentioned this pull request Dec 22, 2016

[MRG] Perform parametrization on certain tests of test_parallel.py #469

Merged

aabadie mentioned this pull request Jan 5, 2017

[MRG] Custom store backends API #397

Merged

4 tasks

lesteve reviewed Jan 5, 2017

View reviewed changes

lesteve force-pushed the pytestify-test-memory branch from 2652056 to 3717577 Compare January 9, 2017 09:20

Karan Desai and others added 7 commits January 10, 2017 10:27

Make a fixture for a memory object.

014a745

Refactor _setup_temporary_cache_folder and tests using it.

1e65d79

Parametrize test_memory_numpy.

abe4222

Parametrize test_partial_decoration.

bda540a

Add dummy declarations of functions to pacify PEP8 warning.

f3197d1

Remove memory and no_memoize_memory fixtures, declare them in tests.

b887839

Replace lingering instance of mem by memory

1740e3a

kdexd force-pushed the pytestify-test-memory branch from 3717577 to 1740e3a Compare January 10, 2017 05:01

Use monkeypatching to tamper with sys path.

ce59dec

kdexd changed the title ~~[WIP] Refactor test_memory.py as per pytest design.~~ [MRG] Refactor test_memory.py as per pytest design. Jan 10, 2017

Karan Desai and others added 2 commits January 10, 2017 14:43

Fix test failures by restoring sys.modules.pop

1b8a832

Remove misleading functions

925ec8d

lesteve reviewed Jan 10, 2017

View reviewed changes

lesteve merged commit bab7c05 into joblib:master Jan 10, 2017

		result.clear() # Do nothing if there is no cache.


		@fixture(scope='function', params=range(2))

		memory.clear()


		def func_with_kwonly_args(a, b, kw1='kw1', kw2='kw2'):

[MRG] Refactor test_memory.py as per pytest design. #466

[MRG] Refactor test_memory.py as per pytest design. #466

Uh oh!

Conversation

kdexd commented Dec 21, 2016

Third Phase (Extended) PR on #411 ( Replacement of #460 , Succeeding #465 )

Uh oh!

kdexd commented Dec 21, 2016

Uh oh!

kdexd left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Reference: http://doc.pytest.org/en/latest/fixture.html#parametrizing-fixtures

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 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesteve commented Jan 5, 2017

Uh oh!

lesteve commented Jan 5, 2017

Uh oh!

kdexd commented Jan 5, 2017

Uh oh!

lesteve commented Jan 9, 2017

Uh oh!

lesteve commented Jan 10, 2017

Uh oh!

kdexd commented Jan 10, 2017

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 Jan 10, 2017

Uh oh!

Uh oh!

kdexd left a comment •

edited

Loading