Convert tests to pytest #25
Conversation
tests/conftest.py
Outdated
| output_file.unlink() | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) |
There was a problem hiding this comment.
nit: I think scope="session" is more efficient than autouse=True for this kind of fixture, as it will just run (and fail) once, rather than every test?
There was a problem hiding this comment.
👍 although it needs scope=session as well as autouse=True, not instead of, otherwise it has to be explicitly included for every test
tests/conftest.py
Outdated
| def run_docker(): | ||
| def _run(command): | ||
| filestem = Path(command.split()[0]).stem | ||
| user = subprocess.check_output( |
There was a problem hiding this comment.
python can do this :)
import os, pwd
uid = os.getuid()
gid = pwd.getpwuid(uid).pw_gid
tests/conftest.py
Outdated
| process = subprocess.run( | ||
| f"docker run --rm --user {user} -e STATA_LICENSE -v {TESTS_PATH.resolve()}:/workspace " | ||
| f"{IMAGE} {command}", | ||
| shell=True, |
There was a problem hiding this comment.
nit: I prefer cmd lists and not setting shell=True (which can often trip linting errors), but this is a test so its probably fine, and just my OCD :)
There was a problem hiding this comment.
I tried to do this and couldn't get it to work, but I've just realised why (the command in some tests is a string with spaces and needed split)
tests/test_arrowload.py
Outdated
|
|
||
| @pytest.fixture() | ||
| def setup_arrow_files(): | ||
| for filep in (TESTS_PATH / "fixtures").glob("*.arrow"): |
There was a problem hiding this comment.
nit: reusing the same fixed test dir, and not deleting them, seems like it might cause issues down the line? But we're not writing any files in the tests, so I guess it doesn't matter
Why to they have to be in the output subdir? Could we just load them from the fixtures subdir?
There was a problem hiding this comment.
I feel like I had a reason once, but now I've no idea what it was. Possibly I thought when I wrote the initial bash test that we didn't have access to the fixtures folder in the container, but we do. I may have just been following the gz test .do file, which loads from the output folder, but that's because it depends on a previous test that generates a file in there.
tests/conftest.py
Outdated
|
|
||
|
|
||
| @pytest.fixture | ||
| def run_docker(): |
There was a problem hiding this comment.
nit: s/run_docker/run_stata? Seems to match what it does a bit more?
There was a problem hiding this comment.
nit: I don't think run_docker needs to be a pytest fixture, it could just be a test helper?
Now that we have a python venv available, convert the bash tests to pytest