Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing suite should be machine and user agnostic #692

Closed
lewisblake opened this issue Jul 5, 2022 · 3 comments · May be fixed by #691
Closed

Testing suite should be machine and user agnostic #692

lewisblake opened this issue Jul 5, 2022 · 3 comments · May be fixed by #691
Labels
bug Something isn't working testing

Comments

@lewisblake
Copy link
Member

lewisblake commented Jul 5, 2022

When trying to run the test suit on PPI, I ran into an issue with permissions. e.g.,

PermissionError: [Errno 13] Permission denied: '/lustre/storeA/project/aerocom/aerocom2/pyaerocom_out/testdata-minimal/scripts/create_subset_ebas.py'

After chatting with @jgriesfeller and @avaldebe and a ticket to IT to change ownership of the directories needed for testing to @jgriesfeller, we concluded that testing is not multi-user capable.

/modules/centos7/user-apps/aerocom/anaconda3/envs/pyadev-applied/lib/python3.9/pathlib.py:1120: in _opener
    return self._accessor.open(self, flags, mode)
E   PermissionError: [Errno 13] Permission denied: '/tmp/test_iris_io/invalid.nc'
(pyadev-applied) jang@ppi-clogin-a1:~/.../Python3/pyaerocom$ ll /tmp/test_iris_io/invalid.nc
-rw-rw-r-- 1 danielh danielh 0 Jul  4 12:44 /tmp/test_iris_io/invalid.nc

In order for tests to be run agnostically, they should not depend on /tmp, but rather be specific to the machine and $USER (@jgriesfeller please let me know if I am forgetting anything). The Python module tempfile may be starting point for thinking of a solution.

@lewisblake lewisblake added testing bug Something isn't working labels Jul 5, 2022
@avaldebe
Copy link
Collaborator

avaldebe commented Jul 5, 2022

/modules/centos7/user-apps/aerocom/anaconda3/envs/pyadev-applied/lib/python3.9/pathlib.py:1120: in _opener
    return self._accessor.open(self, flags, mode)
E   PermissionError: [Errno 13] Permission denied: '/tmp/test_iris_io/invalid.nc'
(pyadev-applied) jang@ppi-clogin-a1:~/.../Python3/pyaerocom$ ll /tmp/test_iris_io/invalid.nc
-rw-rw-r-- 1 danielh danielh 0 Jul  4 12:44 /tmp/test_iris_io/invalid.nc

The lines that create this file are

FAKE_FILE = Path(tempfile.gettempdir()) / "test_iris_io/invalid.nc"
FAKE_FILE.parent.mkdir(exist_ok=True, parents=True)
FAKE_FILE.write_text("")

The quick'n dirty solution would be to create a temporary directory each time with tempfile.mkdtemp() as follows

- FAKE_FILE = Path(tempfile.gettempdir()) / "test_iris_io/invalid.nc"
+ FAKE_FILE = Path(tempfile.mkdtemp()) / "test_iris_io/invalid.nc"

IMO, this file should be created inside a fixture and modify the tests consuming this file to use the fixture.

@avaldebe
Copy link
Collaborator

avaldebe commented Jul 5, 2022

I can find only one other usage of tempfile on the test suite

TMP_PATH = Path(tempfile.mkdtemp())
CFG_FILE_WRONG = TMP_PATH / "paths.txt"
CFG_FILE_WRONG.write_text("")
LOCAL_DB_DIR = TMP_PATH / "data"
LOCAL_DB_DIR.mkdir()
(LOCAL_DB_DIR / "modeldata").mkdir()
(LOCAL_DB_DIR / "obsdata").mkdir()

Fortunately, tempfile.mkdtemp() is used here. It creates a new temporary directory each time, so there is no interference between users.

However, like on my last comment I would prefer these temporary files to be provided by fixtures.

avaldebe added a commit to avaldebe/pyaerocom that referenced this issue Jul 5, 2022
@avaldebe avaldebe mentioned this issue Jul 5, 2022
@avaldebe
Copy link
Collaborator

avaldebe commented Jul 5, 2022

PR #694 dealt with the use of temporary files, and PR #691 updates the testdata-minimal scripts.
Should this issue be closed then #691 gets merged or is there more than needs to be patched up?

@lewisblake lewisblake linked a pull request Jul 8, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants