New issue
Advanced search Search tips

Issue 654081 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

blink-w3c-test-autoroller left mysterious difference in html/semantics/grouping-content/the-pre-element/grouping-pre-reftest-001.html

Project Member Reported by foolip@chromium.org, Oct 7 2016

Issue description

I've compared LayoutTests/imported/wpt/ after https://codereview.chromium.org/2382173004 (commit 40c251cdd3915d64c49b1927c2107aa9b6482ce5) with web-platform-tests at commit 1f1ed355979d2d51c4d93bda34145b2064bdd05a and looked for changes with git diff --diff-filter=M.

Very glad to see no systematic changes left, but surprised to see this:

--- a/html/semantics/grouping-content/the-pre-element/grouping-pre-reftest-001.html
+++ b/html/semantics/grouping-content/the-pre-element/grouping-pre-reftest-001.html
@@ -17,7 +17,7 @@
 
     <p>This reftest passes if below you see "ABC ABC" repeated on two separate lines below (4 ABCs total):</p>
 
-    <pre>&#x202E CBA CBA
+    <pre>&#x202E; CBA CBA
 ABC ABC</pre>
 
 </html>

Where did that semicolon come from?
 
BTW, I checked out web-platform-tests at 1f1ed355979d2d51c4d93bda34145b2064bdd05a and then copied the .git directory to Chromium's import, no copying around of the files themselves.
FWIW, the file hasn't changed in either repo for a long time.
That extra semicolon would have come from webkitpy/w3c/test_converter.py, which actually parses the file with HTMLParser and reconstructs it, potentially modifying it in some places.

Specifically, at: https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter.py?l=212

If there's a way to make a small change to make it leave those things alone, that'd be good. In the longer run, if we could make it so that there's no test converter that has to parse the test file and reconstruct it, that'd be better.
Since that was the *only* change, it looks like that parsing and serializing is unnecessary, none of the transforms are actually in effect, so drop the whole thing?
I tried disabling the converter entirely and importing both wpt and csswg-test, and I got this:

https://codereview.chromium.org/2485493002

So, currently: the differences with and without the converter are:

1. In csswg-test:

(a) Path to support file from reference html file is different for reference tests in csswg-test/css-shapes-1/shape-outside/supported-shapes. So we probably still want the behavior of convert_reference_relpaths in _W3CTestConverter. What we really want to do here is to change the layout test runner to understand <link rel=match href="...">, and then the paths in reference files don't need to be changed.

(b) Conversion adds "-webkit-" prefix to text-emphasis CSS property; this does change behavior, since the unprefixed version "text-emphasis" is only supported by Firefox (-webkit-text-emphasis is supported in Chrome, Opera, Safari).

(c) Conversion mangles "support/tcu-font.woff" into "suppotcu-font.woff". The behavior without conversion is correct, and the test could be failing for us just because of the change when the test is imported.
 
2. In wpt:

As observed above, the only change in wpt is &#x202E -> &#x202E;, and so we don't want any conversion in this case.
Cc: tkent@chromium.org jeffcarp@chromium.org
Components: Blink>Infra
Just tried making a CL that disables the converted for tests in wpt for now; it made some more changes that I didn't expect, but it may still be correct: https://codereview.chromium.org/2479383004
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 8 2016

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

commit b44cdf9eb78d092d51f3ad10cef882025716c4be
Author: qyearsley <qyearsley@chromium.org>
Date: Tue Nov 08 23:05:35 2016

Stop converting test contents for tests in imported/wpt.

Looking at  http://crbug.com/654081 , it looks like it should now
be unnecessary except for some cases in csswg-test.

Import wpt@2782308f4187e73043324272552cf1bd90edc784

Using update-w3c-deps in Chromium a0b55895c5825ff8c8d0c363357a1e45c374b814.

BUG= 654081 

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

[modify] https://crrev.com/b44cdf9eb78d092d51f3ad10cef882025716c4be/third_party/WebKit/LayoutTests/imported/wpt/dom/traversal/unfinished/001.xml
[modify] https://crrev.com/b44cdf9eb78d092d51f3ad10cef882025716c4be/third_party/WebKit/LayoutTests/imported/wpt/dom/traversal/unfinished/002.xml
[modify] https://crrev.com/b44cdf9eb78d092d51f3ad10cef882025716c4be/third_party/WebKit/LayoutTests/imported/wpt/dom/traversal/unfinished/010.xml
[modify] https://crrev.com/b44cdf9eb78d092d51f3ad10cef882025716c4be/third_party/WebKit/LayoutTests/imported/wpt/html/editing/dnd/cross-document/002.html
[modify] https://crrev.com/b44cdf9eb78d092d51f3ad10cef882025716c4be/third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/sizes/sizes-iframed.sub.html
[modify] https://crrev.com/b44cdf9eb78d092d51f3ad10cef882025716c4be/third_party/WebKit/LayoutTests/imported/wpt/html/semantics/grouping-content/the-pre-element/grouping-pre-reftest-001.html
[modify] https://crrev.com/b44cdf9eb78d092d51f3ad10cef882025716c4be/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter.py

Status: Fixed (was: Assigned)
The mysterious &#x202E; issue is now solved, although some csswg-test tests are still modified during import; filed  bug 663773  for the rest of the things listed in #5.

Sign in to add a comment