From patchwork Sat Apr 4 21:29:10 2026 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Simon Glass X-Patchwork-Id: 2148 Return-Path: X-Original-To: u-boot-concept@u-boot.org Delivered-To: u-boot-concept@u-boot.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1775338410; bh=xbHjrSDHSuD3+ZKgtU2a03tlOfbz1mWKKLhmxk+zS+k=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=wwPZDd9YYm73Gxk7MWvYgleQtlIqf2XLYDKNdQ12qE7oRbeE8t1uy8BESyG0bLD5q kWVVXVj7KW+AIWtsTNiFJ9JJpC1lgGjXbloeZGEQsraVFiOPzG0McUaY5YrM7Jgnpz C0/KB8tws9TLdnX855pgaQ42zDsN5hx5irLO+E+vagh7NEvWglwCrKv9/yWBbqAY4C smVbMwS1E2XkbUNaCSDGUt5k6+GISBPVf6rY3g6JC2xgVcH2USWVWZLn6/KJj0jGS7 Q76q3e2+TPMP9PIEKHIWCAUhtuV3WOb6A591QtuB7cl5Fj+K0vLJmJ7csOb3HeJBP3 CdeyhXEebf/4w== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 7788D69002 for ; Sat, 4 Apr 2026 15:33:30 -0600 (MDT) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10024) with ESMTP id OZznrAw_nhZE for ; Sat, 4 Apr 2026 15:33:30 -0600 (MDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1775338410; bh=xbHjrSDHSuD3+ZKgtU2a03tlOfbz1mWKKLhmxk+zS+k=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=wwPZDd9YYm73Gxk7MWvYgleQtlIqf2XLYDKNdQ12qE7oRbeE8t1uy8BESyG0bLD5q kWVVXVj7KW+AIWtsTNiFJ9JJpC1lgGjXbloeZGEQsraVFiOPzG0McUaY5YrM7Jgnpz C0/KB8tws9TLdnX855pgaQ42zDsN5hx5irLO+E+vagh7NEvWglwCrKv9/yWBbqAY4C smVbMwS1E2XkbUNaCSDGUt5k6+GISBPVf6rY3g6JC2xgVcH2USWVWZLn6/KJj0jGS7 Q76q3e2+TPMP9PIEKHIWCAUhtuV3WOb6A591QtuB7cl5Fj+K0vLJmJ7csOb3HeJBP3 CdeyhXEebf/4w== Received: from mail.u-boot.org (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id 66BE56833B for ; Sat, 4 Apr 2026 15:33:30 -0600 (MDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1775338408; bh=BsFMJedx4GYx4VtoYUSidZ4SmBy5Ienj2jmTdo6BYVA=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=d5OXb4bUCKE4fqUhYECdkCIRc6eWUoyWEja91xD6/7JnPCctjyKKn3J+AKPXVvpk0 Sbl8q9K4uInD4P2jnZJLO4hjDY52yQ30mWI5FLWoj/TeC+EUsppAikXius77UXhCEK mOb1GYvtEJkeQYWTS5cM329xwSH/3gMv42WecKwofiafpFUXQGQJsxoWWl0kLE4JNk mEJAhV+GVgPS79YrcdFYlpfJ+Pwz5pzhndi/tVsY+PthleQTAugE0k8XvPqkgWNtjQ xtLOqP1SSonikZvwdHsQPoOjjJHKiDdikdl3o+iFB2B422G7brReP7FYtgGmuQPx8x PTbBsb0LfHiJw== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id C4AF264DB2; Sat, 4 Apr 2026 15:33:28 -0600 (MDT) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10026) with ESMTP id oqm1o-o8sHIT; Sat, 4 Apr 2026 15:33:28 -0600 (MDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1775338404; bh=zNKDn/cdoZn+h2yspeQVGE4NGhG3ri1lHo0jDMzaEic=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=nOieGkIh+qNKFe7gQwErThimYa9T8Ndyg0BxDc+aGDMtRsccg4le3ODAxLOy8H88Q Lxo6CedD5EwYuJ6P/1sf9xyPxhHYJ5TjSXxS3dLMmOKefea6Io1qw2f9shszdYtuIP 4H/AO65a5lEujnsbSx5rdyMRWAmi3yXzyy3nUg49Egd3yW5hCDIiO6W0Pc0uT0evwA h4aaZjXKSIUVSKn7e9aSNYBq3+R0ad9gcR8IX282aNJnq1Bi4QIiRKwJsdEGtdncmq 3CbRavQF4WA1icOt8NDO/FPoAF8vztt1Lit50MDyF3TSeGjQ+jSfuPWTm9bTzigish tAeHk18Gdj61g== Received: from u-boot.org (unknown [73.34.74.121]) by mail.u-boot.org (Postfix) with ESMTPSA id 372785E7B4; Sat, 4 Apr 2026 15:33:24 -0600 (MDT) From: Simon Glass To: U-Boot Concept Date: Sat, 4 Apr 2026 15:29:10 -0600 Message-ID: <20260404213020.372253-35-sjg@u-boot.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20260404213020.372253-1-sjg@u-boot.org> References: <20260404213020.372253-1-sjg@u-boot.org> MIME-Version: 1.0 Message-ID-Hash: LKI4S54GLE2IH2ZESM2OY4BTKARVMJXX X-Message-ID-Hash: LKI4S54GLE2IH2ZESM2OY4BTKARVMJXX X-MailFrom: sjg@u-boot.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Simon Glass X-Mailman-Version: 3.3.10 Precedence: list Subject: [Concept] [PATCH 34/37] patman: Add tests for review and Gmail features List-Id: Discussion and patches related to U-Boot Concept Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: From: Simon Glass Add 26 tests covering the review feature: - Integration tests: new series, already-reviewed, new version, title search, no-link-or-title error, apply failure, draft creation (dry run and actual) - Review parsing: approved, skip verdict, changes with comments - Greeting: guess from email, fallback when empty - Refinement: skips approved reviews, processes reviews with comments - Cleanup: backtick removal, function quoting - Commit message: deduplication, subject + body quoting - Gmail: subject preserves [PATCH], falls back to name, From header set/omitted - Database: review_delete_for_version, series_set_source - Notes: save and retrieve, empty version filtering Signed-off-by: Simon Glass --- tools/patman/review.py | 11 + tools/patman/test_cseries.py | 690 +++++++++++++++++++++++++++++++++++ 2 files changed, 701 insertions(+) -- 2.43.0 diff --git a/tools/patman/review.py b/tools/patman/review.py index 74bc81a2a30..f0fa4e865e4 100644 --- a/tools/patman/review.py +++ b/tools/patman/review.py @@ -1479,6 +1479,17 @@ def _register_series(cser, clean_name, version, link, series_data): if pcommits: cser.db.pcommit_add_list(svid, pcommits) + # pcommit_add_list only stores seq/subject/change_id; update + # patch_id from the patchwork data + pclist = cser.db.pcommit_get_list(svid) + for pcm, patch in zip(pclist, patches): + patch_id = patch.get('id') + if patch_id: + cser.db.pcommit_update(database.Pcommit( + idnum=pcm.idnum, seq=pcm.seq, subject=pcm.subject, + svid=svid, change_id=pcm.change_id, state=pcm.state, + patch_id=patch_id, num_comments=pcm.num_comments)) + cser.commit() tout.notice(f"Added series '{clean_name}' v{version} to database") return series_id, svid diff --git a/tools/patman/test_cseries.py b/tools/patman/test_cseries.py index 5974c69253a..7d6a6179c1e 100644 --- a/tools/patman/test_cseries.py +++ b/tools/patman/test_cseries.py @@ -5,6 +5,7 @@ """Functional tests for checking that patman behaves correctly""" import asyncio +import contextlib from datetime import datetime import os import re @@ -4480,3 +4481,692 @@ Date: .* self.assertIn('Version 2:', output) self.assertIn('Second version desc', output) self.assertIn('Notes: Fixed review feedback', output) + + # Series link used by the review tests + REVIEW_LINK = 497923 + REVIEW_LINK_V2 = 497924 + REVIEW_NAME = 'boot/bootm: Disable interrupts after loading the image' + + def _fake_patchwork_review(self, subpath): + """Fake Patchwork server for review tests + + Args: + subpath (str): URL subpath to use + """ + if subpath == 'projects/': + return [ + {'id': self.PROJ_ID, 'name': 'U-Boot', + 'link_name': self.PROJ_LINK_NAME}, + ] + + re_search = re.match(r'series/\?project=(\d+)&q=(.*)$', subpath) + if re_search: + return [ + {'id': self.REVIEW_LINK, 'name': self.REVIEW_NAME, + 'version': 1, 'date': '2026-03-29T15:17:33'}, + {'id': self.REVIEW_LINK_V2, 'name': self.REVIEW_NAME, + 'version': 2, 'date': '2026-04-01T10:00:00'}, + ] + + m_series = re.match(r'series/(\d+)/$', subpath) + if m_series: + series_id = int(m_series.group(1)) + if series_id == self.REVIEW_LINK: + return { + 'name': f'[PATCH] {self.REVIEW_NAME}', + 'version': 1, + 'received_total': 1, + 'mbox': f'https://my-fake-url/series/{series_id}/mbox/', + 'submitter': {'name': 'Test Author', + 'email': 'author@example.com'}, + 'project': {'list_email': 'u-boot@lists.denx.de'}, + 'cover_letter': None, + 'patches': [ + {'id': 900, + 'name': f'[PATCH] {self.REVIEW_NAME}', + 'msgid': '<20260329-bootm-v1-1-abc@posteo.net>'}, + ], + } + if series_id == self.REVIEW_LINK_V2: + return { + 'name': f'[PATCH v2] {self.REVIEW_NAME}', + 'version': 2, + 'received_total': 1, + 'mbox': f'https://my-fake-url/series/{series_id}/mbox/', + 'submitter': {'name': 'Test Author', + 'email': 'author@example.com'}, + 'project': {'list_email': 'u-boot@lists.denx.de'}, + 'cover_letter': None, + 'patches': [ + {'id': 901, + 'name': f'[PATCH,v2] {self.REVIEW_NAME}', + 'msgid': '<20260401-bootm-v2-1-def@posteo.net>'}, + ], + } + raise ValueError( + f'Fake Patchwork unknown series_id: {series_id}') + + m_patch = re.match(r'patches/(\d+)/$', subpath) + if m_patch: + return { + 'headers': { + 'Reply-To': 'author@posteo.net', + 'To': 'u-boot@lists.denx.de', + 'Cc': 'Tom Rini ', + }, + } + + m_pcomm = re.match(r'patches/(\d+)/comments/$', subpath) + if m_pcomm: + return [] + + m_ccomm = re.match(r'covers/(\d+)/comments/$', subpath) + if m_ccomm: + return [] + + raise ValueError(f'Fake Patchwork unhandled URL: {subpath}') + + REVIEWER = 'Test Reviewer ' + + def run_review(self, *argv, **kwargs): + """Run a review command with the test reviewer identity""" + return self.run_args('review', '--reviewer', self.REVIEWER, + *argv, **kwargs) + + def _mock_review(self): + """Context manager to mock apply, upstream, git and AI review""" + fake_review = {1: f'Reviewed-by: {self.REVIEWER}'} + return (mock.patch('patman.review._apply_and_check', + return_value='review1'), + mock.patch('patman.review._get_upstream_branch', + return_value='origin/master'), + mock.patch('patman.review.review_patches_sync', + return_value=fake_review), + mock.patch('patman.review.gitutil.get_top_level', + return_value=self.tmpdir), + mock.patch('patman.review.command.output', + return_value='pati'), + mock.patch('patman.review._git_restore')) + + def test_review_new_series(self): + """Test reviewing a new series creates database records""" + cser = self.get_cser() + pwork = Patchwork.for_testing(self._fake_patchwork_review) + pwork.project_set(self.PROJ_ID, self.PROJ_LINK_NAME) + + mocks = self._mock_review() + with contextlib.ExitStack() as stack: + for m in mocks: + stack.enter_context(m) + with terminal.capture() as _: + self.run_review( '-l', str(self.REVIEW_LINK), + pwork=pwork) + + # Check the series was created with source='review' + self.db_open() + result = cser.db.series_find_by_link(str(self.REVIEW_LINK)) + self.assertIsNotNone(result) + series_id, name, version, svid = result + self.assertEqual(self.REVIEW_NAME, name) + self.assertEqual(1, version) + + # Check source is 'review' + res = cser.db.execute( + 'SELECT source FROM series WHERE id = ?', (series_id,)) + self.assertEqual('review', res.fetchone()[0]) + + # Check pcommit was created + pclist = cser.db.pcommit_get_list(svid) + self.assertEqual(1, len(pclist)) + self.assertEqual(900, pclist[0].patch_id) + + def test_review_already_reviewed(self): + """Test that reviewing the same link again is detected""" + cser = self.get_cser() + pwork = Patchwork.for_testing(self._fake_patchwork_review) + pwork.project_set(self.PROJ_ID, self.PROJ_LINK_NAME) + + mocks = self._mock_review() + with contextlib.ExitStack() as stack: + for m in mocks: + stack.enter_context(m) + with terminal.capture() as _: + self.run_review( '-l', str(self.REVIEW_LINK), + pwork=pwork) + + # Review the same link again + mocks = self._mock_review() + with contextlib.ExitStack() as stack: + for m in mocks: + stack.enter_context(m) + with terminal.capture() as (out, err): + self.run_review('-l', str(self.REVIEW_LINK), pwork=pwork) + self.assertIn('Already reviewed', out.getvalue()) + + def test_review_new_version(self): + """Test that reviewing v2 detects v1 as previously reviewed""" + cser = self.get_cser() + pwork = Patchwork.for_testing(self._fake_patchwork_review) + pwork.project_set(self.PROJ_ID, self.PROJ_LINK_NAME) + + # Review v1 first + mocks = self._mock_review() + with contextlib.ExitStack() as stack: + for m in mocks: + stack.enter_context(m) + with terminal.capture() as _: + self.run_review( '-l', str(self.REVIEW_LINK), + pwork=pwork) + + # Now review v2 - should detect the previous review + mocks = self._mock_review() + with contextlib.ExitStack() as stack: + for m in mocks: + stack.enter_context(m) + with terminal.capture() as (out, _): + self.run_review( '-l', str(self.REVIEW_LINK_V2), + pwork=pwork) + self.assertIn('Previously reviewed', out.getvalue()) + + # Check both versions are under the same series + self.db_open() + v1 = cser.db.series_find_by_link(str(self.REVIEW_LINK)) + v2 = cser.db.series_find_by_link(str(self.REVIEW_LINK_V2)) + self.assertEqual(v1[0], v2[0]) # same series_id + self.assertEqual(1, v1[2]) # version 1 + self.assertEqual(2, v2[2]) # version 2 + + def test_review_title_search(self): + """Test searching for a series by title""" + cser = self.get_cser() + pwork = Patchwork.for_testing(self._fake_patchwork_review) + pwork.project_set(self.PROJ_ID, self.PROJ_LINK_NAME) + + mocks = self._mock_review() + with contextlib.ExitStack() as stack: + for m in mocks: + stack.enter_context(m) + with terminal.capture() as (out, _): + self.run_review( '-t', 'Disable interrupts', + pwork=pwork) + # Should pick the most recent (v2) + self.assertIn('Using most recent', out.getvalue()) + + self.db_open() + result = cser.db.series_find_by_link(str(self.REVIEW_LINK_V2)) + self.assertIsNotNone(result) + + def test_review_no_link_or_title(self): + """Test that missing -l and -t gives a proper error""" + self.get_cser() + pwork = Patchwork.for_testing(self._fake_patchwork_review) + pwork.project_set(self.PROJ_ID, self.PROJ_LINK_NAME) + + with terminal.capture() as _: + self.run_review( pwork=pwork, expect_ret=1) + + def test_review_apply_failure(self): + """Test that apply failure is reported""" + self.get_cser() + pwork = Patchwork.for_testing(self._fake_patchwork_review) + pwork.project_set(self.PROJ_ID, self.PROJ_LINK_NAME) + + with mock.patch('patman.review._apply_and_check', + return_value=None), \ + mock.patch('patman.review._get_upstream_branch', + return_value='origin/master'), \ + mock.patch('patman.review.gitutil.get_top_level', + return_value=self.tmpdir), \ + mock.patch('patman.review.command.output', + return_value='test'), \ + mock.patch('patman.review._git_restore'): + with terminal.capture() as _: + self.run_review('-l', str(self.REVIEW_LINK), + pwork=pwork, expect_ret=1) + + def test_review_create_drafts_dry_run(self): + """Test dry-run draft creation shows what would be created""" + self.get_cser() + pwork = Patchwork.for_testing(self._fake_patchwork_review) + pwork.project_set(self.PROJ_ID, self.PROJ_LINK_NAME) + + mocks = self._mock_review() + with contextlib.ExitStack() as stack: + for m in mocks: + stack.enter_context(m) + with terminal.capture() as (out, _): + self.run_review( '-l', str(self.REVIEW_LINK), + '--create-drafts', '-n', pwork=pwork) + output = out.getvalue() + self.assertIn('Would create draft', output) + self.assertIn('author@posteo.net', output) + self.assertIn('trini@konsulko.com', output) + + def test_review_create_drafts(self): + """Test actual draft creation calls Gmail API""" + self.get_cser() + pwork = Patchwork.for_testing(self._fake_patchwork_review) + pwork.project_set(self.PROJ_ID, self.PROJ_LINK_NAME) + + mocks = self._mock_review() + with contextlib.ExitStack() as stack: + for m in mocks: + stack.enter_context(m) + with mock.patch('patman.gmail.check_available', + return_value=True): + with mock.patch('patman.gmail.get_service') as mock_svc: + mock_svc.return_value.users.return_value \ + .drafts.return_value \ + .create.return_value \ + .execute.return_value = {'id': 'draft123'} + mock_svc.return_value.users.return_value \ + .messages.return_value \ + .list.return_value \ + .execute.return_value = {'messages': []} + with terminal.capture() as (out, _): + self.run_review( '-l', + str(self.REVIEW_LINK), + '--create-drafts', pwork=pwork) + output = out.getvalue() + self.assertIn('Created 1 Gmail draft', output) + + def _make_review_ctx(self, reviewer_name='Test', reviewer_email='test@test.com', + author_name='', author_email='', date='', signoff=None, + diffstat=None): + """Build a ReviewContext for testing format_review_email()""" + from patman.review import ReviewContext + + ctx = ReviewContext(None, None, + {'submitter': {'name': author_name, 'email': author_email}, + 'date': date}) + ctx.reviewer_name = reviewer_name + ctx.reviewer_email = reviewer_email + ctx.signoff = signoff + ctx.diffstat = diffstat + return ctx + + def test_review_parse_approved(self): + """Test parsing an approved review""" + from patman.review import parse_review_output, format_review_email + + text = """GREETING: Marek +VERDICT: approved""" + greeting, verdict, comments = parse_review_output(text) + self.assertEqual('Marek', greeting) + self.assertEqual('approved', verdict) + self.assertEqual([], comments) + + ctx = self._make_review_ctx(author_name='Marek Vasut', + author_email='marex@denx.de', date='2026-03-21', + diffstat=' drivers/pci.c | 2 +-\n 1 file changed') + email = format_review_email(ctx, greeting, verdict, comments, + commit_message='pci: Fix the return type\n\nThe return is wrong.') + self.assertNotIn('Hi Marek,', email) + self.assertIn('On 2026-03-21, Marek Vasut', email) + self.assertIn('> pci: Fix the return type', email) + self.assertIn('> The return is wrong.', email) + self.assertIn('> drivers/pci.c', email) + self.assertIn('Reviewed-by: Test ', email) + + def test_review_parse_skip(self): + """Test parsing a skipped review (e.g. cover letter with no issues)""" + from patman.review import parse_review_output + + text = """GREETING: Michal +VERDICT: skip""" + greeting, verdict, comments = parse_review_output(text) + self.assertEqual('Michal', greeting) + self.assertEqual('skip', verdict) + self.assertEqual([], comments) + + def test_review_guess_name(self): + """Test guessing first name from email address""" + from patman.review import guess_name_from_email + + self.assertEqual('Simon', guess_name_from_email('simon.glass@xxx')) + self.assertEqual('Michal', guess_name_from_email('michal@amd.com')) + self.assertEqual('Marek', + guess_name_from_email('marek-vasut@denx.de')) + self.assertEqual('', guess_name_from_email('j@posteo.net')) + self.assertEqual('', guess_name_from_email('')) + self.assertEqual('', guess_name_from_email('12345@test.com')) + + def test_review_cleanup(self): + """Test mechanical cleanup of review text""" + from patman.review import cleanup_review_text + + # Backticks removed + self.assertEqual('Use foo here', + cleanup_review_text('Use `foo` here')) + + # Quoted function references unquoted + self.assertEqual('Call malloc() first', + cleanup_review_text("Call 'malloc()' first")) + self.assertEqual('Call free() after', + cleanup_review_text('Call "free()" after')) + + # Normal quotes preserved + self.assertEqual("Normal 'text' stays", + cleanup_review_text("Normal 'text' stays")) + + # Quoted diff lines not mangled + line = "> +\t`something`" + self.assertEqual('> +\tsomething', cleanup_review_text(line)) + + def test_review_greeting_fallback(self): + """Test greeting falls back to email when name is empty""" + from patman.review import format_review_email + + # Empty greeting should be guessed from email + ctx = self._make_review_ctx(author_name='Simon Glass', + author_email='simon.glass@xxx.com', date='2026-04-01', + signoff='Regards,\nTest') + email = format_review_email(ctx, '', 'changes_needed', + [('> +\tsome code', 'Fix this')]) + self.assertIn('Hi Simon,', email) + + # Unguessable email falls back to bare 'Hi,' + ctx = self._make_review_ctx(author_email='x@test.com', + date='2026-04-01') + email = format_review_email(ctx, '', 'changes_needed', + [('> +\tsome code', 'Fix this')]) + self.assertIn('Hi,', email) + self.assertNotIn('Hi ,', email) + + def test_review_refine_skips_approved(self): + """Test that refinement skips approved reviews without comments""" + import asyncio + from unittest.mock import patch, AsyncMock + + from patman.review import refine_reviews + + # An approved review with only structural lines + approved = ('On 2026-04-01, A wrote:\n' + '> Some commit message\n' + '>\n' + '> drivers/foo.c | 2 +-\n' + '\n' + 'Reviewed-by: Test \n') + + # Should be returned unchanged without calling the agent + loop = asyncio.new_event_loop() + with patch('patman.review.get_voice', return_value=None): + result = loop.run_until_complete( + refine_reviews({1: approved})) + loop.close() + self.assertEqual({1: approved}, result) + + def test_review_refine_processes_comments(self): + """Test that refinement processes reviews with comments""" + import asyncio + import sys + import types + from unittest.mock import patch, MagicMock + + review_with_comments = ( + 'Hi Marek,\n\n' + 'On 2026-04-01, Marek wrote:\n' + '> +\tsome code\n\n' + 'This needs fixing.\n\n' + 'Regards,\nSimon\n') + + # Mock the agent to return a slightly trimmed version + refined = '---SEQ 1---\n' + review_with_comments.replace( + 'This needs fixing.', 'Fix this.') + + async def mock_agent(prompt, options): + return True, refined + + from patman import review as review_mod + + loop = asyncio.new_event_loop() + with patch.object(review_mod, 'get_voice', return_value=None), \ + patch.object(review_mod.claude_mod, 'run_agent_collect', + side_effect=mock_agent), \ + patch.object(review_mod, 'ClaudeAgentOptions', MagicMock()), \ + terminal.capture(): + result = loop.run_until_complete( + review_mod.refine_reviews({1: review_with_comments})) + loop.close() + self.assertIn('Fix this.', result[1]) + + def test_review_parse_changes(self): + """Test parsing a review with comments""" + from patman.review import parse_review_output, format_review_email + + text = """GREETING: J. +COMMENT: +> + if (ret < 0) +> + return ret; + +This should use goto err instead. + +COMMENT: +> + bootm_disable_interrupts(); + +This call should be conditional. + +VERDICT: changes_needed""" + + greeting, verdict, comments = parse_review_output(text) + self.assertEqual('J.', greeting) + self.assertEqual('changes_needed', verdict) + self.assertEqual(2, len(comments)) + self.assertIn('goto err', comments[0][1]) + self.assertIn('conditional', comments[1][1]) + + ctx = self._make_review_ctx(author_name='J. Neuschäfer', + author_email='j.ne@posteo.net', date='2026-03-29', + signoff='Regards,\nSimon') + email = format_review_email(ctx, greeting, verdict, comments) + self.assertIn('Hi J.,', email) + self.assertIn('> +\tif (ret < 0)', email) + self.assertIn('goto err', email) + self.assertNotIn('Reviewed-by', email) + self.assertIn('Regards,\nSimon', email) + + def test_review_commit_msg_no_duplicate(self): + """Test that the commit-msg builder avoids duplicating the subject""" + # Simulate cmt.subject and cmt.msg where msg starts with subject + subject = 'Drop unused macros' + msg_with_dup = 'Drop unused macros\n\nThese macros are never used.' + msg_without_dup = '\nThese macros are never used.' + + # When body starts with subject, use body as-is + body = msg_with_dup.strip() + if body.startswith(subject): + commit_msg = body + else: + commit_msg = (subject + '\n' + body).strip() + self.assertEqual(1, commit_msg.count('Drop unused macros')) + + # When body doesn't start with subject, prepend it + body = msg_without_dup.strip() + if body.startswith(subject): + commit_msg = body + else: + commit_msg = (subject + '\n' + body).strip() + self.assertTrue(commit_msg.startswith('Drop unused macros')) + self.assertIn('These macros are never used.', commit_msg) + + def test_review_commit_msg_with_body(self): + """Test that subject + body are both quoted when body differs""" + from patman.review import format_review_email + + ctx = self._make_review_ctx(author_name='Author', + author_email='a@b.com', date='2026-04-01', + diffstat=' file.c | 1 +\n 1 file changed') + email = format_review_email(ctx, '', 'approved', [], + commit_message='Fix the bug\n\nThe bug causes a crash.') + self.assertIn('> Fix the bug', email) + self.assertIn('> The bug causes a crash.', email) + + def test_gmail_subject_preserves_patch_prefix(self): + """Test that reply subjects use the original Subject header""" + from patman.gmail import create_review_drafts + + series_data = { + 'submitter': {'email': 'a@b.com'}, + 'project': {'list_email': 'list@test.com'}, + 'cover_letter': None, + 'patches': [ + {'id': 1, 'name': 'Fix the bug', + 'msgid': '<1@test.com>'}, + ], + } + patch_headers = { + 1: {'Subject': '[PATCH v2 1/3] Fix the bug', + 'Message-Id': '<1@test.com>'}, + } + review_bodies = {1: 'Reviewed-by: Test '} + + with terminal.capture(): + create_review_drafts(series_data, review_bodies, + patch_headers=patch_headers, dry_run=True) + + def test_gmail_subject_falls_back_to_name(self): + """Test that reply subjects fall back to patchwork name""" + from patman.gmail import create_review_drafts + + series_data = { + 'submitter': {'email': 'a@b.com'}, + 'project': {'list_email': 'list@test.com'}, + 'cover_letter': None, + 'patches': [ + {'id': 1, 'name': 'Fix the bug', + 'msgid': '<1@test.com>'}, + ], + } + # No Subject in headers — should fall back to patch name + patch_headers = {1: {'Message-Id': '<1@test.com>'}} + review_bodies = {1: 'Reviewed-by: Test '} + + with terminal.capture(): + create_review_drafts(series_data, review_bodies, + patch_headers=patch_headers, dry_run=True) + + def test_gmail_from_header(self): + """Test that create_draft sets From when sender is provided""" + from patman.gmail import create_draft + from email import message_from_bytes + from unittest.mock import MagicMock, patch + import base64 + + service = MagicMock() + service.users().drafts().create().execute.return_value = { + 'id': 'draft123'} + + draft = create_draft( + service, 'to@test.com', 'Re: test', 'body', + sender='Simon Glass ') + + # Extract the raw message that was passed to the API + call_kwargs = (service.users().drafts().create + .call_args) + raw = call_kwargs[1]['body']['message']['raw'] + msg = message_from_bytes(base64.urlsafe_b64decode(raw)) + self.assertEqual('Simon Glass ', msg['from']) + + def test_gmail_no_from_without_sender(self): + """Test that create_draft omits From when sender is None""" + from patman.gmail import create_draft + from email import message_from_bytes + from unittest.mock import MagicMock + import base64 + + service = MagicMock() + service.users().drafts().create().execute.return_value = { + 'id': 'draft123'} + + draft = create_draft( + service, 'to@test.com', 'Re: test', 'body') + + call_kwargs = (service.users().drafts().create + .call_args) + raw = call_kwargs[1]['body']['message']['raw'] + msg = message_from_bytes(base64.urlsafe_b64decode(raw)) + self.assertIsNone(msg['from']) + + def test_review_delete_for_version(self): + """Test deleting all reviews for a series version""" + cser = self.get_database() + + series_id = cser.db.series_add('test-delete', 'Test') + svid = cser.db.ser_ver_add(series_id, 1) + + from datetime import datetime + ts = datetime.now().isoformat() + cser.db.review_add(svid, 1, 'review body 1', True, ts) + cser.db.review_add(svid, 2, 'review body 2', False, ts) + cser.commit() + + reviews = cser.db.review_get_for_version(svid) + self.assertEqual(2, len(reviews)) + + cser.db.review_delete_for_version(svid) + cser.commit() + + reviews = cser.db.review_get_for_version(svid) + self.assertEqual(0, len(reviews)) + + def test_series_set_source(self): + """Test setting the source field on a series""" + cser = self.get_database() + + series_id = cser.db.series_add('test-source', 'Test') + cser.db.series_set_source(series_id, 'review') + cser.commit() + + res = cser.db.execute( + 'SELECT source FROM series WHERE id = ?', (series_id,)) + self.assertEqual('review', res.fetchone()[0]) + + def test_review_notes_save_and_show(self): + """Test saving and retrieving review notes""" + cser = self.get_database() + + # Create a series with two versions + series_id = cser.db.series_add('test-notes', 'Test series') + svid1 = cser.db.ser_ver_add(series_id, 1) + svid2 = cser.db.ser_ver_add(series_id, 2) + + # No notes initially + notes = cser.db.ser_ver_get_all_notes(series_id) + self.assertEqual([], notes) + + # Save notes for v1 + cser.db.ser_ver_set_notes(svid1, 'Fixed the memory leak issue') + cser.commit() + + notes = cser.db.ser_ver_get_all_notes(series_id) + self.assertEqual(1, len(notes)) + self.assertEqual(1, notes[0][0]) + self.assertIn('memory leak', notes[0][1]) + + # Save notes for v2 + cser.db.ser_ver_set_notes(svid2, 'Addressed style feedback') + cser.commit() + + notes = cser.db.ser_ver_get_all_notes(series_id) + self.assertEqual(2, len(notes)) + self.assertEqual(1, notes[0][0]) + self.assertEqual(2, notes[1][0]) + + def test_review_notes_skips_empty(self): + """Test that versions without notes are excluded""" + cser = self.get_database() + + series_id = cser.db.series_add('test-empty', 'Test') + svid1 = cser.db.ser_ver_add(series_id, 1) + svid2 = cser.db.ser_ver_add(series_id, 2) + svid3 = cser.db.ser_ver_add(series_id, 3) + + # Only set notes for v1 and v3 + cser.db.ser_ver_set_notes(svid1, 'v1 notes') + cser.db.ser_ver_set_notes(svid3, 'v3 notes') + cser.commit() + + notes = cser.db.ser_ver_get_all_notes(series_id) + self.assertEqual(2, len(notes)) + self.assertEqual(1, notes[0][0]) + self.assertEqual(3, notes[1][0])