From c21b6cb071c49afbfa1fc90c8df9b29d95337711 Mon Sep 17 00:00:00 2001 From: Houkime <> Date: Wed, 30 Nov 2022 14:17:53 +0000 Subject: [PATCH 01/10] jobs: dedicated reset test --- tests/test_jobs.py | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/tests/test_jobs.py b/tests/test_jobs.py index 371dca4..220cebc 100644 --- a/tests/test_jobs.py +++ b/tests/test_jobs.py @@ -6,11 +6,22 @@ from selfprivacy_api.jobs import Jobs, JobStatus import selfprivacy_api.jobs as jobsmodule -def test_jobs(authorized_client, jobs_file, shared_datadir): - jobs = Jobs() +def test_add_reset(jobs): + test_job = jobs.add( + type_id="test", + name="Test job", + description="This is a test job.", + status=JobStatus.CREATED, + status_text="Status text", + progress=0, + ) + assert jobs.get_jobs() == [test_job] jobs.reset() assert jobs.get_jobs() == [] + +def test_jobs(jobs): + test_job = jobs.add( type_id="test", name="Test job", @@ -19,7 +30,6 @@ def test_jobs(authorized_client, jobs_file, shared_datadir): status_text="Status text", progress=0, ) - assert jobs.get_jobs() == [test_job] jobs.update( @@ -46,18 +56,9 @@ def test_jobs(authorized_client, jobs_file, shared_datadir): @pytest.fixture -def mock_subprocess_run(mocker): - mock = mocker.patch("subprocess.run", autospec=True) - return mock - - -@pytest.fixture -def mock_shutil_move(mocker): - mock = mocker.patch("shutil.move", autospec=True) - return mock - - -@pytest.fixture -def mock_shutil_chown(mocker): - mock = mocker.patch("shutil.chown", autospec=True) - return mock +def jobs(): + j = Jobs() + j.reset() + assert j.get_jobs() == [] + yield j + j.reset() From b6eeec23ccc185f3542488f4858892095b5a4487 Mon Sep 17 00:00:00 2001 From: Houkime <> Date: Wed, 30 Nov 2022 14:31:37 +0000 Subject: [PATCH 02/10] jobs: singlejob fixture --- tests/test_jobs.py | 43 ++++++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/tests/test_jobs.py b/tests/test_jobs.py index 220cebc..63b06d1 100644 --- a/tests/test_jobs.py +++ b/tests/test_jobs.py @@ -6,31 +6,14 @@ from selfprivacy_api.jobs import Jobs, JobStatus import selfprivacy_api.jobs as jobsmodule -def test_add_reset(jobs): - test_job = jobs.add( - type_id="test", - name="Test job", - description="This is a test job.", - status=JobStatus.CREATED, - status_text="Status text", - progress=0, - ) - assert jobs.get_jobs() == [test_job] - jobs.reset() - assert jobs.get_jobs() == [] +def test_add_reset(jobs_with_one_job): + jobs_with_one_job.reset() + assert jobs_with_one_job.get_jobs() == [] -def test_jobs(jobs): - - test_job = jobs.add( - type_id="test", - name="Test job", - description="This is a test job.", - status=JobStatus.CREATED, - status_text="Status text", - progress=0, - ) - assert jobs.get_jobs() == [test_job] +def test_jobs(jobs_with_one_job): + jobs = jobs_with_one_job + test_job = jobs_with_one_job.get_jobs()[0] jobs.update( job=test_job, @@ -62,3 +45,17 @@ def jobs(): assert j.get_jobs() == [] yield j j.reset() + + +@pytest.fixture +def jobs_with_one_job(jobs): + test_job = jobs.add( + type_id="test", + name="Test job", + description="This is a test job.", + status=JobStatus.CREATED, + status_text="Status text", + progress=0, + ) + assert jobs.get_jobs() == [test_job] + return jobs From 106a083ca2f6d27484f83da5853d106b462d8071 Mon Sep 17 00:00:00 2001 From: Houkime <> Date: Wed, 30 Nov 2022 14:47:56 +0000 Subject: [PATCH 03/10] jobs: simplify reset Also ups test coverage --- selfprivacy_api/jobs/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/selfprivacy_api/jobs/__init__.py b/selfprivacy_api/jobs/__init__.py index 9e4d961..a3007a4 100644 --- a/selfprivacy_api/jobs/__init__.py +++ b/selfprivacy_api/jobs/__init__.py @@ -67,10 +67,9 @@ class Jobs: """ Reset the jobs list. """ - r = RedisPool().get_connection() jobs = Jobs.get_jobs() for job in jobs: - r.delete(redis_key_from_uuid(job.uid)) + Jobs.remove(job) @staticmethod def add( From 7acbba996057220e94bad77254af88d1e90a9e82 Mon Sep 17 00:00:00 2001 From: Houkime <> Date: Wed, 30 Nov 2022 15:12:42 +0000 Subject: [PATCH 04/10] jobs: minimal update test --- tests/test_jobs.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/test_jobs.py b/tests/test_jobs.py index 63b06d1..c7eb3d6 100644 --- a/tests/test_jobs.py +++ b/tests/test_jobs.py @@ -11,6 +11,15 @@ def test_add_reset(jobs_with_one_job): assert jobs_with_one_job.get_jobs() == [] +def test_minimal_update(jobs_with_one_job): + jobs = jobs_with_one_job + test_job = jobs_with_one_job.get_jobs()[0] + + jobs.update(job=test_job, status=JobStatus.ERROR) + + assert jobs.get_jobs() == [test_job] + + def test_jobs(jobs_with_one_job): jobs = jobs_with_one_job test_job = jobs_with_one_job.get_jobs()[0] From f51e378ff0061001517425604e6f2c8be3144ae9 Mon Sep 17 00:00:00 2001 From: Houkime <> Date: Wed, 30 Nov 2022 15:21:57 +0000 Subject: [PATCH 05/10] jobs: test updating more fields --- tests/test_jobs.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_jobs.py b/tests/test_jobs.py index c7eb3d6..df5c952 100644 --- a/tests/test_jobs.py +++ b/tests/test_jobs.py @@ -26,6 +26,8 @@ def test_jobs(jobs_with_one_job): jobs.update( job=test_job, + name="Write Tests", + description="An oddly satisfying experience", status=JobStatus.RUNNING, status_text="Status text", progress=50, From 870d2c408de938e1b35fa215ccf683e09716126d Mon Sep 17 00:00:00 2001 From: Houkime <> Date: Wed, 30 Nov 2022 15:37:59 +0000 Subject: [PATCH 06/10] jobs: test nofail at nonexistent update --- tests/test_jobs.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/test_jobs.py b/tests/test_jobs.py index df5c952..7ef9a9b 100644 --- a/tests/test_jobs.py +++ b/tests/test_jobs.py @@ -20,6 +20,16 @@ def test_minimal_update(jobs_with_one_job): assert jobs.get_jobs() == [test_job] +def test_remove_update_nonexistent(jobs_with_one_job): + test_job = jobs_with_one_job.get_jobs()[0] + + jobs_with_one_job.remove(test_job) + assert jobs_with_one_job.get_jobs() == [] + + result = jobs_with_one_job.update(job=test_job, status=JobStatus.ERROR) + assert result == test_job # even though we might consider changing this behavior + + def test_jobs(jobs_with_one_job): jobs = jobs_with_one_job test_job = jobs_with_one_job.get_jobs()[0] From 14c4ae26abc3916fc3f792f012a83ee1e327a2cd Mon Sep 17 00:00:00 2001 From: Houkime <> Date: Wed, 30 Nov 2022 16:22:46 +0000 Subject: [PATCH 07/10] explicitly mark helper functions private I thought about making them private class members, but that would get unreadable and do more harm than good. --- selfprivacy_api/jobs/__init__.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/selfprivacy_api/jobs/__init__.py b/selfprivacy_api/jobs/__init__.py index a3007a4..6ad8493 100644 --- a/selfprivacy_api/jobs/__init__.py +++ b/selfprivacy_api/jobs/__init__.py @@ -98,7 +98,7 @@ class Jobs: result=None, ) r = RedisPool().get_connection() - store_job_as_hash(r, redis_key_from_uuid(job.uid), job) + _store_job_as_hash(r, _redis_key_from_uuid(job.uid), job) return job @staticmethod @@ -114,7 +114,7 @@ class Jobs: Remove a job from the jobs list. """ r = RedisPool().get_connection() - key = redis_key_from_uuid(job_uuid) + key = _redis_key_from_uuid(job_uuid) r.delete(key) return False @@ -148,9 +148,9 @@ class Jobs: job.finished_at = datetime.datetime.now() r = RedisPool().get_connection() - key = redis_key_from_uuid(job.uid) + key = _redis_key_from_uuid(job.uid) if r.exists(key): - store_job_as_hash(r, key, job) + _store_job_as_hash(r, key, job) if status in (JobStatus.FINISHED, JobStatus.ERROR): r.expire(key, JOB_EXPIRATION_SECONDS) @@ -162,9 +162,9 @@ class Jobs: Get a job from the jobs list. """ r = RedisPool().get_connection() - key = redis_key_from_uuid(uid) + key = _redis_key_from_uuid(uid) if r.exists(key): - return job_from_hash(r, key) + return _job_from_hash(r, key) return None @staticmethod @@ -174,7 +174,7 @@ class Jobs: """ r = RedisPool().get_connection() jobs = r.keys("jobs:*") - return [job_from_hash(r, job_key) for job_key in jobs] + return [_job_from_hash(r, job_key) for job_key in jobs] @staticmethod def is_busy() -> bool: @@ -187,11 +187,11 @@ class Jobs: return False -def redis_key_from_uuid(uuid): +def _redis_key_from_uuid(uuid): return "jobs:" + str(uuid) -def store_job_as_hash(r, redis_key, model): +def _store_job_as_hash(r, redis_key, model): for key, value in model.dict().items(): if isinstance(value, uuid.UUID): value = str(value) @@ -202,7 +202,7 @@ def store_job_as_hash(r, redis_key, model): r.hset(redis_key, key, str(value)) -def job_from_hash(r, redis_key): +def _job_from_hash(r, redis_key): if r.exists(redis_key): job_dict = r.hgetall(redis_key) for date in [ From 5c86706f4ba3279a84aa463d4b7a4cf368397dc8 Mon Sep 17 00:00:00 2001 From: Houkime <> Date: Wed, 30 Nov 2022 16:41:20 +0000 Subject: [PATCH 08/10] Jobs: fix value access in is_busy() Also added a test for is_busy() that highlighted this bug. --- selfprivacy_api/jobs/__init__.py | 2 +- tests/test_jobs.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/selfprivacy_api/jobs/__init__.py b/selfprivacy_api/jobs/__init__.py index 6ad8493..4267819 100644 --- a/selfprivacy_api/jobs/__init__.py +++ b/selfprivacy_api/jobs/__init__.py @@ -182,7 +182,7 @@ class Jobs: Check if there is a job running. """ for job in Jobs.get_jobs(): - if job["status"] == JobStatus.RUNNING.value: + if job.status == JobStatus.RUNNING: return True return False diff --git a/tests/test_jobs.py b/tests/test_jobs.py index 7ef9a9b..3474fc3 100644 --- a/tests/test_jobs.py +++ b/tests/test_jobs.py @@ -33,6 +33,7 @@ def test_remove_update_nonexistent(jobs_with_one_job): def test_jobs(jobs_with_one_job): jobs = jobs_with_one_job test_job = jobs_with_one_job.get_jobs()[0] + assert not jobs.is_busy() jobs.update( job=test_job, @@ -44,6 +45,7 @@ def test_jobs(jobs_with_one_job): ) assert jobs.get_jobs() == [test_job] + assert jobs.is_busy() backup = jobsmodule.JOB_EXPIRATION_SECONDS jobsmodule.JOB_EXPIRATION_SECONDS = 0 From 063dfafc196ad094765fe70f4a9ee34affa68026 Mon Sep 17 00:00:00 2001 From: Houkime <> Date: Wed, 30 Nov 2022 17:06:43 +0000 Subject: [PATCH 09/10] Jobs: fix return value of remove_by_uid And add a test for said return value. --- selfprivacy_api/jobs/__init__.py | 4 +++- tests/test_jobs.py | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/selfprivacy_api/jobs/__init__.py b/selfprivacy_api/jobs/__init__.py index 4267819..1547b84 100644 --- a/selfprivacy_api/jobs/__init__.py +++ b/selfprivacy_api/jobs/__init__.py @@ -115,7 +115,9 @@ class Jobs: """ r = RedisPool().get_connection() key = _redis_key_from_uuid(job_uuid) - r.delete(key) + if (r.exists(key)): + r.delete(key) + return True return False @staticmethod diff --git a/tests/test_jobs.py b/tests/test_jobs.py index 3474fc3..d0f506c 100644 --- a/tests/test_jobs.py +++ b/tests/test_jobs.py @@ -20,6 +20,15 @@ def test_minimal_update(jobs_with_one_job): assert jobs.get_jobs() == [test_job] +def test_remove_by_uid(jobs_with_one_job): + test_job = jobs_with_one_job.get_jobs()[0] + uid_str = str(test_job.uid) + + assert jobs_with_one_job.remove_by_uid(uid_str) + assert jobs_with_one_job.get_jobs() == [] + assert not jobs_with_one_job.remove_by_uid(uid_str) + + def test_remove_update_nonexistent(jobs_with_one_job): test_job = jobs_with_one_job.get_jobs()[0] From d47368cbe9de081b9c5ebe952c6c7bb66debfebe Mon Sep 17 00:00:00 2001 From: Houkime <> Date: Wed, 30 Nov 2022 17:26:38 +0000 Subject: [PATCH 10/10] Jobs: test get_job() return values Coverage is now at 99% --- tests/test_jobs.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/test_jobs.py b/tests/test_jobs.py index d0f506c..56e4aa3 100644 --- a/tests/test_jobs.py +++ b/tests/test_jobs.py @@ -39,6 +39,16 @@ def test_remove_update_nonexistent(jobs_with_one_job): assert result == test_job # even though we might consider changing this behavior +def test_remove_get_nonexistent(jobs_with_one_job): + test_job = jobs_with_one_job.get_jobs()[0] + uid_str = str(test_job.uid) + assert jobs_with_one_job.get_job(uid_str) == test_job + + jobs_with_one_job.remove(test_job) + + assert jobs_with_one_job.get_job(uid_str) is None + + def test_jobs(jobs_with_one_job): jobs = jobs_with_one_job test_job = jobs_with_one_job.get_jobs()[0]