From 13304cd36dd91f9719822a972a6b43ac930a87ea Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 4 Jan 2025 10:30:30 +0530 Subject: [PATCH 1/4] ci: balance test distribution manually Currently one test runner takes significantly longer than another. This is entirely due to test_commands.py which needs to create new site and do backup/restore tests etc. All of which are far far slower than other tests. --- frappe/{tests => commands}/test_commands.py | 0 frappe/commands/testing.py | 3 +-- frappe/parallel_test_runner.py | 19 +++++++++++++------ 3 files changed, 14 insertions(+), 8 deletions(-) rename frappe/{tests => commands}/test_commands.py (100%) diff --git a/frappe/tests/test_commands.py b/frappe/commands/test_commands.py similarity index 100% rename from frappe/tests/test_commands.py rename to frappe/commands/test_commands.py diff --git a/frappe/commands/testing.py b/frappe/commands/testing.py index d0e3549ed8..10dd126ec3 100644 --- a/frappe/commands/testing.py +++ b/frappe/commands/testing.py @@ -360,8 +360,7 @@ def run_parallel_tests( ║ App: {app:<26} ║ ║ Site: {site:<26} ║ ║ Build Number: {build_number:<26} ║ - ║ Total Builds: {total_builds:<26} ║ - ║ Tests in Build: ~{runner.total_tests:<25} ║""" + ║ Total Builds: {total_builds:<26} ║""" if cc.with_coverage: banner += """ ║ Coverage Rep.: {cc.outfile:<26} ║""" diff --git a/frappe/parallel_test_runner.py b/frappe/parallel_test_runner.py index 8c2a429a9e..c116b7d0f3 100644 --- a/frappe/parallel_test_runner.py +++ b/frappe/parallel_test_runner.py @@ -21,6 +21,12 @@ click_ctx = click.get_current_context(True) if click_ctx: click_ctx.color = True +TEST_WEIGHT_OVERRIDES = { + # XXX: command tests are significantly overweight, need a better heuristic than test count + # Possible better solution: stats from previous test runs. + "test_commands.py": 20, +} + class ParallelTestRunner: def __init__(self, app, site, build_number=1, total_builds=1, dry_run=False): @@ -30,7 +36,7 @@ class ParallelTestRunner: self.total_builds = frappe.utils.cint(total_builds) self.dry_run = dry_run self.test_file_list = [] - self.total_tests = 0 + self.total_test_weight = 0 self.test_result = None self.setup_test_file_list() @@ -70,8 +76,7 @@ class ParallelTestRunner: def setup_test_file_list(self): self.test_file_list = self.get_test_file_list() - self.total_tests = sum(self.get_test_count(test) for test in self.test_file_list) - click.echo(f"Estimated total tests for build {self.build_number}: {self.total_tests}") + self.total_test_weight = sum(self.get_test_weight(test) for test in self.test_file_list) def run_tests(self): self.test_result = TestResult(stream=sys.stderr, descriptions=True, verbosity=2) @@ -136,18 +141,20 @@ class ParallelTestRunner: # Load balance based on total # of tests ~ each runner should get roughly same # of tests. test_list = get_all_tests(self.app) - test_counts = [self.get_test_count(test) for test in test_list] + test_counts = [self.get_test_weight(test) for test in test_list] test_chunks = split_by_weight(test_list, test_counts, chunk_count=self.total_builds) return test_chunks[self.build_number - 1] @staticmethod - def get_test_count(test): + def get_test_weight(test): """Get approximate count of tests inside a file""" file_name = "/".join(test) + test_weight = TEST_WEIGHT_OVERRIDES.get(test[-1]) or 1 + with open(file_name) as f: - test_count = f.read().count("def test_") + test_count = f.read().count("def test_") * test_weight return test_count From 47af97661a36eb0f527d76745291becf2976ad5d Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 4 Jan 2025 11:02:54 +0530 Subject: [PATCH 2/4] test: avoid flaky behaviour on test ordering If a test clears local.response and another test depends on it being fresh inited then tests can fail. E.g. response.docs might not exist. --- frappe/parallel_test_runner.py | 2 +- frappe/tests/classes/integration_test_case.py | 5 ++-- frappe/tests/test_client.py | 23 ++++++++----------- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/frappe/parallel_test_runner.py b/frappe/parallel_test_runner.py index c116b7d0f3..ed2840bfaa 100644 --- a/frappe/parallel_test_runner.py +++ b/frappe/parallel_test_runner.py @@ -24,7 +24,7 @@ if click_ctx: TEST_WEIGHT_OVERRIDES = { # XXX: command tests are significantly overweight, need a better heuristic than test count # Possible better solution: stats from previous test runs. - "test_commands.py": 20, + "test_commands.py": 10, } diff --git a/frappe/tests/classes/integration_test_case.py b/frappe/tests/classes/integration_test_case.py index b52095962f..c438ac5358 100644 --- a/frappe/tests/classes/integration_test_case.py +++ b/frappe/tests/classes/integration_test_case.py @@ -63,7 +63,7 @@ class IntegrationTestCase(UnitTestCase): frappe.db.before_commit.add(_commit_watcher) # enqueue teardown actions (executed in LIFO order) - cls.addClassCleanup(_restore_thread_locals, copy.deepcopy(frappe.local.flags)) + cls.addClassCleanup(_restore_ctx_locals, copy.deepcopy(frappe.local.flags)) cls.addClassCleanup(_rollback_db) cls._integration_test_case_class_setup_done = True @@ -183,12 +183,13 @@ def _rollback_db(): frappe.db.rollback() -def _restore_thread_locals(flags): +def _restore_ctx_locals(flags): frappe.local.flags = flags frappe.local.error_log = [] frappe.local.message_log = [] frappe.local.debug_log = [] frappe.local.conf = frappe._dict(frappe.get_site_config()) + frappe.local.response = frappe._dict({"docs": []}) frappe.local.cache = {} frappe.local.lang = "en" frappe.local.preload_assets = {"style": [], "script": [], "icons": []} diff --git a/frappe/tests/test_client.py b/frappe/tests/test_client.py index e6d0cc9552..588b9fa628 100644 --- a/frappe/tests/test_client.py +++ b/frappe/tests/test_client.py @@ -74,19 +74,16 @@ class TestClient(IntegrationTestCase): def test_run_doc_method(self): from frappe.handler import execute_cmd - if not frappe.db.exists("Report", "Test Run Doc Method"): - report = frappe.get_doc( - { - "doctype": "Report", - "ref_doctype": "User", - "report_name": "Test Run Doc Method", - "report_type": "Query Report", - "is_standard": "No", - "roles": [{"role": "System Manager"}], - } - ).insert() - else: - report = frappe.get_doc("Report", "Test Run Doc Method") + report = frappe.get_doc( + { + "doctype": "Report", + "ref_doctype": "User", + "report_name": frappe.generate_hash(), + "report_type": "Query Report", + "is_standard": "No", + "roles": [{"role": "System Manager"}], + } + ).insert() frappe.local.request = frappe._dict() frappe.local.request.method = "GET" From e137e361800d844b2612030d0cb657a3645dc6f5 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 4 Jan 2025 12:02:10 +0530 Subject: [PATCH 3/4] test: don't sleep in webhook tests Wasting 2 minutes doing nothing. --- .../doctype/webhook/test_webhook.py | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/frappe/integrations/doctype/webhook/test_webhook.py b/frappe/integrations/doctype/webhook/test_webhook.py index 5059d75ea8..23d101df89 100644 --- a/frappe/integrations/doctype/webhook/test_webhook.py +++ b/frappe/integrations/doctype/webhook/test_webhook.py @@ -14,6 +14,7 @@ from frappe.integrations.doctype.webhook.webhook import ( get_webhook_headers, ) from frappe.tests import IntegrationTestCase, UnitTestCase +from frappe.tests.classes.context_managers import timeout @contextmanager @@ -84,10 +85,19 @@ class TestWebhook(IntegrationTestCase): frappe.db.delete("Webhook") frappe.db.commit() + @timeout(5, "Test webhooks should never wait, check mocked responses.") def setUp(self): # retrieve or create a User webhook for `after_insert` self.responses = responses.RequestsMock() self.responses.start() + + self.responses.add( + responses.POST, + "https://httpbin.org/post", + status=200, + json={}, + ) + webhook_fields = { "webhook_doctype": "User", "webhook_docevent": "after_insert", @@ -120,6 +130,7 @@ class TestWebhook(IntegrationTestCase): self.responses.reset() super().tearDown() + @timeout(5, "Test webhooks should never wait, check mocked responses.") def test_webhook_trigger_with_enabled_webhooks(self): """Test webhook trigger for enabled webhooks""" @@ -138,18 +149,21 @@ class TestWebhook(IntegrationTestCase): self.assertEqual(execution.webhook.name, self.sample_webhooks[0].name) self.assertEqual(execution.doc.name, self.test_user.name) + @timeout(5, "Test webhooks should never wait, check mocked responses.") def test_validate_doc_events(self): "Test creating a submit-related webhook for a non-submittable DocType" self.webhook.webhook_docevent = "on_submit" self.assertRaises(frappe.ValidationError, self.webhook.save) + @timeout(5, "Test webhooks should never wait, check mocked responses.") def test_validate_request_url(self): "Test validation for the webhook request URL" self.webhook.request_url = "httpbin.org?post" self.assertRaises(frappe.ValidationError, self.webhook.save) + @timeout(5, "Test webhooks should never wait, check mocked responses.") def test_validate_headers(self): "Test validation for request headers" @@ -165,6 +179,7 @@ class TestWebhook(IntegrationTestCase): headers = get_webhook_headers(doc=None, webhook=self.webhook) self.assertEqual(headers, {"Content-Type": "application/json"}) + @timeout(5, "Test webhooks should never wait, check mocked responses.") def test_validate_request_body_form(self): "Test validation of Form URL-Encoded request body" @@ -179,6 +194,7 @@ class TestWebhook(IntegrationTestCase): data = get_webhook_data(doc=self.user, webhook=self.webhook) self.assertEqual(data, {"name": self.user.name}) + @timeout(5, "Test webhooks should never wait, check mocked responses.") def test_validate_request_body_json(self): "Test validation of JSON request body" @@ -193,6 +209,7 @@ class TestWebhook(IntegrationTestCase): data = get_webhook_data(doc=self.user, webhook=self.webhook) self.assertEqual(data, {"name": self.user.name}) + @timeout(5, "Test webhooks should never wait, check mocked responses.") def test_webhook_req_log_creation(self): self.responses.add( responses.POST, @@ -213,6 +230,7 @@ class TestWebhook(IntegrationTestCase): self.assertTrue(frappe.get_all("Webhook Request Log", pluck="name")) + @timeout(5, "Test webhooks should never wait, check mocked responses.") def test_webhook_with_array_body(self): """Check if array request body are supported.""" wh_config = { @@ -258,6 +276,7 @@ class TestWebhook(IntegrationTestCase): log = frappe.get_last_doc("Webhook Request Log") self.assertEqual(len(json.loads(log.response)), 3) + @timeout(5, "Test webhooks should never wait, check mocked responses.") def test_webhook_with_dynamic_url_enabled(self): wh_config = { "doctype": "Webhook", @@ -289,6 +308,7 @@ class TestWebhook(IntegrationTestCase): doc.title = "Test Webhook Note" enqueue_webhook(doc, wh) + @timeout(5, "Test webhooks should never wait, check mocked responses.") def test_webhook_with_dynamic_url_disabled(self): wh_config = { "doctype": "Webhook", From 6b0d13a5bffdf07ad6239afe9ee7bd88fd1303c5 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 4 Jan 2025 12:14:55 +0530 Subject: [PATCH 4/4] test: avoid rebuilding assets --- .github/actions/setup/action.yml | 2 +- frappe/commands/test_commands.py | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/.github/actions/setup/action.yml b/.github/actions/setup/action.yml index 1be6b0f4eb..29d48831f7 100644 --- a/.github/actions/setup/action.yml +++ b/.github/actions/setup/action.yml @@ -216,7 +216,7 @@ runs: start_time=$(date +%s) source ${GITHUB_WORKSPACE}/env/bin/activate - CI=Yes bench build & + CI=Yes bench build --force --production & build_pid=$! bench --site test_site reinstall --yes wait $build_pid diff --git a/frappe/commands/test_commands.py b/frappe/commands/test_commands.py index 21d9a08be5..e331543c59 100644 --- a/frappe/commands/test_commands.py +++ b/frappe/commands/test_commands.py @@ -897,12 +897,8 @@ class TestAddNewUser(BaseTestCommands): self.assertEqual({"Accounts User", "Sales User"}, roles) -class TestBenchBuild(BaseTestCommands): +class TestBenchBuild(IntegrationTestCase): def test_build_assets_size_check(self): - with cli(frappe.commands.utils.build, "--force --production --app frappe") as result: - self.assertEqual(result.exit_code, 0) - self.assertEqual(result.exception, None) - CURRENT_SIZE = 3.3 # MB JS_ASSET_THRESHOLD = 0.01