From c1e2e5a2e0dea4f9bfd53311d08f82aa90ac019c Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 4 Jun 2024 12:15:29 +0200
Subject: [PATCH] [REF] forwardport: update re_matches to not use a regex

Using a regex as the pattern is quite frustrating due to all the
escaping necessary, which in this refactoring I found out I'd missed,
multiple times.

Convert the pattern to something bespoke but not too complicated, we
may want to add anchoring support and a bit more finesse and the
future but for now straightforward "holes" seem to work well. I've
added support for capturing and even named groups even if this as yet
unnecessary and unused.

Fixes #861

[^1]: https://docs.pytest.org/en/stable/reference.html#pytest.hookspec.pytest_assertrepr_compare
---
 forwardport/tests/test_batches.py   | 18 +++++++++---------
 forwardport/tests/test_conflicts.py | 28 ++++++++++++++--------------
 forwardport/tests/test_simple.py    | 20 ++++++++++----------
 forwardport/tests/test_updates.py   | 24 ++++++++++++++----------
 mergebot_test_utils/utils.py        | 26 +++++++++++++++++---------
 runbot_merge/tests/test_basic.py    | 10 +++++-----
 6 files changed, 69 insertions(+), 57 deletions(-)

diff --git a/forwardport/tests/test_batches.py b/forwardport/tests/test_batches.py
index ad1c6f8b..b0b7155a 100644
--- a/forwardport/tests/test_batches.py
+++ b/forwardport/tests/test_batches.py
@@ -1,6 +1,6 @@
 import re
 
-from utils import Commit, make_basic, to_pr, seen, re_matches
+from utils import Commit, make_basic, to_pr, seen, matches
 
 
 def test_single_updated(env, config, make_repo):
@@ -381,26 +381,26 @@ def test_add_to_forward_port_conflict(env, config, make_repo, users):
     assert pr2_c.comments == [
         seen(env, pr2_c, users),
         # should have conflicts
-        (users['user'], re_matches(r"""@{user} cherrypicking of pull request {previous.display_name} failed\.
+        (users['user'], matches("""@{user} cherrypicking of pull request {previous.display_name} failed.
 
 stdout:
 ```
 Auto-merging b
-CONFLICT \(add/add\): Merge conflict in b
+CONFLICT (add/add): Merge conflict in b
 
 ```
 
 stderr:
 ```
-.*
+$$
 ```
 
-Either perform the forward-port manually \(and push to this branch, proceeding as usual\) or close this PR \(maybe\?\)\.
+Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).
 
-In the former case, you may want to edit this PR message as well\.
+In the former case, you may want to edit this PR message as well.
 
-:warning: after resolving this conflict, you will need to merge it via @{project.github_prefix}\.
+:warning: after resolving this conflict, you will need to merge it via @{project.github_prefix}.
 
-More info at https://github\.com/odoo/odoo/wiki/Mergebot#forward-port
-""".format(project=project, previous=pr2_b_id, **users), re.DOTALL))
+More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
+""".format(project=project, previous=pr2_b_id, **users)))
     ]
diff --git a/forwardport/tests/test_conflicts.py b/forwardport/tests/test_conflicts.py
index 4cad2c8c..eed2bcc0 100644
--- a/forwardport/tests/test_conflicts.py
+++ b/forwardport/tests/test_conflicts.py
@@ -2,7 +2,7 @@ import re
 import time
 from operator import itemgetter
 
-from utils import make_basic, Commit, validate_all, re_matches, seen, REF_PATTERN, to_pr
+from utils import make_basic, Commit, validate_all, matches, seen, REF_PATTERN, to_pr
 
 
 def test_conflict(env, config, make_repo, users):
@@ -59,39 +59,39 @@ def test_conflict(env, config, make_repo, users):
     assert prod.read_tree(c) == {
         'f': 'c',
         'g': 'a',
-        'h': re_matches(r'''<<<\x3c<<< HEAD
+        'h': matches('''<<<\x3c<<< HEAD
 a
-|||||||| parent of [\da-f]{7,}.*
+||||||| parent of $$ (temp)
 =======
 xxx
->>>\x3e>>> [\da-f]{7,}.*
+>>>\x3e>>> $$ (temp)
 '''),
     }
     assert prc.comments == [
         seen(env, prc, users),
-        (users['user'], re_matches(
-fr'''@{users['user']} @{users['reviewer']} cherrypicking of pull request {pra_id.display_name} failed\.
+        (users['user'], matches(
+f'''@{users['user']} @{users['reviewer']} cherrypicking of pull request {pra_id.display_name} failed.
 
 stdout:
 ```
 Auto-merging h
-CONFLICT \(add/add\): Merge conflict in h
+CONFLICT (add/add): Merge conflict in h
 
 ```
 
 stderr:
 ```
-.*
+$$
 ```
 
-Either perform the forward-port manually \(and push to this branch, proceeding as usual\) or close this PR \(maybe\?\)\.
+Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).
 
-In the former case, you may want to edit this PR message as well\.
+In the former case, you may want to edit this PR message as well.
 
-:warning: after resolving this conflict, you will need to merge it via @{project.github_prefix}\.
+:warning: after resolving this conflict, you will need to merge it via @{project.github_prefix}.
 
-More info at https://github\.com/odoo/odoo/wiki/Mergebot#forward-port
-''', re.DOTALL))
+More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
+'''))
     ]
 
     prb = prod.get_pr(prb_id.number)
@@ -372,7 +372,7 @@ b
 
     assert pr2.comments == [
         seen(env, pr2, users),
-        (users['user'], re_matches(r'@%s @%s .*CONFLICT' % (users['user'], users['reviewer']), re.DOTALL)),
+        (users['user'], matches('@%s @%s $$CONFLICT' % (users['user'], users['reviewer']))),
         (users['reviewer'], 'hansen r+'),
         (users['user'], f"@{users['user']} @{users['reviewer']} unable to stage: "
                         "All commits must have author and committer email, "
diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py
index e7214871..901b93a7 100644
--- a/forwardport/tests/test_simple.py
+++ b/forwardport/tests/test_simple.py
@@ -6,7 +6,7 @@ from datetime import datetime, timedelta
 
 import pytest
 
-from utils import seen, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all, part_of, to_pr, re_matches
+from utils import seen, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all, part_of, to_pr, matches
 
 FMT = '%Y-%m-%d %H:%M:%S'
 FAKE_PREV_WEEK = (datetime.now() + timedelta(days=1)).strftime(FMT)
@@ -346,27 +346,27 @@ def test_empty(env, config, make_repo, users):
             pr1_id.display_name
         )
     )
-    conflict = (users['user'], re_matches(
-        fr"""@{users['user']} @{users['reviewer']} cherrypicking of pull request {pr1_id.display_name} failed\.
+    conflict = (users['user'], matches(
+        f"""@{users['user']} @{users['reviewer']} cherrypicking of pull request {pr1_id.display_name} failed.
 
 stdout:
 ```
-.*
+$$
 ```
 
 stderr:
 ```
-.*
+$$
 ```
 
-Either perform the forward-port manually \(and push to this branch, proceeding as usual\) or close this PR \(maybe\?\)\.
+Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).
 
-In the former case, you may want to edit this PR message as well\.
+In the former case, you may want to edit this PR message as well.
 
-:warning: after resolving this conflict, you will need to merge it via @{project.github_prefix}\.
+:warning: after resolving this conflict, you will need to merge it via @{project.github_prefix}.
 
-More info at https://github\.com/odoo/odoo/wiki/Mergebot#forward-port
-""", re.DOTALL))
+More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
+"""))
     assert pr1.comments == [
         (users['reviewer'], 'hansen r+'),
         seen(env, pr1, users),
diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py
index 3a6b4a11..0dedf9dc 100644
--- a/forwardport/tests/test_updates.py
+++ b/forwardport/tests/test_updates.py
@@ -6,7 +6,7 @@ import re
 
 import pytest
 
-from utils import seen, re_matches, Commit, make_basic, to_pr
+from utils import seen, matches, Commit, make_basic, to_pr
 
 
 @pytest.mark.parametrize("merge_parent", [False, True])
@@ -433,12 +433,12 @@ def test_subsequent_conflict(env, make_repo, config, users):
     assert repo.read_tree(repo.commit(pr3_id.head)) == {
         'f': 'c',
         'g': 'a',
-        'h': re_matches(r'''<<<\x3c<<< HEAD
+        'h': matches('''<<<\x3c<<< HEAD
 a
-|||||||| parent of [\da-f]{7,}.*
+||||||| parent of $$ (temp)
 =======
 conflict!
->>>\x3e>>> [\da-f]{7,}.*
+>>>\x3e>>> $$ (temp)
 '''),
         'x': '0',
     }
@@ -458,18 +458,22 @@ conflict!
     # 1. link to status page
     # 2. forward-port chain thing
     assert repo.get_pr(pr3_id.number).comments[2:] == [
-        (users['user'], re_matches(f'''\
+        (users['user'], matches(f'''\
 @{users['user']} @{users['reviewer']} WARNING: the update of {pr2_id.display_name} to {pr2_id.head} has caused a \
 conflict in this pull request, data may have been lost.
 
 stdout:
-```.*?
-CONFLICT \(add/add\): Merge conflict in h.*?
+```
+Auto-merging h
+CONFLICT (add/add): Merge conflict in h
 ```
 
 stderr:
 ```
-\\d{{2}}:\\d{{2}}:\\d{{2}}.\\d+ .* {pr2_id.head}
-error: could not apply [0-9a-f]+\\.\\.\\. newfiles
-''', re.DOTALL))
+$$:$$:$$.$$ {pr2_id.head}
+error: could not apply $$... newfiles
+hint: $$
+----------
+status:
+'''))
     ]
diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py
index cba96114..0d0f7dff 100644
--- a/mergebot_test_utils/utils.py
+++ b/mergebot_test_utils/utils.py
@@ -42,18 +42,26 @@ def _simple_init(repo):
     prx = repo.make_pr(title='title', body='body', target='master', head=c2)
     return prx
 
-class re_matches:
+class matches(str):
+    # necessary so str.__new__ does not freak out on `flags`
+    def __new__(cls, pattern, flags=0):
+        return super().__new__(cls, pattern)
+
     def __init__(self, pattern, flags=0):
-        self._r = re.compile(pattern, flags)
+        p, n = re.subn(
+            # `re.escape` will escape the `$`, so we need to handle that...
+            # maybe it should not be $?
+            r'\\\$(\w*?)\\\$',
+            lambda m: f'(?P<{m[1]}>.*?)' if m[1] else '(.*?)',
+            re.escape(self),
+        )
+        assert n, f"matches' pattern should have at least one placeholder, found none in\n{pattern}"
+        self._r = re.compile(p, flags | re.DOTALL)
 
     def __eq__(self, text):
-        return self._r.match(text)
-
-    def __str__(self):
-        return re.sub(r'\\(.)', r'\1', self._r.pattern)
-
-    def __repr__(self):
-        return repr(str(self))
+        if not isinstance(text, str):
+            return NotImplemented
+        return self._r.search(text)
 
 def seen(env, pr, users):
     url = to_pr(env, pr).url
diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py
index 9299738a..d43368e9 100644
--- a/runbot_merge/tests/test_basic.py
+++ b/runbot_merge/tests/test_basic.py
@@ -10,7 +10,7 @@ import requests
 from lxml import html
 
 import odoo
-from utils import _simple_init, seen, re_matches, get_partner, Commit, pr_page, to_pr, part_of, ensure_one
+from utils import _simple_init, seen, matches, get_partner, Commit, pr_page, to_pr, part_of, ensure_one
 
 
 @pytest.fixture
@@ -174,7 +174,7 @@ def test_trivial_flow(env, repo, page, users, config):
             ('', 'Reviewer'),
         ]),
         # staging succeeded
-        (re_matches(r'.*'), f'<p>staging {st.id} succeeded</p>', [
+        (matches('$$'), f'<p>staging {st.id} succeeded</p>', [
             # set merge date
             (False, pr_id.merge_date + 'Z'),
             # updated state
@@ -676,7 +676,7 @@ def test_ff_failure(env, repo, config, page):
     _new, prev = doc.cssselect('li.staging')
 
     assert 'bg-gray-lighter' in prev.classes, "ff failure is ~ cancelling"
-    assert prev.get('title') == re_matches('fast forward failed \(update is not a fast forward\)')
+    assert 'fast forward failed (update is not a fast forward)' in prev.get('title')
 
     assert env['runbot_merge.pull_requests'].search([
         ('repository.name', '=', repo.name),
@@ -1029,7 +1029,7 @@ def test_rebase_failure(env, repo, users, config):
     assert pr_a.comments == [
         (users['reviewer'], 'hansen r+'),
         seen(env, pr_a, users),
-        (users['user'], re_matches(r'^Unable to stage PR')),
+        (users['user'], matches('Unable to stage PR')),
     ]
     assert pr_b.comments == [
         (users['reviewer'], 'hansen r+'),
@@ -3968,7 +3968,7 @@ class TestInfrastructure:
         assert repo.get_ref('heads/master') == m1
 
 def node(name, *children):
-    assert type(name) in (str, re_matches)
+    assert type(name) in (str, matches)
     return name, frozenset(children)
 def log_to_node(log):
     log = list(log)