Bug 54918

Summary:

PrettyPatch.rb doesn't handle diffs where lines matching /^--/ are missing properly

Product:

WebKit

Reporter:

Dirk Pranke <dpranke>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aroben, ojan

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
sample changed version of the test file that will cause PrettyPatch to crash	none
the diff produced by the test in the first attachment. PrettyPatch will crash on this directly.	none
Patch	ojan: review+

Dirk Pranke

Reported 2011-02-21 17:34:50 PST

LayoutTests/platform/mac/fast/objc/dom-html-select-activate.html has a few statements of the form log("-- foo") in it, which causes a text line containing two dashes at the start of the line to be output in dumpAsText() mode. If the test fails in such a way that those lines aren't output (as it does on chromium-mac at the moment), then the unified diff contains a line with "---" at the start. PrettyPatch.rb interprets that as the start of a new file for diffing, and gets confused when there's no filename, and errors out. I think, in FileDiff.parse, it needs to be taught to understand the "@@ -1,26 +1,6 @@" line and skip over the appropriate number of lines of input correctly, rather than just looking for a line containing "---". My Ruby is weak, and I am lazy, and PrettyPatch appears to have no unit tests, so I haven't actually submitted a patch to do this. If, however, you change dom-html-select-activate to the contents of the first attachment, you'll get the diff in the second attachment, and old-run-webkit-tests will keel over with PrettyPatch failing. In the meantime, we could also just change the test and the expected output to not contain lines starting with "--" :). I'll attach a patch to do that as well.

Attachments
sample changed version of the test file that will cause PrettyPatch to crash (412 bytes, text/html) 2011-02-21 17:37 PST, Dirk Pranke	no flags	Details
the diff produced by the test in the first attachment. PrettyPatch will crash on this directly. (961 bytes, text/plain) 2011-02-21 17:37 PST, Dirk Pranke	no flags	Details
Patch (2.68 KB, patch) 2011-02-21 17:45 PST, Dirk Pranke	ojan: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Dirk Pranke

Comment 1 2011-02-21 17:37:06 PST

Created attachment 83244 [details] sample changed version of the test file that will cause PrettyPatch to crash

Dirk Pranke

Comment 2 2011-02-21 17:37:33 PST

Created attachment 83245 [details] the diff produced by the test in the first attachment. PrettyPatch will crash on this directly.

Dirk Pranke

Comment 3 2011-02-21 17:45:19 PST

Created attachment 83247 [details] Patch

Ojan Vafai

Comment 4 2011-02-21 19:39:47 PST

Comment on attachment 83247 [details] Patch I'd eventually like to see prettypatch fixed. Can you file a separate bug for that and make this bug specifically about the layout test?

Dirk Pranke

Comment 5 2011-02-21 19:44:13 PST

(In reply to comment #4) > (From update of attachment 83247 [details]) > I'd eventually like to see prettypatch fixed. Can you file a separate bug for that and make this bug specifically about the layout test? Sure.

Dirk Pranke

Comment 6 2011-02-21 19:45:06 PST

Committed r79281: <http://trac.webkit.org/changeset/79281>

Note You need to log in before you can comment on or make changes to this bug.