New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 672651 link

Starred by 1 user
Status: Fixed
Owner: ----
Closed: May 2018
Cc:
█ mkwst@chromium.org
█ tansell@chromium.org
tkent@chromium.org
qyears...@chromium.org
jeffcarp@chromium.org
dpranke@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 620270
issue 691883



Sign in to add a comment

Replace wdiff and PrettyPatch.rb with simple Python diff utilities.

Project Member Reported by qyears...@chromium.org, Dec 9 2016 
Context:  bug 637478 .

After disabling wdiff on Windows, the rate of hangs on win_chromium_rel_ng dropped (around 5% to around 1%), indicating that most but not all of the hangs on Windows happened while waiting for wdiff.

If possible, using a Python module to replace wdiff could result in avoiding creating a wdiff process for every test, which would reduce the rate of hanging (and would remove an external dependency).

This is related to  bug 658329  (remove/replace PrettyPatch.rb).


It looks like PrettyPatch and wdiff are both used to create easily-viewable diffs for text mismatch failures, and when both PrettyPatch and wdiff are available, there are links to both diffs from the results.html.

Example: see https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Trusty/20809/layout-test-results/results.html with "expected failures" checked.

It looks like wdiff might currently be redundant and could just be removed without breaking anything. Does anyone know otherwise?

Comment 1 by qyears...@chromium.org, Dec 9 2016

Components: Blink>Infra

Comment 2 by dpranke@chromium.org, Dec 9 2016 

I believe that we don't have ruby installed on the windows builders, and so wdiff is the only alternative we have at the moment to plain diffs. 

Mac actually only has ruby (no wdiff). Linux has both.

It's not clear to me how upset anyone would be if we only had plain diffs these days. I'd guess some :).

At any rate, I think it's probably not a lot of work to get some sort of word-by-word diff implemented in python; you might start by looking at 

https://github.com/paulgb/simplediff/tree/master/python

(though we'd have to double-check the license).

Ultimately I'd like to get to the point where we have something implemented in pure python so that we don't need to launch a subprocess or depend on a c++ binary or ruby.

Comment 3 by dpranke@chromium.org, Dec 9 2016 

Oh yeah, I forgot that we're probably better off actually integrating

https://code.google.com/p/google-diff-match-patch/

Comment 4 by qyears...@chromium.org, Dec 9 2016

Summary: Replace wdiff and PrettyPatch.rb with a single Python diff library. (was: Remove or replace wdiff (or re-enable wdiff on Windows).)
Good point, since Ruby isn't available on Windows, the nicest option here might be to replace PrettyPatch.rb with something supported on all platforms, and then remove wdiff.

#3: Is google-diff-match-patch maintained recently? Earlier in  bug 658329  you also mentioned difflib (https://docs.python.org/2/library/difflib.html), which is part of the standard library and supports HTML output, so that may be a promising option.

Comment 5 by qyears...@chromium.org, Dec 9 2016

Cc: mkwst@chromium.org
 Issue 658329  has been merged into this issue.

Comment 6 by qyears...@chromium.org, Dec 15 2016

Owner: qyears...@chromium.org
Status: Started (was: Available)
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 19 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78fa99563509c4f515b37d228f1f93a7f9c1300b

commit 78fa99563509c4f515b37d228f1f93a7f9c1300b
Author: qyearsley <qyearsley@chromium.org>
Date: Mon Dec 19 19:04:27 2016

Add Python html_diff module.

This CL adds a module which is intended to produce simple HTML
diffs to view with layout test results.

BUG= 672651 

Review-Url: https://codereview.chromium.org/2580143002
Cr-Commit-Position: refs/heads/master@{#439516}

[add] https://crrev.com/78fa99563509c4f515b37d228f1f93a7f9c1300b/third_party/WebKit/Tools/Scripts/webkitpy/common/html_diff.py
[add] https://crrev.com/78fa99563509c4f515b37d228f1f93a7f9c1300b/third_party/WebKit/Tools/Scripts/webkitpy/common/html_diff_unittest.py

Comment 8 by qyears...@chromium.org, Dec 22 2016

Blocking: 620270
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/498957556586803f87827652555e1b29d7b4866a

commit 498957556586803f87827652555e1b29d7b4866a
Author: qyearsley <qyearsley@chromium.org>
Date: Tue Dec 27 18:16:51 2016

Always use html_diff to generate -pretty-diff.html files.

The idea of this patch is to start using html_diff for -pretty-diff.html
files; once these files are always available then the wdiff-related code
can be removed and there will still be some kind of HTML diff on Windows.

In order to keep this change minimal, the files are still called
-pretty-diff.html and "has_pretty_patch" is still set in the results JSON,
but after this, the "has_pretty_patch" and "has_wdiff" keys in the results
JSON should be removed.

BUG= 672651 

Review-Url: https://codereview.chromium.org/2580343003
Cr-Commit-Position: refs/heads/master@{#440765}

[modify] https://crrev.com/498957556586803f87827652555e1b29d7b4866a/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py
[modify] https://crrev.com/498957556586803f87827652555e1b29d7b4866a/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py
[modify] https://crrev.com/498957556586803f87827652555e1b29d7b4866a/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 12 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7

commit fa4d63fe4de1c958b38ca404fffeeab5f883bfa7
Author: qyearsley <qyearsley@chromium.org>
Date: Thu Jan 12 22:41:59 2017

Remove use of wdiff from layout test runner.

Now, after http://crrev.com/2580343003, HTML diffs are available on all platforms, and dependency on wdiff can be removed.

BUG= 672651 

Review-Url: https://codereview.chromium.org/2582293004
Cr-Commit-Position: refs/heads/master@{#443391}

[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/LayoutTests/fast/harness/resources/results-test.js
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/LayoutTests/fast/harness/results-expected.txt
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/LayoutTests/fast/harness/results.html
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/common/net/layout_test_results_unittest.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android_unittest.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/mac.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/win.py
[modify] https://crrev.com/fa4d63fe4de1c958b38ca404fffeeab5f883bfa7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py

Comment 12 by qyears...@chromium.org, Feb 14 2017

Blocking: 691883

Comment 13 by ojan@chromium.org, Mar 7 2017

Cc: -ojan@chromium.org

Comment 14 by qyears...@chromium.org, Apr 21 2017

Cc: qyears...@chromium.org
Owner: ----
Status: Available (was: Started)
Summary: Replace wdiff and PrettyPatch.rb with simple Python diff utilities. (was: Replace wdiff and PrettyPatch.rb with a single Python diff library.)
PrettyPatch.rb hasn't been completely replaced, and the reason is that `webkit-patch pretty-diff` works by getting a git diff (possibly including many files) then prettyifying it, whereas the html_diff just makes a pretty diff of two strings.

In order to finish this, we should write a Python module to convert git diff text to pretty HTML diff text.

Comment 15 by foolip@chromium.org, Jan 2 2018

Labels: -Pri-2 Pri-3
 Issue 620270  is P3, so I'm going to boldly claim that this is P3 code health as well. Please undo if that's no right. Or close it if #11 was the last thing to do here.

(Doing triage of Blink>Infra P2 issues with no activity in 60+ days.)

Comment 16 by tkent@chromium.org, May 2 2018

Cc: tkent@chromium.org
work in progress: https://chromium-review.googlesource.com/1039121

Comment 17 by qyears...@chromium.org, May 2 2018 

That's neat! It would be good to remove "webkitruby"/"blinkruby".
Project Member

Comment 18 by bugdroid1@chromium.org, May 8 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/159b8efaa7cdd682b528c4f5d07ab4e2aaea0a91

commit 159b8efaa7cdd682b528c4f5d07ab4e2aaea0a91
Author: Kent Tamura <tkent@chromium.org>
Date: Tue May 08 00:20:34 2018

blinkpy: Remove PrettyPatch-related code from Port class.

run_web_tests.py doesn't use PrettyPatch any longer.

Bug:  672651 
Change-Id: Ia57fc68aaf81b8b60e98f4c6e68bf84b507a3391
Reviewed-on: https://chromium-review.googlesource.com/1045958
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556619}
[modify] https://crrev.com/159b8efaa7cdd682b528c4f5d07ab4e2aaea0a91/third_party/blink/tools/blinkpy/web_tests/port/base.py
[modify] https://crrev.com/159b8efaa7cdd682b528c4f5d07ab4e2aaea0a91/third_party/blink/tools/blinkpy/web_tests/port/base_unittest.py
[modify] https://crrev.com/159b8efaa7cdd682b528c4f5d07ab4e2aaea0a91/third_party/blink/tools/blinkpy/web_tests/port/port_testcase.py
Project Member

Comment 19 by bugdroid1@chromium.org, May 9 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9aca9b6fdc082bacccbf55ea26809dcf935edc6c

commit 9aca9b6fdc082bacccbf55ea26809dcf935edc6c
Author: Kent Tamura <tkent@chromium.org>
Date: Wed May 09 01:19:47 2018

blinkpy: Introduce diff prettifier written in Python

It replaces prettifier written in Ruby.

This CL moves the remaining content of prettypatch.py to tools/
commands/pretty_diff.py because it's very small and there are
no other callsites.

Bug:  672651 
Change-Id: I91e94356a5cfd73cf714b09f3b47bb4cab738454
Reviewed-on: https://chromium-review.googlesource.com/1039121
Commit-Queue: Kent Tamura <tkent@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557047}
[add] https://crrev.com/9aca9b6fdc082bacccbf55ea26809dcf935edc6c/third_party/blink/tools/blinkpy/common/base85.py
[add] https://crrev.com/9aca9b6fdc082bacccbf55ea26809dcf935edc6c/third_party/blink/tools/blinkpy/common/base85_unittest.py
[add] https://crrev.com/9aca9b6fdc082bacccbf55ea26809dcf935edc6c/third_party/blink/tools/blinkpy/common/pretty_diff.py
[add] https://crrev.com/9aca9b6fdc082bacccbf55ea26809dcf935edc6c/third_party/blink/tools/blinkpy/common/pretty_diff_unittest.py
[delete] https://crrev.com/ec75dd2ad7e33b1f1539b1dab7e79fd13d687e25/third_party/blink/tools/blinkpy/common/prettypatch.py
[modify] https://crrev.com/9aca9b6fdc082bacccbf55ea26809dcf935edc6c/third_party/blink/tools/blinkpy/tool/commands/pretty_diff.py
[delete] https://crrev.com/ec75dd2ad7e33b1f1539b1dab7e79fd13d687e25/third_party/blink/tools/blinkruby/PrettyPatch.rb
[delete] https://crrev.com/ec75dd2ad7e33b1f1539b1dab7e79fd13d687e25/third_party/blink/tools/blinkruby/PrettyPatch_test.rb
[delete] https://crrev.com/ec75dd2ad7e33b1f1539b1dab7e79fd13d687e25/third_party/blink/tools/blinkruby/diff.rb
[delete] https://crrev.com/ec75dd2ad7e33b1f1539b1dab7e79fd13d687e25/third_party/blink/tools/blinkruby/prettify.rb

Comment 20 by tkent@chromium.org, May 9 2018

Status: Fixed (was: Available)

Comment 21 by dpranke@chromium.org, May 9 2018 

Nice! Can we remove the check for ruby being installed in base.py as well now?

Comment 22 by tkent@chromium.org, May 9 2018 

> Can we remove the check for ruby being installed in base.py as well now?

Yes. It was already removed!
►

Sign in to add a comment