trac-hacks
Add support for Trac 1.6 and Python3
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! ✨
@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 🕵🏻
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.)
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.
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.
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)
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).
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.
I think Aymeric gave me PyPI access, so if you're ok with it I could attempt a release after the merge.
That sounds good. Should we go ahead and get this merged and then publish a new release?