On 2020/05/07 16:17, Johan Corveleyn wrote: > On Wed, May 6, 2020 at 10:36 AM Yasuhito FUTATSUKI <futat...@poem.co.jp> > wrote: >> >> On 2020/05/04 4:49, Johan Corveleyn wrote: >>> On Sun, May 3, 2020 at 6:23 PM Yasuhito FUTATSUKI <futat...@poem.co.jp> >>> wrote: >>>> >>>> On 2020/05/03 10:02, Johan Corveleyn wrote: >>>>> On Fri, May 1, 2020 at 7:46 PM Yasuhito FUTATSUKI <futat...@poem.co.jp> >>>>> wrote: >>>>>> >>>>>> On 2020/04/23 2:05, Yasuhito FUTATSUKI wrote: >> >>>>>> It seems following tests are EOL style sensitive, but they didn't >>>>>> check without strict EOL style on Windows: >>>>>> >>>>>> merge_tests.merge_conflict_markers_matching_eol >>>>>> merge_tests.merge_eolstyle_handling >>>>>> patch_tests.patch_no_svn_eol_style >>>>>> patch_tests.patch_with_svn_eol_style >>>>>> patch_tests.patch_with_svn_eol_style_uncommitted >>>>>> update_tests.conflict_markers_matching_eol >>>>>> update_tests.update_eol_style_handling >>>>>> >>>>>> I've not check that each tests depend that native EOL is '\n' or not. >>>>>> However, I think if some tests depend on it, other test cases for >>>>>> those aims are needed on Windows, or at least explicitly skip the tests >>>>>> on Windows to clarify that those tests check nothing for the aims. >>>>>> >>>>>> The updated patch attached only make these tests EOL style sensitive >>>>>> (and relax check of EOL on Python 2 if keep_eol_style optional >>>>>> parameter is not specified). >>>>> >>>>> Thank you for continuing to work on this, Yasuhito. >>>>> >>>>> This patch indeed fixes the above 7 tests when running with Python 3 on >>>>> Windows. >>>>> However, when running with Python 2 there are now 6 tests that fail >>>>> (without this patch, all tests are successful with Py2): >>>> >>>> Thank you for testing. >>>> >>>> As far as this test result, those test cases don't depend on specific >>>> native EOL. >>>> >>>>> FAIL: merge_tests.py 34: conflict markers should match the file's eol >>>>> style >>>>> FAIL: patch_tests.py 13: patch target with no svn:eol-style >>>>> FAIL: patch_tests.py 14: patch target with svn:eol-style >>>>> FAIL: patch_tests.py 15: patch target with uncommitted svn:eol-style >>>>> FAIL: patch_tests.py 57: patch a binary file >>>>> FAIL: update_tests.py 26: conflict markers should match the file's eol >>>>> style >>>>> >>>>> See fails.log in attachment. >>>> >>>> I overlooked that result of io.TextIO.read() is unicode on Python 2. >>>> I hope updated patch may resolve this issue. >>> >>> Okay, that latest version fixes this test for Python 2: >>> >>> patch_tests.py 57: patch a binary file >>> >>> But the five other failures still remain for Py2: >>> >>> FAIL: merge_tests.py 34: conflict markers should match the file's eol style >>> FAIL: patch_tests.py 13: patch target with no svn:eol-style >>> FAIL: patch_tests.py 14: patch target with svn:eol-style >>> FAIL: patch_tests.py 15: patch target with uncommitted svn:eol-style >>> FAIL: update_tests.py 26: conflict markers should match the file's eol >>> style >>> >>> See fails_py2.log in attachment. >> >> I read those test code again, and I found that they don't distinct >> text I/O and binary I/O when they write to files in some case, So I >> updated the patch address them. (sain_keep_eol_style_win_patch_20200506.txt) > > Yes, it does :-). Perfect! With this version of the patch, all tests > pass again with Python 2.
Thanks. I think they do what we want them to do now. >>> For Python 3 we're getting close. After this patch and the one for >>> svnrdump_tests, we only have 2 failures left with Python 3: >> >> Please don't forget our goal is not to make the tests be passed but >> to make the tests check what we want to check, on Windows both with >> py2 and py3. > > Agreed. And thank you for being so thorough. It's great that you took > the time to analyse the tests so deeply. > >> I've fixed the test codes to make them what the author intended I think. >> If the results of py2 and py3 are still different, the test code are >> still broken. With a broken test, we can't warrant ether the test target >> code is correct or the test case is correct, even if the tests can be >> passed. So we should also check the test cases (scenarios and expected >> results) themselves are correct on Windows(, and I didn't it... at >> least, yet). Now I've confirmed those cases themselves. It seems they don't depend on what is EOL character(s), except value of variable "native_nl". >>> FAIL: svnadmin_tests.py 35: detect denormalized names and name collisions >> >> This is caused by output message of svnadmin, containing non UTF-8 >> character. Attached patch fix_svnadmin_tests_patch.txt address it, >> however I don't have confidence because I don't know what charset/encoding >> svnadmin on Windows use. With Python 2, output message is treated >> as bytes as is, this is not affected. > > Well, I can confirm that the test passes on my Windows 10 system with > this patch. Both with Python 3 and Python 2. I can't comment on the > general validity (this is a bit above my head), but it does work :-). If svnadmin uses charset/encoding other than 'utf-8' for output messages, this patch is incorrect, because expected strings or regular expressions are always Uniode, which is decoded by approprite codecs for the tool. So I still doubt this result is by chance. >>> FAIL: svndumpfilter_tests.py 7: svndumpfilter with an empty prefix >> >> It seems this is just same reason as svnrdump_tests.py was broken. >> (fix_svndumpfilter_tests_patch.txt) > > Yes, perfect! It works. Thanks, I think this can safely be commited. > Awesome work, Yasuhito! With these 3 patches applied to my trunk > working copy, the entire testsuite now passes for me on Windows both > with Python 2 and Python 3. > > So, as far as I'm concerned: please go ahead and commit those patches! > (one tiny nit: in your log messages, please indent the texts following > the symbols with 2 spaces, so it aligns with the bullets nicely, as in > the examples on > http://subversion.apache.org/docs/community-guide/conventions.html#log-messages) > > We should probably also propose all of your latest fixes for backport > in /branches/1.14.x/STATUS, so they can be included in 1.14.1 (and > interested people can already see that the "known testsuite issues on > Windows" are being addressed). Thank you for your advice. However I worry about Python 2 regression mentioned in another thread. If it is true, those patch can be also affected. So I'll wait for a detailed report. > There is still the issue of PYTHONLEGACYWINDOWSSTDIO that is very much > blocking for anyone running the testsuite on Windows with Python 3. > Worst case, we might have to simply document it, and perhaps emit a > warning if it's not set? Or can we simply set that envvar ourselves, > automatically, from within some central place in the testsuite (as > soon as redirection to a logfile has been requested or something)? > Unless you still have some more magic up your sleeve to handle this, > Yasuhito :-). ... Ah, I know it is already resolved without using os.dup/dup2 by one of subscriber of this list but not me, and waiting for contribution :) Cheers, -- Yaushito FUTATSUKI <futat...@poem.co.jp>/<futat...@yf.bsdclub.org>
