blink-w3c-test-autoroller left mysterious difference in html/semantics/grouping-content/the-pre-element/grouping-pre-reftest-001.html |
|||
Issue descriptionI'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>‮ CBA CBA + <pre>‮ CBA CBA ABC ABC</pre> </html> Where did that semicolon come from?
,
Oct 7 2016
FWIW, the file hasn't changed in either repo for a long time.
,
Oct 7 2016
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.
,
Oct 7 2016
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?
,
Nov 8 2016
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 ‮ -> ‮, and so we don't want any conversion in this case.
,
Nov 8 2016
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
,
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
,
Nov 9 2016
The mysterious ‮ 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 |
|||
Comment 1 by foolip@chromium.org
, Oct 7 2016