Bug 205291

Created attachment 385803 [details] Patch

Comment on attachment 385803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385803&action=review > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:182 > + self.should_test_processes = not self._platform.is_win() and sys.version_info < (3, 0) This seems like a huge change. Do we know what the downsides are going to be? > Tools/Scripts/webkitpy/layout_tests/views/printing.py:242 > + if timing[1] > min_seconds_to_print: why the change? > Tools/Scripts/webkitpy/port/driver.py:652 > + if early_return: This doesn't seem any more readable to me than just using the if > Tools/Scripts/webkitpy/port/driver.py:695 > + if out_line[-1] != '\n' and out_line[-1] != 10: Why do we need the second check? > Tools/Scripts/webkitpy/port/image_diff.py:62 > + self._process.write(actual_contents) Have you compared the output? You're inserting a newline here where there wasn't one.

Comment on attachment 385803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385803&action=review >> Tools/Scripts/webkitpy/layout_tests/views/printing.py:242 >> + if timing[1] > min_seconds_to_print: > > why the change? The old code was just wrong, not sure how we lived so long with it. Line 236 is: timings.append((directory, round(stats[directory]['total_time'], 1), stats[directory]['num_tests'])) Clearly, timing[0] is the directory, timing[1] is the time the directory took. >> Tools/Scripts/webkitpy/port/driver.py:695 >> + if out_line[-1] != '\n' and out_line[-1] != 10: > > Why do we need the second check? Because Python 2 is silly. The second check is actually the correct one, but Python 2 doesn't like comparing characters against the integer representations of those same characters, so 'foo\n'[-1] != 10. On the other hand, Python 3 doesn't like comparing bytes with characters, so b'foo\n'[-1] != '\n', and even b'foo\n'[-1] != b'\n' won't work in Python 3 >> Tools/Scripts/webkitpy/port/image_diff.py:62 >> + self._process.write(actual_contents) > > Have you compared the output? You're inserting a newline here where there wasn't one. I think there was a newline before, just hiding between %d and %s: self._process.write('Content-Length: %d\n%s...

Created attachment 385839 [details] Patch

>>> + if timing[1] > min_seconds_to_print: >> >> why the change? > > > The old code was just wrong, not sure how we lived so long with it. How does this change the output? I never paid very close attention to the timing printed at the end of test runs, but what I did see appeared reasonable.

(In reply to Alexey Proskuryakov from comment #5) > >>> + if timing[1] > min_seconds_to_print: > >> > >> why the change? > > > > > > The old code was just wrong, not sure how we lived so long with it. > > How does this change the output? I never paid very close attention to the > timing printed at the end of test runs, but what I did see appeared > reasonable. I think the old behavior would have ended up being pretty non-deterministic. We were comparing a string against some limit, the string probably would have usually resolved to something "greater" than min_seconds_to_print. In effect, we probably were printing more than expected, but I'm not exactly sure, and it would depend on the directory name.

Comment on attachment 385839 [details] Patch Clearing flags on attachment: 385839 Committed r253804: <https://trac.webkit.org/changeset/253804>

All reviewed patches have been landed. Closing bug.

<rdar://problem/58097078>

Re-opened since this is blocked by bug 205622

I'm seeing a lot of strange test failures that seem to have started after this patch. Will try rolling back, and will re-land if it doesn't help.

The rollback did eliminate the "diff (null%)" failures seen on many tests. These same tests continue to fail with larger diffs on other hardware though. I think that the regression from this patch is about dealing with reftest mismatches that are within noise level. Before: 04:02:09.776 30742 svg/gradients/spreadMethodDiagonal4.svg -> ref test hashes didn't match but diff passed After: 22:24:23.922 2955 svg/gradients/spreadMethodDiagonal4.svg : ImageDiff timed out 22:24:23.923 2955 ref test mismatch did not produce an image diff. 22:24:23.924 2911 [48488/52894] svg/gradients/spreadMethodDiagonal4.svg failed unexpectedly (reference mismatch)

Created attachment 387233 [details] Patch

(In reply to Alexey Proskuryakov from comment #12) > The rollback did eliminate the "diff (null%)" failures seen on many tests. > These same tests continue to fail with larger diffs on other hardware though. > > I think that the regression from this patch is about dealing with reftest > mismatches that are within noise level. > > Before: > > 04:02:09.776 30742 svg/gradients/spreadMethodDiagonal4.svg -> ref test > hashes didn't match but diff passed > > After: > > 22:24:23.922 2955 svg/gradients/spreadMethodDiagonal4.svg : ImageDiff > timed out > 22:24:23.923 2955 ref test mismatch did not produce an image diff. > 22:24:23.924 2911 [48488/52894] svg/gradients/spreadMethodDiagonal4.svg > failed unexpectedly (reference mismatch) I am pretty sure that this was caused by a failure to decode the content-length. I ran a test with the decode, with the previous patch and now I'm comparing those results against no patch at all.

(In reply to Jonathan Bedard from comment #14) > (In reply to Alexey Proskuryakov from comment #12) > > The rollback did eliminate the "diff (null%)" failures seen on many tests. > > These same tests continue to fail with larger diffs on other hardware though. > > > > I think that the regression from this patch is about dealing with reftest > > mismatches that are within noise level. > > > > Before: > > > > 04:02:09.776 30742 svg/gradients/spreadMethodDiagonal4.svg -> ref test > > hashes didn't match but diff passed > > > > After: > > > > 22:24:23.922 2955 svg/gradients/spreadMethodDiagonal4.svg : ImageDiff > > timed out > > 22:24:23.923 2955 ref test mismatch did not produce an image diff. > > 22:24:23.924 2911 [48488/52894] svg/gradients/spreadMethodDiagonal4.svg > > failed unexpectedly (reference mismatch) > > I am pretty sure that this was caused by a failure to decode the > content-length. I ran a test with the decode, with the previous patch and > now I'm comparing those results against no patch at all. Was able to confirm this is the problem by looking at test results without the change. This makes sense, bytes are likely using their ASCII representation when cast to integers rather than the number that the string actually represents. ImageDiff was timing out because it was waiting on data that would never come.

Comment on attachment 387233 [details] Patch Clearing flags on attachment: 387233 Committed r254340: <https://trac.webkit.org/changeset/254340>

All reviewed patches have been landed. Closing bug.

Reopening to attach new patch.

Created attachment 387344 [details] Patch for landing

(In reply to Jonathan Bedard from comment #19) > Created attachment 387344 [details] > Patch for landing Accidentally broke some Python 3 unit tests in this process, fixing those. We aren't running them in automation yet.

Comment on attachment 387344 [details] Patch for landing Clearing flags on attachment: 387344 Committed r254346: <https://trac.webkit.org/changeset/254346>

All reviewed patches have been landed. Closing bug.

Committed r254762: <https://trac.webkit.org/changeset/254762>