Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Trac 1.6 and Python3 #141

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

bmispelon
Copy link
Contributor

@bmispelon bmispelon commented Jan 29, 2024

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 bmispelon force-pushed the trac-1.6-py3 branch 3 times, most recently from 9eacaf7 to 2adbaef Compare February 3, 2024 11:44
@bmispelon bmispelon changed the title [WIP] Add support for Trac 1.6 and Python3 Add support for Trac 1.6 and Python3 Feb 3, 2024
@bmispelon
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor

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

@neverpanic
Copy link
Member

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
Copy link
Member

rjollos commented Feb 23, 2024

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

@bmispelon
Copy link
Contributor Author

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
Copy link
Member

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)
 
 

Copy link
Member

@neverpanic neverpanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

runtests.py Outdated Show resolved Hide resolved
runtests.py Outdated Show resolved Hide resolved
@bmispelon
Copy link
Contributor Author

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 bmispelon force-pushed the trac-1.6-py3 branch 5 times, most recently from b8ad631 to c039002 Compare March 14, 2024 08:44
Hybrid py2/py3 on a single codebase is achieved by
using the `six` library here and there (mostly for
imports) and a few `if six.PY2` conditionals.

CI will test all currently supported versions of Python
(that means 3.8 -> 3.12), plus 2.7 and 3.7 (supported
in Ubuntu 20.04)
@bmispelon
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants