Use pytest tmpdir fixture

[neggert]

As mentioned in #463, replace manually creating and deleting save directories with the pytest tmpdir feature. This eliminates the need to manually clear out directories.

Right now, I've just done this for one of the test files. I want to get some feedback on the change before I spend the time to do the rest of them. @williamFalcon @Borda LMK if you agree that this is a good idea and I'll go ahead and do the rest.

[neggert]

Ping @williamFalcon @Borda any thoughts before I go ahead and update the rest of the tests?

[Borda]

I would use Unittests - https://docs.python.org/3/library/unittest.html where you can define some action on start and end, in this case, create temp folder and remove at the end, for example:

import os
import unittest
import uuid
import shutils

class TestCPUModels(unittest.TestCase):
    def setUp(self):
        self.tmp_dir = os.path.join(os.path.dirname(__file__), uuid.uuid4().hex)
        os.mkdir(self.tmp_dir)

    def test_early_stopping_cpu_model():
        ...
    def test_running_test_after_fitting():
        ...

    def tearDown(self):
        shutils.rmtree(self.tmp_dir)

[williamFalcon]

@neggert sorry for the delay. Fairly busy 2 weeks.

I don't have strong feelings about either. unless one clearly outweighs the other.

@Borda i'm not sure your approach solves the issue? you need a new tmpdir for each test, not just a class of tests

[Borda]

@williamFalcon see the doc of unittests, it says "The setUp() and tearDown() methods allow you to define instructions that will be executed before and after each test method. They are covered in more detail in the section Organizing test code."
in contrast, there is "setUpClass() - A class method called before tests in an individual class are run. setUpClass is called with the class as the only argument and must be decorated as a classmethod()" and "tearDownClass() - A class method called after tests in an individual class have run. tearDownClass is called with the class as the only argument and must be decorated as a classmethod()"
so it will generate a unique temp folder for each test method... :)

[neggert]

@Borda pytest has similar functionality. No need to swap out our entire testing framework. I prefer pytest over unittest anyway.

Here I'm using a fixture that is built-in to pytest that does what you're describing. Pytest generates and deletes a temp folder for you.

[Borda]

@neggert it is not about swapping framework, unittest is just another functionality as well as doctests... I would call pytest as an ecosystem... where unit-tests belongs :)
btw, you probably missed the link...

[neggert]

I guess I'm not clear on why you're suggesting using unittest when we already have this functionality from our existing test framework. Maybe I'm missing your point.

I think the thing you're suggesting we use in unittest is exactly what this PR does using our existing pytest framework.

[williamFalcon]

@neggert @Borda let's come back to this after the documentation and coverage is back up to speed.

While it's clear we have two really good options, let's go with the solution proposed by @neggert in the meantime while we look deeper into @Borda 's option.

I'd like to spend more time on game-changing features instead! :)

[neggert]

Sounds good. I'll go ahead and update the rest of the tests.

[neggert]

@williamFalcon I think this is ready to go. I don't have a GPU that can run the AMP tests, so I'd appreciate a double-check on those.

There's also something strange going on with the Comet logger test, but I'm not sure if it's related to this PR.

[neggert]

ping @williamFalcon. I think this is ready to merge.

[williamFalcon]

@neggert sorry, just getting back online. mind rebasing?

[neggert]

Fixed the conflicts. Also figured out what's going on with the CometML logger. I'd like to leave that for a separate PR, if you don't mind.