From ec0817d1821e6aa72b23820a61224b70136277d1 Mon Sep 17 00:00:00 2001 From: Sambasiva Suda Date: Wed, 18 Oct 2023 01:14:54 +0530 Subject: [PATCH 1/5] fix: cron job firing immediately after save --- frappe/core/doctype/scheduled_job_type/scheduled_job_type.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py b/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py index fc84204b37..a209a1d720 100644 --- a/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py +++ b/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py @@ -106,7 +106,7 @@ class ScheduledJobType(Document): self.cron_format = CRON_MAP[self.frequency] return croniter( - self.cron_format, get_datetime(self.last_execution or datetime(2000, 1, 1)) + self.cron_format, get_datetime(self.last_execution) if self.last_execution else now_datetime() ).get_next(datetime) def execute(self): From 6193f23fd60c88855d515ade1f63818a668c897d Mon Sep 17 00:00:00 2001 From: Sambasiva Suda Date: Wed, 18 Oct 2023 01:28:59 +0530 Subject: [PATCH 2/5] chore: adding test case --- .../doctype/scheduled_job_type/test_scheduled_job_type.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py b/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py index 7edad24ac4..74783b63e7 100644 --- a/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py +++ b/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py @@ -4,6 +4,7 @@ import frappe from frappe.core.doctype.scheduled_job_type.scheduled_job_type import sync_jobs from frappe.tests.utils import FrappeTestCase from frappe.utils import get_datetime +from frappe.utils.data import add_to_date class TestScheduledJobType(FrappeTestCase): @@ -65,9 +66,16 @@ class TestScheduledJobType(FrappeTestCase): self.assertFalse(job.is_event_due(get_datetime("2019-01-31 23:59:59"))) def test_cron_job(self): + # Daily but offset by 45 minutes + job = frappe.get_doc("Scheduled Job Type", dict(method="frappe.core.doctype.log_settings.log_settings.run_log_clean_up")) + self.assertEqual( + job.next_execution, + add_to_date(None, days=1).replace(hour=0, minute=45, second=0, microsecond=0) + ) # runs every 15 mins job = frappe.get_doc("Scheduled Job Type", dict(method="frappe.oauth.delete_oauth2_data")) job.db_set("last_execution", "2019-01-01 00:00:00") + self.assertEqual(job.next_execution, get_datetime("2019-01-01 00:15:00")) self.assertTrue(job.is_event_due(get_datetime("2019-01-01 00:15:01"))) self.assertFalse(job.is_event_due(get_datetime("2019-01-01 00:05:06"))) self.assertFalse(job.is_event_due(get_datetime("2019-01-01 00:14:59"))) From a8452484a05da182d1a2588d990e9149bef6ba71 Mon Sep 17 00:00:00 2001 From: Sambasiva Suda Date: Wed, 18 Oct 2023 01:55:33 +0530 Subject: [PATCH 3/5] chore: linter fix --- .../core/doctype/scheduled_job_type/scheduled_job_type.py | 2 +- .../doctype/scheduled_job_type/test_scheduled_job_type.py | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py b/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py index a209a1d720..9174dcbf0a 100644 --- a/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py +++ b/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py @@ -106,7 +106,7 @@ class ScheduledJobType(Document): self.cron_format = CRON_MAP[self.frequency] return croniter( - self.cron_format, get_datetime(self.last_execution) if self.last_execution else now_datetime() + self.cron_format, get_datetime(self.last_execution or now_datetime()) ).get_next(datetime) def execute(self): diff --git a/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py b/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py index 74783b63e7..37b975bcc6 100644 --- a/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py +++ b/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py @@ -67,10 +67,13 @@ class TestScheduledJobType(FrappeTestCase): def test_cron_job(self): # Daily but offset by 45 minutes - job = frappe.get_doc("Scheduled Job Type", dict(method="frappe.core.doctype.log_settings.log_settings.run_log_clean_up")) + job = frappe.get_doc( + "Scheduled Job Type", + dict(method="frappe.core.doctype.log_settings.log_settings.run_log_clean_up"), + ) self.assertEqual( job.next_execution, - add_to_date(None, days=1).replace(hour=0, minute=45, second=0, microsecond=0) + add_to_date(None, days=1).replace(hour=0, minute=45, second=0, microsecond=0), ) # runs every 15 mins job = frappe.get_doc("Scheduled Job Type", dict(method="frappe.oauth.delete_oauth2_data")) From d4c49a70c20c083ffe1ecd2cb83d88a52065a712 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 18 Oct 2023 12:56:05 +0530 Subject: [PATCH 4/5] fix: handle cold start edge case --- .../doctype/scheduled_job_type/scheduled_job_type.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py b/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py index 9174dcbf0a..6f56180c89 100644 --- a/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py +++ b/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py @@ -105,9 +105,12 @@ class ScheduledJobType(Document): if not self.cron_format: self.cron_format = CRON_MAP[self.frequency] - return croniter( - self.cron_format, get_datetime(self.last_execution or now_datetime()) - ).get_next(datetime) + # If this is a cold start then last_execution will not be set. + # Creation is set as fallback because if very old fallback is set job might trigger + # immediately, even when it's meant to be daily. + # A dynamic fallback like current time might miss the scheduler interval and job will never start. + last_execution = get_datetime(self.last_execution or self.creation) + return croniter(self.cron_format, last_execution).get_next(datetime) def execute(self): self.scheduler_log = None From 05d6f5cc8a4f6eff1039bc2e158cfe5451348937 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 18 Oct 2023 14:20:31 +0530 Subject: [PATCH 5/5] test: Add cold start tests --- .../test_scheduled_job_type.py | 19 ++++++++++++++++++- frappe/tests/test_test_utils.py | 10 ++++++++++ frappe/tests/utils.py | 14 ++++++++++++++ pyproject.toml | 1 + 4 files changed, 43 insertions(+), 1 deletion(-) diff --git a/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py b/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py index 37b975bcc6..0db573c5b9 100644 --- a/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py +++ b/frappe/core/doctype/scheduled_job_type/test_scheduled_job_type.py @@ -1,10 +1,12 @@ # Copyright (c) 2019, Frappe Technologies and Contributors # License: MIT. See LICENSE +from datetime import timedelta + import frappe from frappe.core.doctype.scheduled_job_type.scheduled_job_type import sync_jobs from frappe.tests.utils import FrappeTestCase from frappe.utils import get_datetime -from frappe.utils.data import add_to_date +from frappe.utils.data import add_to_date, now_datetime class TestScheduledJobType(FrappeTestCase): @@ -82,3 +84,18 @@ class TestScheduledJobType(FrappeTestCase): self.assertTrue(job.is_event_due(get_datetime("2019-01-01 00:15:01"))) self.assertFalse(job.is_event_due(get_datetime("2019-01-01 00:05:06"))) self.assertFalse(job.is_event_due(get_datetime("2019-01-01 00:14:59"))) + + def test_cold_start(self): + now = now_datetime() + just_before_12_am = now.replace(hour=11, minute=59, second=30) + just_after_12_am = now.replace(hour=0, minute=0, second=30) + timedelta(days=1) + + job = frappe.new_doc("Scheduled Job Type") + job.frequency = "Daily" + job.set_user_and_timestamp() + + with self.freeze_time(just_before_12_am): + self.assertFalse(job.is_event_due()) + + with self.freeze_time(just_after_12_am): + self.assertTrue(job.is_event_due()) diff --git a/frappe/tests/test_test_utils.py b/frappe/tests/test_test_utils.py index 4e5c424ca6..de004ac028 100644 --- a/frappe/tests/test_test_utils.py +++ b/frappe/tests/test_test_utils.py @@ -1,5 +1,8 @@ +from datetime import timedelta + import frappe from frappe.tests.utils import FrappeTestCase, change_settings +from frappe.utils.data import now_datetime class TestTestUtils(FrappeTestCase): @@ -27,6 +30,13 @@ class TestTestUtils(FrappeTestCase): restored_settings = frappe.get_system_settings("logout_on_password_reset") self.assertEqual(current_setting, restored_settings) + def test_time_freezing(self): + now = now_datetime() + + tomorrow = now + timedelta(days=1) + with self.freeze_time(tomorrow): + self.assertEqual(now_datetime(), tomorrow) + def tearDownModule(): """assertions for ensuring tests didn't leave state behind""" diff --git a/frappe/tests/utils.py b/frappe/tests/utils.py index 77fc740d33..ee34fb0a5e 100644 --- a/frappe/tests/utils.py +++ b/frappe/tests/utils.py @@ -7,9 +7,12 @@ from collections.abc import Sequence from contextlib import contextmanager from unittest.mock import patch +import pytz + import frappe from frappe.model.base_document import BaseDocument, get_controller from frappe.utils import cint +from frappe.utils.data import convert_utc_to_timezone, get_datetime, get_system_timezone datetime_like_types = (datetime.datetime, datetime.date, datetime.time, datetime.timedelta) @@ -154,6 +157,17 @@ class FrappeTestCase(unittest.TestCase): frappe.init(old_site, force=True) frappe.connect() + @contextmanager + def freeze_time(self, time_to_freeze, *args, **kwargs): + from freezegun import freeze_time + + # Freeze time expects UTC or tzaware objects. We have neither, so convert to UTC. + timezone = pytz.timezone(get_system_timezone()) + fake_time_with_tz = timezone.localize(get_datetime(time_to_freeze)).astimezone(pytz.utc) + + with freeze_time(fake_time_with_tz, *args, **kwargs): + yield + class MockedRequestTestCase(FrappeTestCase): def setUp(self): diff --git a/pyproject.toml b/pyproject.toml index b0521bbf0a..6a829aa885 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -106,3 +106,4 @@ unittest-xml-reporting = "~=3.2.0" watchdog = "~=3.0.0" hypothesis = "~=6.77.0" responses = "==0.23.1" +freezegun = "~=1.2.2"