Add support for Trac 1.6 and Python3

[bmispelon]

This PR adds support for Python3 (>= 3.7) and Trac 1.6 (while retaining support for Python 2.7 and Trac >= 1.2).

I've tried to split it into several commits for easier review and/or cherry picking 🍒
I started with 3 commits (marked with [runtests]) that refactor the test suite to make the compatibility easier (the big one being replacing urllib2 with requests). Then I tried to make one [py3-compat] commit per change, except for the final one which I found too hard to split.

Let me know if there's anything that doesn't make sense, or that you'd like changed.

Once this is merged I should be able to contribute a fix for #136 and after that I would like to assist you in getting a release on PyPI if that's possible 🤞🏻

Thanks! ✨

[bmispelon]

@neverpanic All the tests seem to pass on CI so I believe this PR is ready for review when you have the time for it 🕵🏻

[aaugustin]

I recommend that we give Baptiste commit access to this repository. Given the low bus factor and low activity, that would be a win for everyone. I don't remember how the trac-hacks organization is managed and don't know how to make it happen, though. @neverpanic @rjollos @raimue Is one of you able to help? (We're the only people with significant contributions to this plugin.)

[aaugustin]

(I know Baptiste because we contributed to Django together.)

[neverpanic]

Fine with me. I haven't forgotten about this, but it's been a few busy weeks at work, so I didn't have the energy to review this yet.

I don't have admin access to this repo, so I can't make the change. IIRC only @rjollos can.

[rjollos]

Added @aaugustin and @bmispelon to the trac-github team.

[bmispelon]

Fine with me. I haven't forgotten about this, but it's been a few busy weeks at work, so I didn't have the energy to review this yet.

I completely understand, not trying to rush you.

If it helps, we (meaning django/code.djangoproject.com) have been running the PR's branch in production for about two weeks and haven't seen any issues with it. I would like not to have to maintain that fork, so I'm interested in having a proper PyPI release but other than that there's no particular time pressure.

Do let me know if there are things I can do to make your reviewing easier @neverpanic , I'm here to help.

[neverpanic]

Is the Python 3.12 CI step actually using Python 3.12?

I needed an additional patch to make the tests pass:

diff --git a/runtests.py b/runtests.py
index 0b77f41..e306541 100755
--- a/runtests.py
+++ b/runtests.py
@@ -951,19 +951,19 @@ class GitHubPostCommitHookTests(TracGitHubTests):
     def testCommit(self):
         self.makeGitCommit(GIT, 'foo', 'foo content\n')
         output = self.openGitHubHook()
-        self.assertRegexpMatches(output, r"Running hook on \(default\)\n"
-                                         r"\* Updating clone\n"
-                                         r"\* Synchronizing with clone\n"
-                                         r"\* Adding commit [0-9a-f]{40}\n")
+        six.assertRegex(self, output, r"Running hook on \(default\)\n"
+                                      r"\* Updating clone\n"
+                                      r"\* Synchronizing with clone\n"
+                                      r"\* Adding commit [0-9a-f]{40}\n")
 
     def testMultipleCommits(self):
         self.makeGitCommit(GIT, 'bar', 'bar content\n')
         self.makeGitCommit(GIT, 'bar', 'more bar content\n')
         output = self.openGitHubHook(2)
-        self.assertRegexpMatches(output, r"Running hook on \(default\)\n"
-                                         r"\* Updating clone\n"
-                                         r"\* Synchronizing with clone\n"
-                                         r"\* Adding commits [0-9a-f]{40}, [0-9a-f]{40}\n")
+        six.assertRegex(self, output, r"Running hook on \(default\)\n"
+                                      r"\* Updating clone\n"
+                                      r"\* Synchronizing with clone\n"
+                                      r"\* Adding commits [0-9a-f]{40}, [0-9a-f]{40}\n")
 
     def testCommitOnBranch(self):
         self.makeGitBranch(ALTGIT, 'stable/1.0')
@@ -971,21 +971,21 @@ class GitHubPostCommitHookTests(TracGitHubTests):
         self.makeGitBranch(ALTGIT, 'unstable/1.0')
         self.makeGitCommit(ALTGIT, 'unstable', 'unstable branch\n', branch='unstable/1.0')
         output = self.openGitHubHook(2, 'alt')
-        self.assertRegexpMatches(output, r"Running hook on alt\n"
-                                         r"\* Updating clone\n"
-                                         r"\* Synchronizing with clone\n"
-                                         r"\* Adding commit [0-9a-f]{40}\n"
-                                         r"\* Skipping commit [0-9a-f]{40}\n")
+        six.assertRegex(self, output, r"Running hook on alt\n"
+                                      r"\* Updating clone\n"
+                                      r"\* Synchronizing with clone\n"
+                                      r"\* Adding commit [0-9a-f]{40}\n"
+                                      r"\* Skipping commit [0-9a-f]{40}\n")
 
     def testUnknownCommit(self):
         # Emulate self.openGitHubHook to use a non-existent commit id
         random_id = ''.join(random.choice('0123456789abcdef') for _ in range(40))
         payload = {'commits': [{'id': random_id, 'message': '', 'distinct': True}]}
         response = requests.post(u('github'), json=payload, headers=HEADERS)
-        self.assertRegexpMatches(response.text, r"Running hook on \(default\)\n"
-                                         r"\* Updating clone\n"
-                                         r"\* Synchronizing with clone\n"
-                                         r"\* Unknown commit [0-9a-f]{40}\n")
+        six.assertRegex(self, response.text, r"Running hook on \(default\)\n"
+                                             r"\* Updating clone\n"
+                                             r"\* Synchronizing with clone\n"
+                                             r"\* Unknown commit [0-9a-f]{40}\n")
 
     def testNotification(self):
         ticket = Ticket(self.env)
@@ -1141,25 +1141,25 @@ class GitHubPostCommitHookWithUpdateHookTests(TracGitHubTests):
         self.makeGitCommit(GIT, 'foo', 'foo content\n')
         payload = self.makeGitHubHookPayload()
         output = self.openGitHubHook(payload=payload)
-        self.assertRegexpMatches(output, r"Running hook on \(default\)\n"
-                                         r"\* Updating clone\n"
-                                         r"\* Synchronizing with clone\n"
-                                         r"\* Adding commit [0-9a-f]{40}\n"
-                                         r"\* Running trac-github-update hook\n")
+        six.assertRegex(self, output, r"Running hook on \(default\)\n"
+                                      r"\* Updating clone\n"
+                                      r"\* Synchronizing with clone\n"
+                                      r"\* Adding commit [0-9a-f]{40}\n"
+                                      r"\* Running trac-github-update hook\n")
         self.assertEqual(output.split('\n')[-1], json.dumps(payload))
 
     def testUpdateHookExecFailure(self):
         os.chmod(d(UPDATEHOOK), 0o644)
         self.makeGitCommit(GIT, 'bar', 'bar content\n')
         payload = self.makeGitHubHookPayload()
-        with self.assertRaisesRegexp(requests.HTTPError, r'^500 Server Error: Internal Server Error'):
+        with six.assertRaisesRegex(self, requests.HTTPError, r'^500 Server Error: Internal Server Error'):
             output = self.openGitHubHook(payload=payload)
 
     def testUpdateHookFailure(self):
         self.createFailingUpdateHook()
         self.makeGitCommit(GIT, 'baz', 'baz content\n')
         payload = self.makeGitHubHookPayload()
-        with self.assertRaisesRegexp(requests.HTTPError, r'^500 Server Error: Internal Server Error'):
+        with six.assertRaisesRegex(self, requests.HTTPError, r'^500 Server Error: Internal Server Error'):
             output = self.openGitHubHook(payload=payload)
 
 

[neverpanic]

Other than the Python 3.12 fix I posted the patch for, this looks good to me. The nitpicks I commented can be ignored or fixed later.

Let me know whether you want me to add the assertRegex patch as a commit, or whether you want to do it yourself. We can merge this afterwards.

I still need to run a test with Python 2.x locally, but I guess CI already tells us that it works as expected.

[bmispelon]

Thanks for the review, much appreciated!

Not sure what happened with the assertRegexp stuff, I will look into it. I hope to be able to get to it this week (maybe Saturday?), but in any case I'll post an update next week (March 21st).

[bmispelon]

As it turned out, I had misconfigured nox in the github action file and the tests were actually only running in Python 3.7 - 3.10.

I've added a session.run("python", "--version") to the noxfile.py and checked the logs to make sure the right python versions are running (https://github.com/trac-hacks/trac-github/actions/runs/8277955827/job/22649335453?pr=141#step:7:2197)

I've also fixed your nitpicks (they were my fault, I'd messed up some rebasing).

I think Aymeric gave me PyPI access, so if you're ok with it I could attempt a release after the merge.