[MRG] Replace manual setup / teardown of temp directory with 'tmpdir' fixture.#463
[MRG] Replace manual setup / teardown of temp directory with 'tmpdir' fixture.#463lesteve merged 10 commits intojoblib:masterfrom
Conversation
c316ac9 to
be05384
Compare
03eecda to
903e7bd
Compare
|
@lesteve please have a round of reviews as per convenience. Github says it has added "Request to Review" feature on PRs but I can't find anything here on this interface. 😕 |
| def test_numpy_persistence_bufferred_array_compression(): | ||
| def test_numpy_persistence_bufferred_array_compression(tmpdir): | ||
| big_array = np.ones((_IO_BUFFER_SIZE + 100), dtype=np.uint8) | ||
| filename = env['filename'] + str(random.randint(0, 1000)) |
There was a problem hiding this comment.
I always thought that this random.randint for naming the file was not so nice. I would hope we can remove it while we are doing the tmpdir work.
Slightly related is tmpdir a function or module fixture?
There was a problem hiding this comment.
Just checked it is a function fixture. So I think most of the str(random.randint(0, 1000)) can go (AFAICT there were there so that each test functions would not on each others toe because the temporary folder was defined for the whole file).
Inside a single test function, you may want to to use different filenames:
filename1 = tmpdir.join('test1.pkl').strpath
...
filename2 = tmpdir.join('test2.pkl').strpath
...although when the variable filename is reused you can probably only define it once and reuse it.
There was a problem hiding this comment.
@lesteve there is a session scoped tmpdir_factory fixture available as well. I can make a subdirectory, and limit it to a module scoped if needed.
There was a problem hiding this comment.
So I'll remove this random ints wherever they look similar..
There was a problem hiding this comment.
I think it is better to use tmpdir. Because tmpdir is a function fixture you know that each test function is independent from each other.
There was a problem hiding this comment.
And my guess is that most random.randint can be removed.
There was a problem hiding this comment.
And my guess is that most random.randint can be removed.
@lesteve: Yeah, everything worked perfectly fine !
|
|
||
| ############################################################################### | ||
| # Helper function for the tests | ||
| def check_identity_lazy(func, accumulator): |
There was a problem hiding this comment.
Nitpick: I would replace tmpdir by cachedir, this feels more clear. You will need to call check_identity_lazy(func, accumulator, tmpdir.strpath) in a few places.
| a = rnd.random_sample((10, 2)) | ||
| for compress in (False, True, 0, 3): | ||
| # We use 'a.T' to have a non C-contiguous array. | ||
| for index, obj in enumerate(((a,), (a.T,), (a, a), [a, a, a])): |
There was a problem hiding this comment.
Remove coment it does not apply any more
| filename = env['filename'] | ||
| filename = tmpdir.join('test.pkl').strpath | ||
| for compress in [0, 1]: | ||
| for member in typelist: |
There was a problem hiding this comment.
Remove comment. Please check all the "Change the file name ..." there may be others lying around
There was a problem hiding this comment.
Commit coming within 5 minutes, stay tuned 😁
| assert isinstance(b, np.memmap) | ||
|
|
||
| # Test with an object containing multiple numpy arrays | ||
| filename = env['filename'] + str(random.randint(0, 1000)) |
There was a problem hiding this comment.
Is there a reason why you used 'test2.pkl' here rather than just reuse the same filename?
There was a problem hiding this comment.
This was a linear program so I wasn't sure about the side effects. Upon inspection I guess it is safe to use same name here.
There was a problem hiding this comment.
@lesteve This causes a failure on Windows platform..
There was a problem hiding this comment.
Restored these lines back and it's solved. Sadly I can't reproduce errors related to Windows locally.
There was a problem hiding this comment.
OK fair enough, the Windows error is not very clear, let's leave it like this.
| u"C'est l'\xe9t\xe9 !"] | ||
|
|
||
| with tempfile.NamedTemporaryFile(suffix='.gz', dir=env['dir']) as f: | ||
| fname = f.name |
|
LGTM, merging, thanks! |
Fourth Phase PR on #411 (Succeeding PR #461 )
Several modules declare a setup and teardown where a temporary directory was created and its path was set as a global variable. Creation and deletion of this directory was handled manually while setup and teardown. This has been replaced by pytest's
tmpdirfixture.It is a
py.path.localobject, and has to be specified as a parameter in tests requiring a temporary directory.