Fix test_steps_source_svn on Python 3#2796
Fix test_steps_source_svn on Python 3#2796tardyp merged 2 commits intobuildbot:masterfrom rodrigc:svn1
Conversation
|
@rodrigc, thanks for your PR! By analyzing the history of the files in this pull request, we identified @djmitche, @in3xes and @srinupiits to be potential reviewers. |
Codecov Report
@@ Coverage Diff @@
## master #2796 +/- ##
==========================================
+ Coverage 87.31% 87.32% +0.01%
==========================================
Files 308 308
Lines 32651 32693 +42
==========================================
+ Hits 28508 28550 +42
Misses 4143 4143
Continue to review full report at Codecov.
|
tardyp
left a comment
tardyp
left a comment
There was a problem hiding this comment.
python2 and python3 definitively need to output the same thing.
| "http://foo.com/\x10\xe6%", "http://foo.com/%10%E6%25") | ||
| "http://foo.com/\x10\xe6%", | ||
| ("http://foo.com/%10%E6%25", # Python 2 | ||
| "http://foo.com/%10%C3%A6%25" # Python 3 |
There was a problem hiding this comment.
wait a minute.. I think this just means the svnUriCanonicalize does not work as expected with python3.
if for svn, url canonyfication of http://foo.com/\x10\xe6% is supposed to return http://foo.com/%10%E6%25, we cannot make it return http://foo.com/%10%C3%A6%25.
I think what can be assumed here is that the input of svnUriCanonicalize is supposed to be bytes (and not native string). Wouldn't it simplify the problem you had?
There was a problem hiding this comment.
Using bytes doesn't work, and results in ascii decode errors on Python 3.
There was a problem hiding this comment.
normal, it is not ascii. We really need to fix svnUriCanonicalize to work
|
@noc0lour I need your help with your superior debugging skills. Can you help with this one? and then type: I get a test failure. I tracked it down to somewhere inside urllib unquote(), where |
|
With your branch I don't get a test failure on python3? |
|
Re-run the test from master branch. @tardyp doesn't like the fix I did here. I need another pair of eyes and smart brain (yours!) to look at it from another angle. |
|
@rodrigc noc0lour@6db1796 |
|
@noc0lour can you submit a PR for the patch you came up with? I don't think there is a clean solution for this, so let's just move forward with this and see where it goes. |
|
I find @noc0lour solution pretty clean. We may add it in from buildbot.compat import urlquote |
Codecov Report
@@ Coverage Diff @@
## master #2796 +/- ##
==========================================
+ Coverage 87.33% 87.33% +<.01%
==========================================
Files 309 310 +1
Lines 32771 32784 +13
==========================================
+ Hits 28620 28632 +12
- Misses 4151 4152 +1
Continue to review full report at Codecov.
|
solution found by @noc0lour
|
I think your solution now is fine as well. i think I can close my PR now ;) |
|
@noc0lour thank you for investigating and leading to the solution! |
|
great cooperation guys thank you! |
|
No problem, I'm glad I can help out :) Since my virtualenvs on my machines are organized using virtualevnwrapper it takes literally no time to test on py2/py3. |
5 participants
I tried various combinations to get this test to work on Python 2 and Python 3,
but due to some corner cases where "somestring" is unicode on Python 3,
and bytes on Python 2, and the bytes in this testcase can't be decoded as ASCII,
I couldn't get this to work any other way.