Page MenuHomePhabricator

tests: look for CRLF on Windows
Needs RevisionPublic

Authored by indygreg on Mar 29 2020, 9:30 PM.

Details

Reviewers
durin42
marmoute
baymax
Group Reviewers
hg-reviewers
Summary

The ui.editor code calls util.tonativeeol() to normalize line endings
to the platform native format. So, editor files may have CRLF
in them. For some reason, just this one specific test case was
failing on Python 3 on Windows. (The output before this change
was ...\r (esc), but only on the first line of output. I suspect
the test harness output parser was getting confused by the specific
byte sequences in this output or something.

I wanted to hack around this by filtering output via sed to remove
the CR. But check-code says this practice is apparently banned.
So I just made the test conditional.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

indygreg created this revision.Mar 29 2020, 9:30 PM

The test harness *should* match existing \n output as a fallback, which got me to wondering if it was the (esc) at the end screwing it up. I tried this patch:

diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -2065,6 +2065,8 @@ class TTest(Test):
             if PYTHON3:
                 el = el[:-7].decode('unicode_escape') + '\n'
                 el = el.encode('utf-8')
+                log('el: %s' % el)
+                log(' l: %s' % l)
             else:
                 el = el[:-7].decode('string-escape') + '\n'
         if el == l or os.name == 'nt' and el[:-1] + b'\r\n' == l:

... and got this output:

$ time py -3 run-tests.py --local test-histedit-arguments.t#abortflag --view bcompare
running 1 tests using 1 parallel processes
el: b'pick 3d3ea1f3a10b 5 1234567890123456789012345678901234567890123456789012345\xc3\xa3\xc2\x81\xc2\x82...\n'
 l: b'pick 3d3ea1f3a10b 5 1234567890123456789012345678901234567890123456789012345\xe3\x81\x82...\r\n'

So I'm not sure where the bytes are being lost. If this doesn't suggest a larger problem, I've got no problem with this patch as written. But I wonder if there's some stdio that needs to be patched up, because I thought I saw that pattern in some failing py3 tests at one point.

@mharbison72 @indygreg, IIRC Matt is saying this is the wrong fix. What's the status of this?

@mharbison72 @indygreg, IIRC Matt is saying this is the wrong fix. What's the status of this?

I'm not sure that it's *wrong*, I'm just wondering if there's an encoding and/or stdio issue lurking here somewhere.

marmoute accepted this revision.Apr 17 2020, 4:00 PM
baymax requested changes to this revision.Jul 31 2020, 1:56 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jul 31 2020, 1:56 PM