-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix: go 1.22 test breakage #3459
Conversation
@gmlewis could you please approve actions test to see if all works correctly? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3459 +/- ##
=======================================
Coverage 91.03% 91.03%
=======================================
Files 179 179
Lines 15535 15535
=======================================
Hits 14142 14142
Misses 1221 1221
Partials 172 172 ☔ View full report in Codecov by Sentry. |
Yes, however we don't want to remove problematic (but valid) branch names from the unit tests (as you have done in this PR)... |
I am not sure if I get what you're saying, I haven't really removed any branch names, instead in the mux urls(the one's to be called) by api, I have just Path escaped them, github also does this, for eg, This would go like this:
Also see, tests in net/http. IMO, one thing that could be done better here is,to create a util function of sort that would take the branch name and path escape them before adding it to mux route. |
Ah! I see now what you are saying, @amanv8060. I apologize for the confusion I caused. Yes, a utility function would help tremendously, I would think, and make it clearer what is happening. Would you mind doing that as part of this PR? |
I would prefer to do that in seperate pr so that review becomes easier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @amanv8060 !
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
@stevehipwell - might you have time for a code review? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you, @stevehipwell and @alexandear ! |
Use correct url escaping to fix the errors caused by mux changes.
Fixes: #3409