Uh oh!
There was an error while loading. Please reload this page.
fix: go 1.22 test breakage - #3459
Conversation
amanv8060
commented
Feb 5, 2025
@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)... |
amanv8060
commented
Feb 5, 2025
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. |
gmlewis
commented
Feb 5, 2025
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? |
amanv8060
commented
Feb 6, 2025
I would prefer to do that in seperate pr so that review becomes easier. |
gmlewis
left a comment
There was a problem hiding this comment.
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!
gmlewis
commented
Feb 6, 2025
Thank you, @stevehipwell and @alexandear ! |
Use correct url escaping to fix the errors caused by mux changes.
Fixes: #3409