Re: Problems running testsuite on Windows with Python 3

On Wed, May 6, 2020 at 10:36 AM Yasuhito FUTATSUKI <futatuki_at_poem.co.jp> wrote:
>
> On 2020/05/04 4:49, Johan Corveleyn wrote:
> > On Sun, May 3, 2020 at 6:23 PM Yasuhito FUTATSUKI <futatuki_at_poem.co.jp> wrote:
> >>
> >> On 2020/05/03 10:02, Johan Corveleyn wrote:
> >>> On Fri, May 1, 2020 at 7:46 PM Yasuhito FUTATSUKI <futatuki_at_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.

> > 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).
>
> > 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 :-).

> > 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.

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).

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 :-).

-- 
Johan