feat(ci): Improve test coverage for build and deploy

press/press/doctype/app_release_approval_request/test_app_release_approval_request.py

@@ -1,9 +1,170 @@
 # Copyright (c) 2021, Frappe and Contributors
 # See license.txt
 
-# import frappe
+from unittest.mock import patch
+
+import frappe
 from frappe.tests.utils import FrappeTestCase
 
+from press.press.doctype.app.test_app import create_test_app
+from press.press.doctype.app_release.test_app_release import create_test_app_release
+from press.press.doctype.app_release_approval_request.app_release_approval_request import (
+	AppReleaseApprovalRequest,
+)
+from press.press.doctype.team.test_team import create_test_team
+
+
+def _create_marketplace_app(app, team):
+	return frappe.get_doc(
+		{
+			"doctype": "Marketplace App",
+			"app": app.name,
+			"description": "Test marketplace app",
+			"team": team.name,
+		}
+	).insert(ignore_permissions=True, ignore_if_duplicate=True)
+
+
+def _create_request(marketplace_app, release):
+	"""Create an approval request, patching sendmail to avoid SMTP calls."""
+	with patch("frappe.sendmail"):
+		AppReleaseApprovalRequest.create(marketplace_app.name, release.name)
+	return frappe.get_last_doc("App Release Approval Request", {"app_release": release.name})
+
+
+class TestApprovalRequestGuards(FrappeTestCase):
+	"""before_insert guards on AppReleaseApprovalRequest prevent invalid requests.
+
+	Three guards run before insert:
+	- Duplicate request for the same release must be blocked (double-submit protection)
+	- Another Open request from the same source must be blocked (queue discipline)
+	- Yanked releases must be blocked entirely (audit-failed code must not be re-reviewed)
+	"""
+
+	def setUp(self):
+		super().setUp()
+		frappe.set_user("Administrator")
+		from press.press.doctype.app_source.test_app_source import create_test_app_source
+
+		self.team = create_test_team()
+		self.app = create_test_app()
+		self.source = create_test_app_source("Version 14", self.app, team=self.team.name)
+		self.release = create_test_app_release(self.source)
+		self.mapp = _create_marketplace_app(self.app, self.team)
+
+	def tearDown(self):
+		frappe.set_user("Administrator")
+		frappe.db.rollback()
+
+	def test_creating_request_sets_release_to_awaiting_approval(self):
+		_create_request(self.mapp, self.release)
+		self.release.reload()
+		self.assertEqual(self.release.status, "Awaiting Approval")
+
+	def test_duplicate_request_for_same_release_raises(self):
+		_create_request(self.mapp, self.release)
+		with self.assertRaises(frappe.ValidationError), patch("frappe.sendmail"):
+			AppReleaseApprovalRequest.create(self.mapp.name, self.release.name)
+
+	def test_request_for_yanked_release_raises(self):
+		frappe.db.set_value("App Release", self.release.name, "status", "Yanked")
+		with self.assertRaises(frappe.ValidationError), patch("frappe.sendmail"):
+			AppReleaseApprovalRequest.create(self.mapp.name, self.release.name)
+
+	def test_another_open_request_from_same_source_raises(self):
+		_create_request(self.mapp, self.release)
+		release2 = create_test_app_release(self.source)
+		with self.assertRaises(frappe.ValidationError), patch("frappe.sendmail"):
+			AppReleaseApprovalRequest.create(self.mapp.name, release2.name)
+
+
+class TestApprovalRequestStatusPropagation(FrappeTestCase):
+	"""on_update propagates request status changes back to the linked App Release.
+
+	If this is broken: Approving leaves the release stuck as "Awaiting Approval" and
+	blocks it from Deploy Candidates; Rejecting leaves it non-"Rejected" so new requests
+	from the same source are blocked; Cancelling leaves it non-"Draft" so resubmission
+	is impossible.
+	"""
+
+	def setUp(self):
+		super().setUp()
+		frappe.set_user("Administrator")
+		from press.press.doctype.app_source.test_app_source import create_test_app_source
+
+		self.team = create_test_team()
+		self.app = create_test_app()
+		self.source = create_test_app_source("Version 14", self.app, team=self.team.name)
+		self.release = create_test_app_release(self.source)
+		self.mapp = _create_marketplace_app(self.app, self.team)
+		self.request = _create_request(self.mapp, self.release)
+
+	def tearDown(self):
+		frappe.set_user("Administrator")
+		frappe.db.rollback()
+
+	def test_approved_request_sets_release_to_approved(self):
+		with patch("frappe.sendmail"), patch("frappe.db.commit"):
+			self.request.status = "Approved"
+			# bypass audit validation for unit test
+			with patch.object(self.request, "validate_audit_for_approval"):
+				self.request.save(ignore_permissions=True)
+		self.release.reload()
+		self.assertEqual(self.release.status, "Approved")
+
+	def test_rejected_request_sets_release_to_rejected(self):
+		with patch("frappe.sendmail"), patch("frappe.db.commit"):
+			self.request.status = "Rejected"
+			self.request.save(ignore_permissions=True)
+		self.release.reload()
+		self.assertEqual(self.release.status, "Rejected")
+
+	def test_cancelled_request_sets_release_to_draft(self):
+		with patch("frappe.db.commit"):
+			self.request.status = "Cancelled"
+			self.request.save(ignore_permissions=True)
+		self.release.reload()
+		self.assertEqual(self.release.status, "Draft")
+
+
+class TestApprovalRequestAutoApproval(FrappeTestCase):
+	"""before_save auto-approves requests for featured apps and auto-release teams.
+
+	This mirrors the AppRelease auto-approval but at the request level: a featured
+	app or trusted team should never have to wait for manual review.
+	"""
+
+	def setUp(self):
+		super().setUp()
+		frappe.set_user("Administrator")
+		from press.press.doctype.app_source.test_app_source import create_test_app_source
+
+		self.team = create_test_team()
+		self.app = create_test_app()
+		self.source = create_test_app_source("Version 14", self.app, team=self.team.name)
+		self.release = create_test_app_release(self.source)
+		self.mapp = _create_marketplace_app(self.app, self.team)
+
+	def tearDown(self):
+		frappe.set_user("Administrator")
+		frappe.db.rollback()
+
+	def test_featured_app_request_is_auto_approved(self):
+		ms = frappe.get_single("Marketplace Settings")
+		ms.append("featured_apps", {"app": self.app.name})
+		ms.save(ignore_permissions=True)
+
+		request = _create_request(self.mapp, self.release)
+		self.assertEqual(request.status, "Approved")
+
+	def test_auto_release_team_request_is_auto_approved(self):
+		ms = frappe.get_single("Marketplace Settings")
+		ms.append("auto_release_teams", {"team": self.team.name})
+		ms.save(ignore_permissions=True)
+
+		request = _create_request(self.mapp, self.release)
+		self.assertEqual(request.status, "Approved")
 
-class TestAppReleaseApprovalRequest(FrappeTestCase):
-	pass
+	def test_regular_team_non_featured_app_request_stays_open(self):
+		request = _create_request(self.mapp, self.release)
+		self.assertEqual(request.status, "Open")

press/press/doctype/app_release_difference/test_app_release_difference.py

@@ -1,10 +1,172 @@
 # Copyright (c) 2020, Frappe and Contributors
 # See license.txt
 
-
-# import frappe
+import frappe
 from frappe.tests.utils import FrappeTestCase
 
+from press.press.doctype.app.test_app import create_test_app
+from press.press.doctype.app_release.test_app_release import create_test_app_release
+from press.press.doctype.app_release_difference.app_release_difference import is_migrate_needed
+from press.press.doctype.team.test_team import create_test_team
+
+
+class TestIsMigrateNeeded(FrappeTestCase):
+	"""is_migrate_needed classifies changed file paths as requiring a database
+	migration (bench migrate) or not.
+
+	A wrong False skips data patches or schema changes and silently corrupts the
+	database. A wrong True forces an unnecessary bench migrate on every trivial
+	.py commit, adding minutes to each deploy.
+	"""
+
+	# --- files that require a migrate ---
+
+	def test_patches_txt_requires_migrate(self):
+		self.assertTrue(is_migrate_needed(["frappe/patches.txt"]))
+
+	def test_hooks_py_requires_migrate(self):
+		self.assertTrue(is_migrate_needed(["frappe/hooks.py"]))
+
+	def test_fixtures_directory_requires_migrate(self):
+		# regex: \w+/fixtures/ — one word segment before fixtures/
+		self.assertTrue(is_migrate_needed(["erpnext/fixtures/user_type.json"]))
+
+	def test_custom_directory_requires_migrate(self):
+		# regex: \w+/\w+/custom/ — two word segments before custom/
+		self.assertTrue(is_migrate_needed(["erpnext/accounts/custom/custom_field.json"]))
+
+	def test_languages_json_requires_migrate(self):
+		self.assertTrue(is_migrate_needed(["frappe/geo/languages.json"]))
+
+	def test_doctype_json_requires_migrate(self):
+		# Pattern \w+/\w+/\w+/(.+)/\1\.json matches <app>/<pkg>/<module>/<name>/<name>.json
+		self.assertTrue(is_migrate_needed(["erpnext/erpnext/accounts/account/account.json"]))
+
+	def test_one_migrate_file_in_mixed_batch_requires_migrate(self):
+		self.assertTrue(
+			is_migrate_needed(
+				[
+					"frappe/frappe/core/doctype/user/user.py",
+					"frappe/patches.txt",
+				]
+			)
+		)
+
+	def test_nested_patches_path_requires_migrate(self):
+		# The regex \w+/patches\.txt also matches nested paths
+		self.assertTrue(is_migrate_needed(["erpnext/patches.txt"]))
+
+	# --- files that do not require a migrate ---
+
+	def test_python_file_no_migrate(self):
+		self.assertFalse(is_migrate_needed(["frappe/frappe/core/doctype/user/user.py"]))
+
+	def test_js_file_no_migrate(self):
+		self.assertFalse(is_migrate_needed(["frappe/frappe/public/js/frappe.js"]))
+
+	def test_empty_file_list_no_migrate(self):
+		self.assertFalse(is_migrate_needed([]))
+
+	def test_multiple_safe_py_files_no_migrate(self):
+		self.assertFalse(
+			is_migrate_needed(
+				[
+					"frappe/frappe/core/doctype/user/user.py",
+					"erpnext/erpnext/accounts/account/account.py",
+				]
+			)
+		)
+
+
+class TestAppReleaseDifferenceValidate(FrappeTestCase):
+	"""AppReleaseDifference.validate blocks same-release diffs.
+
+	Creating a diff from a release to itself is meaningless — the deploy pipeline
+	would see an empty diff and skip the migrate, even though the source and
+	destination are the same commit (making the diff a no-op, not a skip).
+	"""
+
+	def setUp(self):
+		super().setUp()
+		frappe.set_user("Administrator")
+		from press.press.doctype.app_source.test_app_source import create_test_app_source
+
+		self.team = create_test_team()
+		self.app = create_test_app()
+		self.source = create_test_app_source("Version 14", self.app, team=self.team.name)
+		self.release = create_test_app_release(self.source)
+
+	def tearDown(self):
+		frappe.set_user("Administrator")
+		frappe.db.rollback()
+
+	def test_same_source_and_destination_release_raises(self):
+		with self.assertRaises(frappe.ValidationError):
+			frappe.get_doc(
+				{
+					"doctype": "App Release Difference",
+					"app": self.app.name,
+					"source": self.source.name,
+					"source_release": self.release.name,
+					"destination_release": self.release.name,
+				}
+			).insert(ignore_permissions=True)
+
+	def test_different_releases_do_not_raise(self):
+		release2 = create_test_app_release(self.source)
+		doc = frappe.get_doc(
+			{
+				"doctype": "App Release Difference",
+				"app": self.app.name,
+				"source": self.source.name,
+				"source_release": self.release.name,
+				"destination_release": release2.name,
+			}
+		).insert(ignore_permissions=True)
+		self.assertIsNotNone(doc.name)
+
+
+class TestHasBranchChanged(FrappeTestCase):
+	"""AppReleaseDifference.has_branch_changed detects source-to-destination branch switches.
+
+	When two releases come from different branches, a full migrate is forced because
+	a branch switch may skip commits containing patches or schema changes.
+	"""
+
+	def setUp(self):
+		super().setUp()
+		frappe.set_user("Administrator")
+		from press.press.doctype.app_source.test_app_source import create_test_app_source
+
+		self.team = create_test_team()
+		self.app = create_test_app()
+		self.source_main = create_test_app_source("Version 14", self.app, team=self.team.name, branch="main")
+		self.source_develop = create_test_app_source(
+			"Version 14", self.app, team=self.team.name, branch="develop"
+		)
+		self.release_main = create_test_app_release(self.source_main)
+		self.release_develop = create_test_app_release(self.source_develop)
+
+	def tearDown(self):
+		frappe.set_user("Administrator")
+		frappe.db.rollback()
+
+	def _diff(self, source_release, destination_release):
+		return frappe.get_doc(
+			{
+				"doctype": "App Release Difference",
+				"app": self.app.name,
+				"source": self.source_main.name,
+				"source_release": source_release,
+				"destination_release": destination_release,
+			}
+		).insert(ignore_permissions=True)
+
+	def test_different_branches_returns_true(self):
+		diff = self._diff(self.release_main.name, self.release_develop.name)
+		self.assertTrue(diff.has_branch_changed())
 
-class TestAppReleaseDifference(FrappeTestCase):
-	pass
+	def test_same_branch_returns_false(self):
+		release2 = create_test_app_release(self.source_main)
+		diff = self._diff(self.release_main.name, release2.name)
+		self.assertFalse(diff.has_branch_changed())