Bug 36983

I'm not actively working on this, so I'm gonna disclaim ownership in the hopes that someone else wants to pick this up for an easy commit ....

(In reply to comment #1) > I'm not actively working on this, so I'm gonna disclaim ownership in the hopes that someone else wants to pick this up for an easy commit .... I will take this one.

I was just bit by this bug. Let's see if I can help and get rid of it.

Created attachment 68339 [details] 1st attempt to fix Not sure whether the two instances of s/^(\r\n)*// in my patch are necessary. I assumed they are, because, otherwise, the original code would have used rstrip instead of strip, but I could be wrong. Also, not entirely sure whether the use of strip was originally intended to remove all trailing "\r\n" or just one of them. I assumed all; otherwise, the original code would have probably been much simpler, something like: text = text.replace("\r\n", "\n") if not text.endswith("\n"): text = text + "\n"

Comment on attachment 68339 [details] 1st attempt to fix Can you please add a regression test for this behavior?

From looking at the review, I would ask a couple of different things: 1) Why is the initial code broken and the new code correct? I.e., how are we/you defining the "correct" behavior? Is this what old-run-webkit-tests (the perl version) does? 2) Have you run this over the layout tests on a couple of different ports and seen if anything starts (or stops) diffing? I would expect something to change here, and wouldn't want to land this without updated baselines as well. As JamesR said, we need unit tests for this and a comment or some other form of documentation as to what the behavior is supposed to be. Matching legacy is arguably good enough, but bonus points for figuring out why old-run-webkit-tests does what it does as well (e.g., win/unix CRLF conversion, Git NL handling, etc.)

Created attachment 76482 [details] 2nd attempt to fix For pre-review only. This needs a lot of rebaselining. Changelogs are missing as well.

Per discussion w/ ctruta in IRC, it looks like the code change will do what he wants it to do, and it looks like what he wants it to do will probably match what ORWT does. He is going to document what he wants the code to do and add tests for it.

Doing the required rebaselining in bug 51018 first.

Created attachment 76634 [details] Fix and unit tests The rebaseline in bug 51018 has landed, and, luckily, no new disruptive tests have arrived. Keeping fingers crossed that things will stay this way until this patch lands.

Comment on attachment 76634 [details] Fix and unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=76634&action=review Thanks again for working on this. I hope you don't mind how meticulous I"m being, but this is (IMO) one of the least-obvious routines in the code, so I'd like for it to be well-tested and documented. > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:327 > + the actual text outputs.""" I wouldn't put the comment about the actual text output here, since this routine has nothing to do with the actual output. If we do want a comment for the actual output (not a bad idea), it should be in the Driver.run_test() method in the same file. > WebKitTools/Scripts/webkitpy/layout_tests/test_types/text_diff.py:56 > + return output.replace("\r\n", "\n").replace("\r\n", "\n") Not to be a pain here, but ... 1) I don't think the code matches the comment any more. '\r\r\n' => '\n', and the comment seems to indicate that it should be '\r\n'. The comment may be wrong, in which case it should be updated. Or, maybe your fix is wrong? (It actually looks like the old code would've also produced '\n'). 2) I think output.replace("\r\r\n", "\r\n").replace("\r\n", "\n") is slightly clearer and the same result as what you have. What you have might lead someone to wonder why you're doing the same replace twice. That said, it's just my opinion, so if you prefer the existing version that's fine. 3) I would also consider adding a comment in test.py about this in passes/text.html, or updating this comment to mention that this is tested for in test.py's passes/text.html example. The former is probably better, since the comment is next to the test and hence harder for it to become out of sync. 4) What happens if the actual output contains "\r\r\r\n"? It looks like that gets would get changed to '\r\n'. Is that what we want? I'd like to see a test with at least one more \r than the number of replace() calls. A test with 10 \r's instead of 3 would be fine as well.

Created attachment 76637 [details] Fix and unit tests Resubmitting to comply with Dirk's comments. (In reply to comment #11) > 4) What happens if the actual output contains "\r\r\r\n"? It looks like that gets would get changed to '\r\n'. Is that what we want? I'd like to see a test with at least one more \r than the number of replace() calls. A test with 10 \r's instead of 3 would be fine as well. I don't think we "want" \r\r\r\n to change to \r\n. I found no tests to require such lines. Instead, I preferred to say that we don't want too many \r...\r\n to be absorbed into \n, because that might mean something's wrong. So I tested this as an expected failure, not as a pass.

Change LGTM! Thanks for all the work.

(In reply to comment #12) > I don't think we "want" \r\r\r\n to change to \r\n. I found no tests to require such lines. > Instead, I preferred to say that we don't want too many \r...\r\n to be absorbed into \n, because that might mean something's wrong. So I tested this as an expected failure, not as a pass. I guess what I was really asking was, does what this version will do match what old-run-webkit-tests does in this case?

Comment on attachment 76637 [details] Fix and unit tests R=me

(In reply to comment #14) > > I don't think we "want" \r\r\r\n to change to \r\n. [...] > I guess what I was really asking was, does what this version will do match what old-run-webkit-tests does in this case? Yes and no. In my best interpretation of ORWT, not even \r\r\n is changed to \n, so, strictly speaking, I wouldn't say that NRWT fully matches ORWT, because of this reason. But in a loose sense, yes: the Python/Cygwin tools are said to convert \r\n to \r\r\n, and NRWT makes up for that. Also in a loose sense, yes, because neither NRWT nor ORWT accept more than two \r characters in \r\r...\r\n. I am personally fond of the idea of investigating why exactly are those \r\r\n being produced. Eliminate that cause, then stop converting \r\r\n to \n. That would be my favorite solution, a lower-priority, but still potentially useful work item that makes the testing even more robust. Then we can absolutely say that NRWT matches ORWT.

Adding Adam Roben, who is likely the only person who might have observed what happens when running old-run-webkit-tests on windows under cygwin. Cosmin, do we know what tests trigger this behavior? Or does someone need to investigate it?

(In reply to comment #17) > Adding Adam Roben, who is likely the only person who might have observed what happens when running old-run-webkit-tests on windows under cygwin. > > Cosmin, do we know what tests trigger this behavior? Or does someone need to investigate it? And do we know if this is only with cygwin python, or win python (and if it's dependent on cygwin's newline conversion settings?)

(In reply to comment #17) > Adding Adam Roben, who is likely the only person who might have observed what happens when running old-run-webkit-tests on windows under cygwin. I'm not sure exactly what's being asked of me, but here's what I think is the current situation for Apple's Windows port: 1) Developers are expected to tell Cygwin to use UNIX line endings 2) Checked-in expected test results have UNIX line endings 3) DRT outputs UNIX line endings (by putting stdout/stderr in binary mode) So I think no carriage returns ever come into play. I could be wrong, though.

Comment on attachment 76637 [details] Fix and unit tests Clearing flags on attachment: 76637 Committed r74136: <http://trac.webkit.org/changeset/74136>

All reviewed patches have been landed. Closing bug.

(In reply to comment #19) > (In reply to comment #17) > > Adding Adam Roben, who is likely the only person who might have observed what happens when running old-run-webkit-tests on windows under cygwin. > > I'm not sure exactly what's being asked of me, but here's what I think is the current situation for Apple's Windows port: > > 1) Developers are expected to tell Cygwin to use UNIX line endings > 2) Checked-in expected test results have UNIX line endings > 3) DRT outputs UNIX line endings (by putting stdout/stderr in binary mode) > > So I think no carriage returns ever come into play. I could be wrong, though. Thanks for clarifying, Adam! I wasn't actually intending to ask you anything, just making you aware of the conversation in case we did find some discrepancy. I'm almost wondering if this comment/bug doesn't actually exist any more, or if was something stupid Chromium was doing on Windows (e.g., before we upstreamed this stuff).

(In reply to comment #22) > (In reply to comment #19) > > (In reply to comment #17) > > > Adding Adam Roben, who is likely the only person who might have observed what happens when running old-run-webkit-tests on windows under cygwin. > > > > I'm not sure exactly what's being asked of me, but here's what I think is the current situation for Apple's Windows port: > > > > 1) Developers are expected to tell Cygwin to use UNIX line endings > > 2) Checked-in expected test results have UNIX line endings > > 3) DRT outputs UNIX line endings (by putting stdout/stderr in binary mode) > > > > So I think no carriage returns ever come into play. I could be wrong, though. > > Thanks for clarifying, Adam! I wasn't actually intending to ask you anything, just making you aware of the conversation in case we did find some discrepancy. I'm almost wondering if this comment/bug doesn't actually exist any more, or if was something stupid Chromium was doing on Windows (e.g., before we upstreamed this stuff). Also adding Tony Chang, who is probably the most familiar with our current DRT win port. For example, one possibility is that since both new-run-webkit-tests and DRT run directly under Win32, we actually were getting CRLFs instead of just LFs.

> Cosmin, do we know what tests trigger this behavior? Or does someone need to investigate it? > And do we know if this is only with cygwin python, or win python (and if it's dependent on cygwin's newline conversion settings?) I know this thing to happen in general, but I do not know of any specifics in this context. > 1) Developers are expected to tell Cygwin to use UNIX line endings I find this a little fragile, and easy to avoid if binary mode I/O is used consistently. 2) Checked-in expected test results have UNIX line endings > Maybe non-chromium ones do, but I found tests with CRLF in platform/chromium-win that are picked up by chromium-linux and chromium-mac. They run fine. > 3) DRT outputs UNIX line endings (by putting stdout/stderr in binary mode) The DRT outputs are fine, I did not have to rebaseline any of those. But I'm pretty sure about the rest of *-expected.txt files that people abuse them in text editors sometimes. Otherwise, I can't explain why I had to look at every single of those 400+ expectation files that needed rebaselining in bug 51018. Most were missing or having extra trailing empty lines, very inconsistently, almost randomly; but some even had missing or extra leading empty lines. If those text files were taken straight out of run-webkit-tests --new-baseline, this wouldn't have happened.

I had the indentation of my response to 2) backwards. Here is how I meant it: > 2) Checked-in expected test results have UNIX line endings Maybe non-chromium ones do, but I found tests with CRLF in platform/chromium-win that are picked up by chromium-linux and chromium-mac. They run fine.

(In reply to comment #24) > > Cosmin, do we know what tests trigger this behavior? Or does someone need to investigate it? > > And do we know if this is only with cygwin python, or win python (and if it's dependent on cygwin's newline conversion settings?) > > I know this thing to happen in general, but I do not know of any specifics in this context. > > > 1) Developers are expected to tell Cygwin to use UNIX line endings > > I find this a little fragile, and easy to avoid if binary mode I/O is used consistently. > If you don't tell users to use UNIX line endings, and you use binary mode I/O, then users may use text editors that save files either with "\n" or "\r\n" depending on what they happened to use for UNIX line endings in Cygwin, no? That would then require the test harness to do the "\r\n" -> "\n" conversion we're doing. > 2) Checked-in expected test results have UNIX line endings > > Maybe non-chromium ones do, but I found tests with CRLF in platform/chromium-win that are picked up by chromium-linux and chromium-mac. They run fine. chromium-mac doesn't (shouldn't) ever look at chromium-win, but otherwise I could certainly believe it, since our code would work in this case but I believe ORWT would choke. > > > 3) DRT outputs UNIX line endings (by putting stdout/stderr in binary mode) > > The DRT outputs are fine, I did not have to rebaseline any of those. But I'm pretty sure about the rest of *-expected.txt files that people abuse them in text editors sometimes. Otherwise, I can't explain why I had to look at every single of those 400+ expectation files that needed rebaselining in bug 51018. Most were missing or having extra trailing empty lines, very inconsistently, almost randomly; but some even had missing or extra leading empty lines. If those text files were taken straight out of run-webkit-tests --new-baseline, this wouldn't have happened. Hard to say if that's true. If we weren't checking for trailing lines before, the tests could easily have been broken at different points and we'd never have noticed.

Opened bug 51147 to add some missing expectation files.

Assigning to myself to own since ctruta isn't a committer yet.

Finished rebaselining in bug 51147, using output files that I took from Chromium's canary build 69353. I _think_ it is safe to re-land the NRWT fix.

Actually, the patch to bug 51147 still needs to land. It got stuck, apparently, for no good reason.

The patch to bug 51147 has landed, so we should be able to re-land this fix.

Chromium try runs seem fairly clean, so we should be good to give this another go. Unfortunately I'm unlikely to have time to babysit this through landing this evening but I could try tomorrow.

reopening it to try again

Created attachment 76919 [details] Fix and unit tests

Comment on attachment 76919 [details] Fix and unit tests Clearing review flag - this was already reviewed by jamesr@chromium.org . Clearing CQ flag - landing now.

Created attachment 77013 [details] Fix and unit tests For some mystery reason, the last patch failed to land last Friday. Sigh... (Could it have been because of the renaming of WebKitTools/ to Tools/ ?) Naturally, even more bad expectation files came up, which I'll address in bug 51340. Is this one of those neverending stories? I will kindly ask one of the Chromium sheriffs to run this patch on the Chromium trybots first, to make sure I won't miss anything in bug 51340.

Comment on attachment 77013 [details] Fix and unit tests R=me. Will land by hand.

Comment on attachment 77013 [details] Fix and unit tests Clearing flags on attachment: 77013 Committed r74362: <http://trac.webkit.org/changeset/74362>

All reviewed patches have been landed. Closing bug.