Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect treatment of EOL causes test failures on Windows #6078

Closed
p6rt opened this issue Feb 15, 2017 · 7 comments
Closed

Incorrect treatment of EOL causes test failures on Windows #6078

p6rt opened this issue Feb 15, 2017 · 7 comments

Comments

@p6rt
Copy link

@p6rt p6rt commented Feb 15, 2017

Migrated from rt.perl.org#130788 (status was 'resolved')

Searchable as RT130788$

@p6rt
Copy link
Author

@p6rt p6rt commented Feb 15, 2017

From sinan@unur.com

$ perl t\harness5 t\spec\S17-procasync\basic.rakudo.moar --verbosity=5
...
not ok 33 - Tapping stdout supply after start of process does not lose data

# Failed test 'Tapping stdout supply after start of process does not lose data'
# at t\spec\S17-procasync\basic.rakudo.moar line 109
# expected​: "Hello World\n"
# got​: "Hello World\r\n"
ok 34 - Process that doesn't output anything will not emit
# FUDGED!
# Looks like you failed 1 test of 34
Dubious, test returned 1 (wstat 256, 0x100)

For text streams, the standard way of dealing with differing EOL
conversions is to convert "\n" to the platform specific EOL sequence
on output and convert the platform specific EOL sequence to "\n" on
input.

It looks like that is not being done in Proc​::Async or whatever
plumbing it depends on.

See also https://www.nu42.com/2015/12/perl6-newline-behavior-fixed.html
and Raku/roast#232 (comment)

-- Sinan

@p6rt
Copy link
Author

@p6rt p6rt commented Feb 15, 2017

From @zoffixznet

Looks like several other tests in S17-procasync/basic.t would be failing as well if it weren't for the explicit kludges[^1][^2] added to replace "\r\n" to "\n". And `grep -nFR '\r\n' | grep subs` shows[^3] 32 potential places with a similar workaround.

Pretty LTA for portable Perl 6 code to require any such thing.

[1] https://github.com/perl6/roast/blob/045e538f0f28dedb7261b7c013eec16a5cf2ee7e/S17-procasync/basic.t#L19-L20
[2] https://github.com/perl6/roast/blob/045e538f0f28dedb7261b7c013eec16a5cf2ee7e/S17-procasync/basic.t#L62-L63
[3] https://gist.github.com/zoffixznet/c343ed73b830d57e022eb99478764c78

@p6rt
Copy link
Author

@p6rt p6rt commented Feb 15, 2017

The RT System itself - Status changed from 'new' to 'open'

@p6rt
Copy link
Author

@p6rt p6rt commented Feb 15, 2017

From sinan@unur.com

On Wed, Feb 15, 2017 at 3​:40 PM, Zoffix Znet via RT
<perl6-bugs-followup@​perl.org> wrote​:

Looks like several other tests in S17-procasync/basic.t would be failing as well if it weren't for the explicit kludges[^1][^2] added to replace "\r\n" to "\n".

*Sigh* ... I was in a hurry, so I did not look.

This is disappointing.

-- Sinan

@p6rt
Copy link
Author

@p6rt p6rt commented Feb 15, 2017

@p6rt
Copy link
Author

@p6rt p6rt commented Feb 28, 2017

From @jnthn

On Wed, 15 Feb 2017 10​:11​:01 -0800, sinan@​unur.com wrote​:

$ perl t\harness5 t\spec\S17-procasync\basic.rakudo.moar
--verbosity=5
...
not ok 33 - Tapping stdout supply after start of process does not lose
data

# Failed test 'Tapping stdout supply after start of process does not
lose data'
# at t\spec\S17-procasync\basic.rakudo.moar line 109
# expected​: "Hello World\n"
# got​: "Hello World\r\n"
ok 34 - Process that doesn't output anything will not emit
# FUDGED!
# Looks like you failed 1 test of 34
Dubious, test returned 1 (wstat 256, 0x100)

For text streams, the standard way of dealing with differing EOL
conversions is to convert "\n" to the platform specific EOL sequence
on output and convert the platform specific EOL sequence to "\n" on
input.

It looks like that is not being done in Proc​::Async or whatever
plumbing it depends on.

Looks like it may have worked originally (back when the \r\n translation fixes were done late 2015), but then regressed more recently when a large set of changes were made to make decoding error handling more robust. I've fixed it now to do the translation again, and also added a :translate-nl option that can be used to control it (the default is to do the translation, but this will allow it to be disabled for anybody who wishes to do so).

I've also added a test that runs a program with Proc​::Async that spits a \r\n out whatever platform we're running on, so if we break this again we'll end up with a test failure even on Linux/OSX.

Finally, the tests that did an explicit .subst have now been changed not to do so; I suspect they date back a few years, before we did any of the work on newline translation.

@p6rt
Copy link
Author

@p6rt p6rt commented Feb 28, 2017

@jnthn - Status changed from 'open' to 'resolved'

@p6rt p6rt closed this Feb 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.