Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2037743002:
Support converting utf-16 files when importing w3c tests. (Closed)
|
Created:
4 years, 6 months ago by dcampb Modified:
4 years, 5 months ago CC:
blink-reviews, blink-reviews-w3ctests_chromium.org, chromium-reviews, Dirk Pranke, raikiri Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport converting utf-16 files when importing w3c tests.
Previously, the test converter assumed that input files are always valid utf-8;
this change makes the converter also support utf-16 files. There are some files
in the wpt repo that are not valid utf-8 but are valid utf-16, e.g.
web-platform-tests/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html.
BUG=463001
Committed: https://crrev.com/3f020273cd7070adbe0691ca4c88d243d3c91761
Cr-Commit-Position: refs/heads/master@{#397775}
Patch Set 1 #Patch Set 2 : Modified files to match Master/Origin (with changes) #
Total comments: 1
Patch Set 3 : Revisions Made from CL #
Total comments: 2
Patch Set 4 : Support converting utf-16 files when importing w3c tests. #
Messages
Total messages: 24 (12 generated)
dcampb@google.com changed reviewers: + dpranke@chromium.org, qyearsley@chromium.org
lgtm as-is, though you could consider the changes I suggest below. Did you still have other things you wanted to ask about how to test this, or did you figure this out after we traded emails? Also, can you confirm that we do actually still see the problem with current imports before we change things? There's no reason to support UTF-16 if we don't actually have any UTF-16 files that are being imported. https://codereview.chromium.org/2037743002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py (right): https://codereview.chromium.org/2037743002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:331: convert_for_webkit('', '/file', '', host) Yup, this is pretty much how I'd expect you to test it. I would've done this slightly differently, as: from webkitpy.common.system.systemhost_mock import MockSystemHost files = {...} host = MockSystemHost(filesystem=MockFileSystem(files=files)) convert_for_webkit(...) but that's not really all that different from what you did, it just avoids using real Host objects and overriding methods.
On 2016/06/02 19:22:24, Dirk Pranke wrote: > lgtm as-is, though you could consider the changes I suggest below. > > Did you still have other things you wanted to ask about how to test this, or did > you figure this out after we traded emails? > > Also, can you confirm that we do actually still see the problem with current > imports before we change things? There's no reason to support UTF-16 if we don't > actually have any UTF-16 files that are being imported. > > https://codereview.chromium.org/2037743002/diff/20001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py > (right): > > https://codereview.chromium.org/2037743002/diff/20001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:331: > convert_for_webkit('', '/file', '', host) > Yup, this is pretty much how I'd expect you to test it. > > I would've done this slightly differently, as: > > from webkitpy.common.system.systemhost_mock import MockSystemHost > > files = {...} > host = MockSystemHost(filesystem=MockFileSystem(files=files)) > convert_for_webkit(...) > > but that's not really all that different from what you did, it just avoids using > real Host objects > and overriding methods. changes have been made. My host (qyearsley) assisted me in finding the solution after we traded emails. To reproduce the error, I passed in the 'resolving-urls' file located in W3CImportExpectations, That incorporated utf-16. Not sure if its being used now, but if it were ever not skipped, this change will support it.
LGTM, although I have two minor suggestions about optional comment changes. Also, before committing I think it would be good to edit the CL description; usually the CL description contains one line (same as CL title) then details, then the BUG= line, separated by blank lines, e.g.: Support converting utf-16 files when importing w3c tests. Previously, the test converter assumed that input files are always valid utf-8; this change makes the converter also support utf-16 files. There are some files in the wpt repo that are not valid utf-8 but are valid utf-16, e.g. web-platform-tests/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html. BUG=463001 https://codereview.chromium.org/2037743002/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py (right): https://codereview.chromium.org/2037743002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:316: # Test for convert_for_webkit; determing if 'utf-16' decodes correctly I think this comment (and the similar one below) is probably unnecessary, and could be omitted. https://codereview.chromium.org/2037743002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:321: convert_for_webkit('', '/file', '', host) Optionally, you could add a comment that this test passes if no Exception is raised.
Description was changed from ========== Modified test_converter for decoding utf-16 capabilities modified test_converter.py and tested with test_converter_unittest.py BUG=463001 ========== to ========== Support converting utf-16 files when importing w3c tests Previously, the test converter assumed that input files are always valid utf-8; this change makes the converter also support utf-16 files. There are some files in the wpt repo that are not valid utf-8 but are valid utf-16, e.g. web-platform-tests/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html. BUG=463001 ==========
Description was changed from ========== Support converting utf-16 files when importing w3c tests Previously, the test converter assumed that input files are always valid utf-8; this change makes the converter also support utf-16 files. There are some files in the wpt repo that are not valid utf-8 but are valid utf-16, e.g. web-platform-tests/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html. BUG=463001 ========== to ========== Support converting utf-16 files when importing w3c tests Previously, the test converter assumed that input files are always valid utf-8; this change makes the converter also support utf-16 files. There are some files in the wpt repo that are not valid utf-8 but are valid utf-16, e.g. web-platform-tests/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html. BUG=463001 ==========
On 2016/06/02 21:05:17, qyearsley wrote: > LGTM, although I have two minor suggestions about optional comment changes. > > Also, before committing I think it would be good to edit the CL description; > usually the CL description contains one line (same as CL title) then details, > then the BUG= line, separated by blank lines, e.g.: > > > Support converting utf-16 files when importing w3c tests. > > Previously, the test converter assumed that input files are always valid utf-8; > this change makes the converter also support utf-16 files. There are some files > in the wpt repo that are not valid utf-8 but are valid utf-16, e.g. > web-platform-tests/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html. > > BUG=463001 > > https://codereview.chromium.org/2037743002/diff/40001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py > (right): > > https://codereview.chromium.org/2037743002/diff/40001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:316: # > Test for convert_for_webkit; determing if 'utf-16' decodes correctly > I think this comment (and the similar one below) is probably unnecessary, and > could be omitted. > > https://codereview.chromium.org/2037743002/diff/40001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_converter_unittest.py:321: > convert_for_webkit('', '/file', '', host) > Optionally, you could add a comment that this test passes if no Exception is > raised. Okay, changes have been made.
The CQ bit was checked by dcampb@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2037743002/#ps60001 (title: "Support converting utf-16 files when importing w3c tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2037743002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2037743002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2037743002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dcampb@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2037743002/60001
Message was sent while issue was closed.
Description was changed from ========== Support converting utf-16 files when importing w3c tests Previously, the test converter assumed that input files are always valid utf-8; this change makes the converter also support utf-16 files. There are some files in the wpt repo that are not valid utf-8 but are valid utf-16, e.g. web-platform-tests/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html. BUG=463001 ========== to ========== Support converting utf-16 files when importing w3c tests Previously, the test converter assumed that input files are always valid utf-8; this change makes the converter also support utf-16 files. There are some files in the wpt repo that are not valid utf-8 but are valid utf-16, e.g. web-platform-tests/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html. BUG=463001 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Support converting utf-16 files when importing w3c tests Previously, the test converter assumed that input files are always valid utf-8; this change makes the converter also support utf-16 files. There are some files in the wpt repo that are not valid utf-8 but are valid utf-16, e.g. web-platform-tests/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html. BUG=463001 ========== to ========== Support converting utf-16 files when importing w3c tests Previously, the test converter assumed that input files are always valid utf-8; this change makes the converter also support utf-16 files. There are some files in the wpt repo that are not valid utf-8 but are valid utf-16, e.g. web-platform-tests/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html. BUG=463001 Committed: https://crrev.com/3f020273cd7070adbe0691ca4c88d243d3c91761 Cr-Commit-Position: refs/heads/master@{#397775} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3f020273cd7070adbe0691ca4c88d243d3c91761 Cr-Commit-Position: refs/heads/master@{#397775}
Message was sent while issue was closed.
Description was changed from ========== Support converting utf-16 files when importing w3c tests Previously, the test converter assumed that input files are always valid utf-8; this change makes the converter also support utf-16 files. There are some files in the wpt repo that are not valid utf-8 but are valid utf-16, e.g. web-platform-tests/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html. BUG=463001 Committed: https://crrev.com/3f020273cd7070adbe0691ca4c88d243d3c91761 Cr-Commit-Position: refs/heads/master@{#397775} ========== to ========== Support converting utf-16 files when importing w3c tests. Previously, the test converter assumed that input files are always valid utf-8; this change makes the converter also support utf-16 files. There are some files in the wpt repo that are not valid utf-8 but are valid utf-16, e.g. web-platform-tests/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html. BUG=463001 Committed: https://crrev.com/3f020273cd7070adbe0691ca4c88d243d3c91761 Cr-Commit-Position: refs/heads/master@{#397775} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
