From 6ac1d955843adbde8169824d06fc91c10ca82110 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 15 Apr 2022 11:37:50 +0530 Subject: [PATCH 1/5] test: fix badly written tests --- frappe/tests/test_permissions.py | 33 ++++++++++--------- .../doctype/blog_post/test_blog_post.py | 6 ++-- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/frappe/tests/test_permissions.py b/frappe/tests/test_permissions.py index 70297a4f54..4164b0be36 100644 --- a/frappe/tests/test_permissions.py +++ b/frappe/tests/test_permissions.py @@ -24,25 +24,26 @@ test_dependencies = ["Blogger", "Blog Post", "User", "Contact", "Salutation"] class TestPermissions(FrappeTestCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + frappe.clear_cache(doctype="Blog Post") + user = frappe.get_doc("User", "test1@example.com") + user.add_roles("Website Manager") + user.add_roles("System Manager") + + user = frappe.get_doc("User", "test2@example.com") + user.add_roles("Blogger") + + user = frappe.get_doc("User", "test3@example.com") + user.add_roles("Sales User") + + user = frappe.get_doc("User", "testperm@example.com") + user.add_roles("Website Manager") + def setUp(self): frappe.clear_cache(doctype="Blog Post") - if not frappe.flags.permission_user_setup_done: - user = frappe.get_doc("User", "test1@example.com") - user.add_roles("Website Manager") - user.add_roles("System Manager") - - user = frappe.get_doc("User", "test2@example.com") - user.add_roles("Blogger") - - user = frappe.get_doc("User", "test3@example.com") - user.add_roles("Sales User") - - user = frappe.get_doc("User", "testperm@example.com") - user.add_roles("Website Manager") - - frappe.flags.permission_user_setup_done = True - reset("Blogger") reset("Blog Post") diff --git a/frappe/website/doctype/blog_post/test_blog_post.py b/frappe/website/doctype/blog_post/test_blog_post.py index 0eddad4bfe..558795458b 100644 --- a/frappe/website/doctype/blog_post/test_blog_post.py +++ b/frappe/website/doctype/blog_post/test_blog_post.py @@ -1,12 +1,12 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE import re -import unittest from bs4 import BeautifulSoup import frappe from frappe.custom.doctype.customize_form.customize_form import reset_customization +from frappe.tests.utils import FrappeTestCase from frappe.utils import random_string, set_request from frappe.website.doctype.blog_post.blog_post import get_blog_list from frappe.website.serve import get_response @@ -16,7 +16,7 @@ from frappe.website.website_generator import WebsiteGenerator test_dependencies = ["Blog Post"] -class TestBlogPost(unittest.TestCase): +class TestBlogPost(FrappeTestCase): def setUp(self): reset_customization("Blog Post") @@ -61,7 +61,7 @@ class TestBlogPost(unittest.TestCase): category_page_link = list(soup.find_all("a", href=re.compile(blog.blog_category)))[0] category_page_url = category_page_link["href"] - cached_value = frappe.db.value_cache[("DocType", "Blog Post", "name")] + cached_value = frappe.db.value_cache.get(("DocType", "Blog Post", "name")) frappe.db.value_cache[("DocType", "Blog Post", "name")] = (("Blog Post",),) # Visit the category page (by following the link found in above stage) From f748ae85fc8f51b4c4dcf9029f452703aa238b7a Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 15 Apr 2022 11:48:13 +0530 Subject: [PATCH 2/5] fix: set docstatus to 0 if None present --- frappe/model/base_document.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/model/base_document.py b/frappe/model/base_document.py index f8d60d0763..fd8bb8e71d 100644 --- a/frappe/model/base_document.py +++ b/frappe/model/base_document.py @@ -348,7 +348,7 @@ class BaseDocument(object): @property def docstatus(self): - return DocStatus(self.get("docstatus")) + return DocStatus(cint(self.get("docstatus"))) @docstatus.setter def docstatus(self, value): From 8a0a5c54da9caa0fc3323a90415d9e8e045360cc Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 15 Apr 2022 11:56:34 +0530 Subject: [PATCH 3/5] test!: dont autocommit on test object recreation --- frappe/parallel_test_runner.py | 6 +++--- frappe/test_runner.py | 29 ++++++++++++++++------------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/frappe/parallel_test_runner.py b/frappe/parallel_test_runner.py index 65c8eb470b..4fd03773ef 100644 --- a/frappe/parallel_test_runner.py +++ b/frappe/parallel_test_runner.py @@ -46,7 +46,7 @@ class ParallelTestRunner: if hasattr(test_module, "global_test_dependencies"): for doctype in test_module.global_test_dependencies: - make_test_records(doctype) + make_test_records(doctype, commit=True) elapsed = time.time() - start_time elapsed = click.style(f" ({elapsed:.03}s)", fg="red") @@ -76,7 +76,7 @@ class ParallelTestRunner: def create_test_dependency_records(self, module, path, filename): if hasattr(module, "test_dependencies"): for doctype in module.test_dependencies: - make_test_records(doctype) + make_test_records(doctype, commit=True) if os.path.basename(os.path.dirname(path)) == "doctype": # test_data_migration_connector.py > data_migration_connector.json @@ -86,7 +86,7 @@ class ParallelTestRunner: with open(test_record_file_path, "r") as f: doc = json.loads(f.read()) doctype = doc["name"] - make_test_records(doctype) + make_test_records(doctype, commit=True) def get_module(self, path, filename): app_path = frappe.get_pymodule_path(self.app) diff --git a/frappe/test_runner.py b/frappe/test_runner.py index 509be36f86..96feac532f 100644 --- a/frappe/test_runner.py +++ b/frappe/test_runner.py @@ -226,7 +226,7 @@ def run_tests_for_doctype( if force: for name in frappe.db.sql_list("select name from `tab%s`" % doctype): frappe.delete_doc(doctype, name, force=True) - make_test_records(doctype, verbose=verbose, force=force) + make_test_records(doctype, verbose=verbose, force=force, commit=True) modules.append(importlib.import_module(test_module)) return _run_unittest( @@ -245,7 +245,7 @@ def run_tests_for_module( module = importlib.import_module(module) if hasattr(module, "test_dependencies"): for doctype in module.test_dependencies: - make_test_records(doctype, verbose=verbose) + make_test_records(doctype, verbose=verbose, commit=True) frappe.db.commit() return _run_unittest( @@ -330,7 +330,7 @@ def _add_test(app, path, filename, verbose, test_suite=None, ui_tests=False): if hasattr(module, "test_dependencies"): for doctype in module.test_dependencies: - make_test_records(doctype, verbose=verbose) + make_test_records(doctype, verbose=verbose, commit=True) is_ui_test = True if hasattr(module, "TestDriver") else False @@ -346,12 +346,12 @@ def _add_test(app, path, filename, verbose, test_suite=None, ui_tests=False): with open(txt_file, "r") as f: doc = json.loads(f.read()) doctype = doc["name"] - make_test_records(doctype, verbose) + make_test_records(doctype, verbose, commit=True) test_suite.addTest(unittest.TestLoader().loadTestsFromModule(module)) -def make_test_records(doctype, verbose=0, force=False): +def make_test_records(doctype, verbose=0, force=False, commit=False): if not frappe.db: frappe.connect() @@ -364,8 +364,8 @@ def make_test_records(doctype, verbose=0, force=False): if options not in frappe.local.test_objects: frappe.local.test_objects[options] = [] - make_test_records(options, verbose, force) - make_test_records_for_doctype(options, verbose, force) + make_test_records(options, verbose, force, commit=commit) + make_test_records_for_doctype(options, verbose, force, commit=commit) def get_modules(doctype): @@ -405,7 +405,7 @@ def get_dependencies(doctype): return options_list -def make_test_records_for_doctype(doctype, verbose=0, force=False): +def make_test_records_for_doctype(doctype, verbose=0, force=False, commit=False): if not force and doctype in get_test_record_log(): return @@ -420,17 +420,19 @@ def make_test_records_for_doctype(doctype, verbose=0, force=False): elif hasattr(test_module, "test_records"): if doctype in frappe.local.test_objects: frappe.local.test_objects[doctype] += make_test_objects( - doctype, test_module.test_records, verbose, force + doctype, test_module.test_records, verbose, force, commit=commit ) else: frappe.local.test_objects[doctype] = make_test_objects( - doctype, test_module.test_records, verbose, force + doctype, test_module.test_records, verbose, force, commit=commit ) else: test_records = frappe.get_test_records(doctype) if test_records: - frappe.local.test_objects[doctype] += make_test_objects(doctype, test_records, verbose, force) + frappe.local.test_objects[doctype] += make_test_objects( + doctype, test_records, verbose, force, commit=commit + ) elif verbose: print_mandatory_fields(doctype) @@ -438,7 +440,7 @@ def make_test_records_for_doctype(doctype, verbose=0, force=False): add_to_test_record_log(doctype) -def make_test_objects(doctype, test_records=None, verbose=None, reset=False): +def make_test_objects(doctype, test_records=None, verbose=None, reset=False, commit=False): """Make test objects from given list of `test_records` or from `test_records.json`""" records = [] @@ -495,7 +497,8 @@ def make_test_objects(doctype, test_records=None, verbose=None, reset=False): records.append(d.name) - frappe.db.commit() + if commit: + frappe.db.commit() return records From 7274014b3b453a537d7bafc6c6fc37725fd143d0 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 22 Mar 2022 17:22:27 +0530 Subject: [PATCH 4/5] test: improve FrappeTestCase - discard state after test finishes - add assertDocumentEqual for quick document check - add commit "watcher" to find commits during tests - add tests for tests. who watches the watchmen? --- frappe/tests/test_test_utils.py | 34 +++++++++++++++ frappe/tests/utils.py | 77 ++++++++++++++++++++++++++++++--- 2 files changed, 104 insertions(+), 7 deletions(-) create mode 100644 frappe/tests/test_test_utils.py diff --git a/frappe/tests/test_test_utils.py b/frappe/tests/test_test_utils.py new file mode 100644 index 0000000000..4e5c424ca6 --- /dev/null +++ b/frappe/tests/test_test_utils.py @@ -0,0 +1,34 @@ +import frappe +from frappe.tests.utils import FrappeTestCase, change_settings + + +class TestTestUtils(FrappeTestCase): + SHOW_TRANSACTION_COMMIT_WARNINGS = True + + def test_document_assertions(self): + + currency = frappe.new_doc("Currency") + currency.currency_name = "STONKS" + currency.smallest_currency_fraction_value = 0.420_001 + currency.save() + + self.assertDocumentEqual(currency.as_dict(), currency) + + def test_thread_locals(self): + frappe.flags.temp_flag_to_be_discarded = True + + def test_temp_setting_changes(self): + current_setting = frappe.get_system_settings("logout_on_password_reset") + + with change_settings("System Settings", {"logout_on_password_reset": int(not current_setting)}): + updated_settings = frappe.get_system_settings("logout_on_password_reset") + self.assertNotEqual(current_setting, updated_settings) + + restored_settings = frappe.get_system_settings("logout_on_password_reset") + self.assertEqual(current_setting, restored_settings) + + +def tearDownModule(): + """assertions for ensuring tests didn't leave state behind""" + assert "temp_flag_to_be_discarded" not in frappe.flags + assert not frappe.db.exists("Currency", "STONKS") diff --git a/frappe/tests/utils.py b/frappe/tests/utils.py index bad368afd0..d5c25db26a 100644 --- a/frappe/tests/utils.py +++ b/frappe/tests/utils.py @@ -1,23 +1,86 @@ import copy +import datetime import signal import unittest from contextlib import contextmanager import frappe +from frappe.model.base_document import BaseDocument +from frappe.utils import cint + +datetime_like_types = (datetime.datetime, datetime.date, datetime.time, datetime.timedelta) class FrappeTestCase(unittest.TestCase): """Base test class for Frappe tests.""" - @classmethod - def setUpClass(cls) -> None: - frappe.db.commit() - return super().setUpClass() + SHOW_TRANSACTION_COMMIT_WARNINGS = False @classmethod - def tearDownClass(cls) -> None: - frappe.db.rollback() - return super().tearDownClass() + def setUpClass(cls) -> None: + # flush changes done so far to avoid flake + frappe.db.commit() + if cls.SHOW_TRANSACTION_COMMIT_WARNINGS: + frappe.db.add_before_commit(_commit_watcher) + + # enqueue teardown actions (executed in LIFO order) + cls.addClassCleanup(_restore_thread_locals, copy.deepcopy(frappe.local.flags)) + cls.addClassCleanup(_rollback_db) + + return super().setUpClass() + + # --- Frappe Framework specific assertions + def assertDocumentEqual(self, expected, actual): + """Compare a (partial) expected document with actual Document.""" + + if isinstance(expected, BaseDocument): + expected = expected.as_dict() + + for field, value in expected.items(): + if isinstance(value, list): + actual_child_docs = actual.get(field) + self.assertEqual(len(value), len(actual_child_docs), msg=f"{field} length should be same") + for exp_child, actual_child in zip(value, actual_child_docs): + self.assertDocumentEqual(exp_child, actual_child) + else: + self._compare_field(value, actual.get(field), actual, field) + + def _compare_field(self, expected, actual, doc, field): + msg = f"{field} should be same." + + if isinstance(expected, float): + precision = doc.precision(field) + self.assertAlmostEqual(expected, actual, f"{field} should be same to {precision} digits") + elif isinstance(expected, (bool, int)): + self.assertEqual(expected, cint(actual), msg=msg) + elif isinstance(expected, datetime_like_types): + self.assertEqual(str(expected), str(actual), msg=msg) + else: + self.assertEqual(expected, actual, msg=msg) + + +def _commit_watcher(): + import traceback + + print("Warning:, transaction committed during tests.") + traceback.print_stack(limit=5) + + +def _rollback_db(): + frappe.local.before_commit = [] + frappe.local.rollback_observers = [] + frappe.db.value_cache = {} + frappe.db.rollback() + + +def _restore_thread_locals(flags): + frappe.local.flags = flags + frappe.local.error_log = [] + frappe.local.message_log = [] + frappe.local.debug_log = [] + frappe.local.realtime_log = [] + frappe.local.conf = frappe._dict(frappe.get_site_config()) + frappe.local.cache = {} @contextmanager From 1fa60ba3e729b5e006b592c53a94c3466eca8b05 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 23 Apr 2022 13:08:02 +0530 Subject: [PATCH 5/5] test: explicitly start transaction Postgres for some reason is going in autocommit mode if transaction isn't started with BEGIN... will fix it separately. --- frappe/tests/utils.py | 1 + 1 file changed, 1 insertion(+) diff --git a/frappe/tests/utils.py b/frappe/tests/utils.py index d5c25db26a..7d00a0c1f9 100644 --- a/frappe/tests/utils.py +++ b/frappe/tests/utils.py @@ -20,6 +20,7 @@ class FrappeTestCase(unittest.TestCase): def setUpClass(cls) -> None: # flush changes done so far to avoid flake frappe.db.commit() + frappe.db.begin() if cls.SHOW_TRANSACTION_COMMIT_WARNINGS: frappe.db.add_before_commit(_commit_watcher)