pytest tmp_dir fixture #706
Conversation
Refactor tests to use the fixture Fix movielens bug
tests/conftest.py
Outdated
| @@ -23,6 +23,15 @@ | |||
| pass # so the environment without spark doesn't break | |||
|
|
|||
|
|
|||
| @pytest.fixture | |||
| def tmp_dir(tmp_path_factory): | |||
There was a problem hiding this comment.
a note of naming, the pytest temporary dir is called tmpdir which is almost the same as ours. Maybe we should rename it to avoid users mistake pytest fixture with our fixture. Maybe temporary_dir?
There was a problem hiding this comment.
good suggestion. maybe even more specific, something like fn_scope_tmpdir?
There was a problem hiding this comment.
or does it sound like some kind of function because of some name convention?
There was a problem hiding this comment.
i'm missing where tmp_path_factory is coming from?
There was a problem hiding this comment.
@gramhagen it is pytest's fixture: https://docs.pytest.org/en/latest/tmpdir.html
There was a problem hiding this comment.
name changed to just tmp, named after linux tmp dir. I picked that for simplicity, but if you think it is not very descriptive, feel free to let me know or give any suggestions. temporary_dir looks (visually) unbalanced to me...haha
There was a problem hiding this comment.
one question, do we foresee any case where we want to have a directory per (pytest) module or even per (pytest) session?
There was a problem hiding this comment.
We can make fixtures for them when we need, like sess_tmp and module_tmp.
tests/smoke/test_movielens.py
Outdated
| @@ -50,53 +49,50 @@ def test_load_pandas_df( | |||
| title_example, | |||
| genres_example, | |||
| year_example, | |||
| tmp_dir, | |||
There was a problem hiding this comment.
there is no need to import this since it is a fixture that it is in conftest, see this: https://github.com/Microsoft/Recommenders/blob/master/tests/conftest.py#L4
There was a problem hiding this comment.
ohhhh good! will update
There was a problem hiding this comment.
ah, nvm. it means you don't need to do import your_fixture_module. You still need to pass the fixture object as an argument. I think the comments contain misleading contents.
There was a problem hiding this comment.
good point, yeah, what you said is more detailed. It would be nice to have it in the code
| @@ -23,6 +23,15 @@ | |||
| pass # so the environment without spark doesn't break | |||
|
|
|||
|
|
|||
| @pytest.fixture | |||
There was a problem hiding this comment.
I also like the context manager option, maybe it can be implemented as we are discussing here: #701 (comment)
There was a problem hiding this comment.
Let me try out that. looks promising.
There was a problem hiding this comment.
checked and turns out normal fixture does the same thing since newer version of pytest.
Since pytest-3.0, fixtures using the normal fixture decorator can use a yield statement to provide fixture values and execute teardown code, exactly like yield_fixture in previous versions. Marking functions as yield_fixture is still supported, but deprecated and should not be used in new code.
There was a problem hiding this comment.
good, so if we use the fixture as you program it using: with temp_dir: is the folder erased after the test function is finished?
There was a problem hiding this comment.
yes. Even try/finally does remove the dir too, but using the context manager made the code simpler and give us peace of mind :-)
Change to use context manager Change fixture name from tmp_dir to tmp
| WARNING_HAVE_SCHEMA_AND_HEADER = """Both schema and header are provided. | ||
| The header argument will be ignored.""" | ||
| ERROR_MOVIE_LENS_SIZE = "Invalid data size. Should be one of {100k, 1m, 10m, or 20m}" | ||
| ERROR_NO_HEADER = "No header (schema) information" | ||
| ERROR_NO_HEADER = "No header (schema) information. At least user and movie column names should be provided" |
There was a problem hiding this comment.
Really like these constant string of error/warning message. Do we later want to consolidate this into somewhere in common/constants? Or, at least, adopt this practice in other utility codes.
There was a problem hiding this comment.
If we need, sure why not. We can even generalize further, e.g. use obj.__name__ or to accept names as args.
| if movie_col is not None: | ||
| item_header.append(movie_col) | ||
| usecols.append(0) | ||
| if title_col is None and genres_col is None and year_col is None: |
There was a problem hiding this comment.
Looks like the check now is different from before? The previous checks the movie_col and return None if it does not exist, whilst now it checks other three columns.
There was a problem hiding this comment.
yeah, made movie_col as a default (must-have) column, checking earlier as: if header is None or len(header) < 2: (2 is the first two columns, user and movie), because there is no point to load only user ids.
| @@ -23,6 +23,12 @@ | |||
| pass # so the environment without spark doesn't break | |||
|
|
|||
|
|
|||
| @pytest.fixture | |||
| def tmp(tmp_path_factory): | |||
There was a problem hiding this comment.
This looks nice and clean ❤️
There was a problem hiding this comment.
kudos to @miguelgfierro @anargyri!
* pytest tmp_dir fixture Refactor tests to use the fixture Fix movielens bug * Update conftest description
Description
Related Issues
#701
Checklist: