From 77e7f71ee3124714f6db27af0fa1f0bd43100ca5 Mon Sep 17 00:00:00 2001 From: Athanasius Date: Wed, 7 Sep 2022 12:03:50 +0100 Subject: [PATCH 1/3] tests: journal_lock: Add, and use, `_release_lock()` to fix TestJournalLock.test_release_lock Because the `with ...` was leaving the file locked it then couldn't be `os.unlink()`'d. --- tests/journal_lock.py/test_journal_lock.py | 35 ++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/journal_lock.py/test_journal_lock.py b/tests/journal_lock.py/test_journal_lock.py index eb100269..6d93d54a 100644 --- a/tests/journal_lock.py/test_journal_lock.py +++ b/tests/journal_lock.py/test_journal_lock.py @@ -104,6 +104,40 @@ def _obtain_lock(prefix: str, filehandle) -> bool: return False return True + + +def _release_lock(prefix: str, filehandle) -> bool: + """ + Release the JournalLock. + + :param prefix: str - what to prefix output with. + :param filehandle: File handle already open on the lockfile. + :return: bool - True if we released the lock. + """ + if sys.platform == 'win32': + print(f'{prefix}: On win32') + import msvcrt + try: + print(f'{prefix}: Trying msvcrt.locking() ...') + filehandle.seek(0) + msvcrt.locking(filehandle.fileno(), msvcrt.LK_UNLCK, 4096) + + except Exception as e: + print(f'{prefix}: Unable to unlock file: {e!r}') + return False + + else: + import fcntl + + print(f'{prefix}: Not win32, using fcntl') + try: + fcntl.flock(filehandle, fcntl.LOCK_UN) + + except Exception as e: + print(f'{prefix}: Unable to unlock file: {e!r}') + return False + + return True ########################################################################### @@ -316,6 +350,7 @@ class TestJournalLock: # And finally check it actually IS unlocked. with open(mock_journaldir.getbasetemp() / 'edmc-journal-lock.txt', mode='w+') as lf: assert _obtain_lock('release-lock', lf) + assert _release_lock('release-lock', lf) # Cleanup, to avoid side-effect on other tests os.unlink(str(jlock.journal_dir_lockfile_name)) From 480b0712ddca4eb3e15871759269a5acb27510e0 Mon Sep 17 00:00:00 2001 From: Athanasius Date: Wed, 7 Sep 2022 12:21:57 +0100 Subject: [PATCH 2/3] tests: journal_lock: Close filehandle in `test_obtain_lock_already_locked()` After the test completes as expected (ALREADY_LOCKED), ensure we close the filehandle for that, else it seems to interfere with the sub-process cleanup, which then leads to subsequent tests not being able to cleanup. Tests as now working after 10s of runs of `pytest` in a loop. --- tests/journal_lock.py/test_journal_lock.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/journal_lock.py/test_journal_lock.py b/tests/journal_lock.py/test_journal_lock.py index 6d93d54a..7afa588c 100644 --- a/tests/journal_lock.py/test_journal_lock.py +++ b/tests/journal_lock.py/test_journal_lock.py @@ -70,6 +70,7 @@ def other_process_lock(continue_q: mp.Queue, exit_q: mp.Queue, lockfile: pathlib exit_q.get(block=True, timeout=None) # And clean up + _release_lock('sub-process', lf) os.unlink(lockfile / 'edmc-journal-lock.txt') @@ -329,6 +330,8 @@ class TestJournalLock: # Fails on Linux, because flock(2) is per process, so we'd need to # use multiprocessing to test this. assert second_attempt == JournalLockResult.ALREADY_LOCKED + # And need to release any handles on the lockfile + jlock.journal_dir_lockfile.close() print('Telling sub-process to quit...') exit_q.put('quit') print('Waiting for sub-process...') From 5a0e31436b2f3f84239d4b3993bc631660319fb4 Mon Sep 17 00:00:00 2001 From: Athanasius Date: Wed, 7 Sep 2022 12:27:35 +0100 Subject: [PATCH 3/3] tests: journal_lock: Works 100% on Linux, remove 'plan' docstring text --- tests/journal_lock.py/test_journal_lock.py | 39 ++++------------------ 1 file changed, 6 insertions(+), 33 deletions(-) diff --git a/tests/journal_lock.py/test_journal_lock.py b/tests/journal_lock.py/test_journal_lock.py index 7afa588c..016a8f1b 100644 --- a/tests/journal_lock.py/test_journal_lock.py +++ b/tests/journal_lock.py/test_journal_lock.py @@ -1,33 +1,4 @@ -""" -Tests for journal_lock.py code. - -Tests: - - Is file actually locked after obtain_lock(). Problem: We opened the - file in a manner which means nothing else can open it. Also I assume - that the same process will either be allowed to lock it 'again' or - overwrite the lock. - - Expected failures if: - - 1. Lock already held (elsewhere). - 2. Can't open lock file 'w+'. - 3. Path to lock file doesn't exist. - 4. journaldir is None (default on Linux). - - - Does release_lock() work? Easier to test, if it's worked.... - 1. return True if not locked. - 2. return True if locked, but successful unlock. - 3. return False otherwise. - - - JournalLock.set_path_from_journaldir - 1. When journaldir is None. - 2. Succeeds otherwise? - - - Can any string to pathlib.Path result in an invalid path for other - operations? - - - Not sure about testing JournalAlreadyLocked class. -""" +"""Tests for journal_lock.py code.""" import multiprocessing as mp import os import pathlib @@ -327,11 +298,13 @@ class TestJournalLock: # Now attempt to lock with to-test code jlock = JournalLock() second_attempt = jlock.obtain_lock() - # Fails on Linux, because flock(2) is per process, so we'd need to - # use multiprocessing to test this. assert second_attempt == JournalLockResult.ALREADY_LOCKED - # And need to release any handles on the lockfile + + # Need to release any handles on the lockfile else the sub-process + # might not be able to clean up properly, and that will impact + # on later tests. jlock.journal_dir_lockfile.close() + print('Telling sub-process to quit...') exit_q.put('quit') print('Waiting for sub-process...')