adds different scopes for datadirs

[salotz]

In order to accomplish this I generalized the creation of datadirs,
and then provided aliases for the old fixtures of the same name.

This adds a number of fixtures to the namespace which are the same as
before but prefixed by the pytest scope they apply to,
e.g. function_shared_datadir. Which is the same as the old
shared_datadir.

This also opens up for more complex (i.e. non-hardcoded) usage
scenarios, although I have just stuck to the same behaviors as before.

The only difference perhaps is that previously there was a behavior
where if the original data dir was not a directory at all a directory
in the temp dir was created with that name, although I didn't quite
understand why that was needed or what pattern it supports.

[salotz]

Fixes #26 (even though it was closed). Let me know what you think. Feel free to make suggestions and edits.

[salotz]

I'll work on fixing the tests, just wanted to get this on here to see if there was any big objections to a change like this.

[igortg]

Seems a very nice addition. If it's full backward compatible, it's a go for me.

[salotz]

Realized there was an issue with the previous implementation. Shouldn't have been so quick to do metaprogramming. New one is much cleaner IMO, although not parametric in the scope nor does it work for sessions (which is a limitation of tmpdir and shouldn't have worked last time either).

[nicoddemus]

Hi @salotz,

Thanks a lot for your PR, we appreciate it!

Besides my comments and fixing existing tests, please also add new tests for the new fixtures.

[salotz]

Does this behavior really make sense https://github.com/gabrielcnr/pytest-datadir/blob/master/tests/test_nonexistent.py ? I'm all for backwards compatibility, but this sounds like a silent failure. If you ask for a datadir but don't have one you probably want to remove that fixture from your test, right? If you just want a tmpdir, then just ask for that fixture.

[nicoddemus]

Does this behavior really make sense /tests/test_nonexistent.py@master ?

I understand the reasoning, but having the datadir work like tmpdir if there is no data directory is very convenient; it saves you from having to refactor in case things change. Please keep the existing behavior. 👍

[nicoddemus]

Hi @salotz,

Thanks a lot for the contribution!

Please take a look at my comments, but in summary I think we don't need to have a separate fixture for each possible scope. IMHO we should follow how pytest implements tmp_path and tmp_path_factory, so this plugin should provide:

• A session-scoped datadir_factory
• A function-scoped datadir
• A function-scoped shared_datadir

And that's it. Having multiple fixtures for each scope just adds boilerplate code to the plugin without adding much to the end user. For example, consider a module-scoped fixture:

@pytest.fixture(scope='module')
def myfiles(datadir_factory):
    storage = datadir_factory.mkdatadir() / 'store'
    ...

This is both convenient and easy to understand, without having to remember several *datadir fixtures.

Besides this, we would also need tests and docs.

[nicoddemus]

I'm not sure having specific "shared_datadir" fixtures for each scope makes sense. Users could just use datadir_factory directly, similar to how pytest does it.

[salotz]

I disagree here. Requesting new datadirs (mkdatadir()) from the factory involves copying which we are trying to avoid.

[nicoddemus]

Ditto here about having multiple scopes.

[salotz]

ditto disagreement above

[salotz]

Hi @nicoddemus,

I really appreciate your patience and taking the time to review these and give me feedback and actually understanding my intention. I will cogitate on this a bit to make sure I understand it, as you pointed out some errors in correctness of the code's intended behavior.

My motivation for the for having datadir fixtures at different scopes was as follows:

1. The "function" scoped tmpdir was really an implicit scope. I like and tend to follow in my own codebases the "explicit is better than implicit" mantra. Despite the added verbosity (which I feel is more of a design problem with implicit in-band signalling with pytest fixture names) I think it is much more explicit about what you are doing. I had to go in and read the code to find out what scope tmpdir, datadir etc. were which is obfuscation IMO.

2. tmpdir and datadir fixtures at scopes other than session and function are useful. I like how you can rely on the scoping to do the teardown stuff for you so that you don't have to worry about making and naming separate directories for different tests, just because you have to use the session-scoped tmp_path_factory. This was one of the features that led me to adopting pytest in the first place. If they aren't in pytest-datadir I would probably end up adding them to my conftest.py or my own fork, but then this package would just be passing off the buck of boilerplate to users (unless I am the only one), which kind of defeats the purpose of a module.

I don't want to start feature creep but the scope of functionality for this project was very small to begin with, and I hardly think that this increases the complexity of the codebase or API by much.

In particular I would like to know anyone else's take on the issues in point 2. How do you handle, say, a datadir/tmpdir for a class scope?

Hopefully, I'm not making a mountain of a mole hill here, but these are motivated by my own real usage of the package and the boilerplate I wrote to address the packages shortcomings.

[nicoddemus]

I really appreciate your patience and taking the time to review these and give me feedback and actually understanding my intention.

No problem at all, always glad to receive and discuss new contributions. 👍

I had to go in and read the code to find out what scope tmpdir, datadir etc. were which is obfuscation IMO.

Fair point, but this probably should be improved on pytest itself to mention the fixture scope with pytest --fixtures.

tmpdir and datadir fixtures at scopes other than session and function are useful. I like how you can rely on the scoping to do the teardown stuff for you so that you don't have to worry about making and naming separate directories for different tests

But please keep in mind that just by making a fixture module or class-scoped, that the directories will be removed automatically at the end of that scope. The only thing the fixture scope dictates is that the "teardown" code of the fixture will be executed when the fixture gets out of scope, but we don't have any specific teardown code in place that removes the directories created by the fixture: we rely on the tmp_path_factory behavior that removes directories from previous runs.

In particular I would like to know anyone else's take on the issues in point 2. How do you handle, say, a datadir/tmpdir for a class scope?

I've never needed this with datadir, but the few times this has come up with tmp_path I just used tmp_path_factory to create a new directory. Note that I don't care that the directory effectively gets removed when the fixture gets out of scope, I delegate to the tmp_path_factory to remove the created directory eventually.

[salotz]

The main idea is to reduce the amount of copying that is needed for large inputs. If you are using the same data over and over again for many tests calling mkdatadir() over and over will make your tests much slower than they need to be. The module_datadir etc. just copies once for the scope. For example there is only one copy here for the module:

def test_something(module_datadir):
    with open(os.path.join(module_datadir, 'the_big_data.txt'), 'r') as f:
        data = f.read()
        assert something(data)

def test_somethingelse(module_datadir):
    with open(os.path.join(module_datadir, 'the_big_data.txt'), 'r') as f:
        data = f.read()
        assert somethingelse(data)

If you for sure want to make a new (very clean) directory then you would use the approach you outlined. The idea though is that the scoping structure of your tests (module, class, and function) implicitly gives you default datadirs that minimize copying.

I actually hadn't included the singleton datadir_factory fixture in the API before, I will add it in. However, it will have to be a module scoped fixture because we rely on using the module name for the non-shared datadirs.

I was a little confused before but I have wittled it down to the necessary parts with latest commit. (I'll clean up with a rebase). We really are only introducing 2 to 3.5 new things plus some copy-pasting for the different scopes:

• DatadirFactory which we could even make hidden since its not that useful to users (i.e. _DatadirFactory)
• datadir_factory singleton
• <scope>_<shared>_datadir fixtures