[webkit-reviews] review denied: [Bug 42336] Add script to synchronize WebKit and Khronos WebGL tests : [Attachment 66506] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Sep 3 14:06:02 PDT 2010
Kenneth Russell <kbr at google.com> has denied Adrienne Walker <enne at google.com>'s
request for review:
Bug 42336: Add script to synchronize WebKit and Khronos WebGL tests
https://bugs.webkit.org/show_bug.cgi?id=42336
Attachment 66506: Patch
https://bugs.webkit.org/attachment.cgi?id=66506&action=review
------- Additional Comments from Kenneth Russell <kbr at google.com>
Thank you very much for picking this up. It looks like you have some git
history encapsulated in this patch which causes
WebKitTools/Scripts/webkitpy/layout_tests/update-webgl-conformance-tests.py to
be deleted and then added. Could you please clean up this history and
regenerate the patch? r- because of the need for cleanup. Couple of comments
below.
> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index
b4797d02797797bf9acbe68c89b52347283e65f6..00e2dfd7bd42638a32b595f70094fc2ef0788
690 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,14 @@
> +2010-09-02 Adrienne Walker <enne at google.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Add script to synchronize WebKit and Khronos WebGL tests
> + https://bugs.webkit.org/show_bug.cgi?id=42336
> +
> + * Scripts/update-webgl-conformance-tests: Added.
> + * Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:
Added.
> + *
Scripts/webkitpy/layout_tests/update_webgl_conformance_tests_unittest.py:
Added.
> +
> 2010-09-01 Gabor Rapcsanyi <rgabor at inf.u-szeged.hu>
>
> Reviewed by Antonio Gomes.
> diff --git a/WebKitTools/Scripts/update-webgl-conformance-tests
b/WebKitTools/Scripts/update-webgl-conformance-tests
> new file mode 100755
> index
0000000000000000000000000000000000000000..847dec79288e16b5f84c0875ad0f2f44d78f1
bcd
> --- /dev/null
> +++ b/WebKitTools/Scripts/update-webgl-conformance-tests
> @@ -0,0 +1,41 @@
> +#!/usr/bin/env python
> +# Copyright (C) 2010 Google Inc. All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +# * Redistributions in binary form must reproduce the above
> +# copyright notice, this list of conditions and the following disclaimer
> +# in the documentation and/or other materials provided with the
> +# distribution.
> +# * Neither the name of Google Inc. nor the names of its
> +# contributors may be used to endorse or promote products derived from
> +# this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +"""Wrapper around webkitpy/layout_tests/update-webgl-conformance-tests.py"""
> +import os
> +import sys
> +
> +scripts_directory = os.path.dirname(os.path.abspath(sys.argv[0]))
> +webkitpy_directory = os.path.join(scripts_directory, "webkitpy")
> +sys.path.append(os.path.join(webkitpy_directory, "layout_tests"))
> +
> +import update_webgl_conformance_tests
> +
> +if __name__ == '__main__':
> + update_webgl_conformance_tests.main()
> diff --git
a/WebKitTools/Scripts/webkitpy/layout_tests/update-webgl-conformance-tests.py
b/WebKitTools/Scripts/webkitpy/layout_tests/update-webgl-conformance-tests.py
> deleted file mode 100755
> index
e03dbc91a8d03954bb354392fde47dbbf9d2ea8e..0000000000000000000000000000000000000
000
> ---
a/WebKitTools/Scripts/webkitpy/layout_tests/update-webgl-conformance-tests.py
> +++ /dev/null
> @@ -1,112 +0,0 @@
> -#!/usr/bin/env python
> -
> -# Copyright (C) 2010 Google Inc. All rights reserved.
> -#
> -# Redistribution and use in source and binary forms, with or without
> -# modification, are permitted provided that the following conditions
> -# are met:
> -# 1. Redistributions of source code must retain the above copyright
> -# notice, this list of conditions and the following disclaimer.
> -# 2. Redistributions in binary form must reproduce the above copyright
> -# notice, this list of conditions and the following disclaimer in the
> -# documentation and/or other materials provided with the distribution.
> -#
> -# THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> -# EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> -# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> -# PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE COMPUTER, INC. OR
> -# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> -# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> -# PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> -# PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
> -# OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> -# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> -# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> -
> -import glob
> -import os
> -import re
> -import sys
> -from webkitpy.common.system.deprecated_logging import error, log
> -from webkitpy.common.checkout import scm
> -
> -def usage():
> - print os.path.basename(sys.argv[0]) + " (input file or directory)
(optional output directory)"
> -
> -def removeFirstLineComment(text):
> - return re.compile(r'^<!--.*?-->\s*', re.DOTALL).sub('', text)
> -
> -def translateIncludes(text):
> - # Mapping of single filename to relative path under WebKit root.
> - # Assumption: these filenames are globally unique.
> - includeMapping = {
> - "js-test-style.css" : "../../js/resources",
> - "js-test-pre.js" : "../../js/resources",
> - "js-test-post.js" : "../../js/resources",
> - "desktop-gl-constants.js" : "resources",
> - };
> -
> - for filename, path in includeMapping.items():
> - text = re.sub(r'(?:[^"\'= ]*/)?' + re.escape(filename),
os.path.join(path, filename), text)
> -
> - return text
> -
> -def translateKhronosTest(text):
> - """
> - This method translates the contents of a Khronos test to a WebKit test.
> - """
> -
> - translateFuncs = [
> - removeFirstLineComment,
> - translateIncludes,
> - ]
> -
> - for f in translateFuncs:
> - text = f(text)
> -
> - return text
> -
> -def updateFile(inputFileName, outputDir):
> - # check inputFileName exists
> - # check outputDir exists
> - outputFileName = os.path.join(outputDir,
os.path.basename(inputFileName))
> -
> - with open(inputFileName, 'r') as inputFile:
> - with open(outputFileName, 'w') as outputFile:
> - outputFile.write(translateKhronosTest(inputFile.read()))
> -
> -def updateDirectory(inputDir, outputDir):
> - for fileName in glob.glob(os.path.join(inputDir, '*.html')):
> - updateFile(os.path.join(inputDir, fileName), outputDir)
> -
> -def defaultOutputDir():
> - current_scm = scm.detect_scm_system(os.path.dirname(sys.argv[0]))
> - if (not current_scm):
> - error("No SCM detected. Please specify an explict output dir.");
> - root_dir = current_scm.checkout_root
> - if (not root_dir):
> - error("Couldn't find SCM root. Please specify an explicit output
dir.");
> - return os.path.join(root_dir, "LayoutTests/fast/canvas/webgl")
> -
> -def main(argv):
> - if "--help" in argv or len(argv) == 0:
> - usage();
> - sys.exit(0)
> - inputName = argv[0]
> - if len(argv) == 1:
> - outputDir = defaultOutputDir()
> - else:
> - outputDir = argv[1]
> -
> - if (os.path.isfile(inputName)):
> - updateFile(inputName, outputDir)
> - elif (os.path.isdir(inputName)):
> - updateDirectory(inputName, outputDir)
> - else:
> - error("'" + inputName + "' is not a directory or a file.");
> - sys.exit(2)
> -
> - sys.exit(0)
> -
> -if __name__ == "__main__":
> - main(sys.argv[1:])
> diff --git
a/WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py
b/WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py
> new file mode 100755
> index
0000000000000000000000000000000000000000..ecf2b67f0e1158243df9aa9114227773ef453
681
> --- /dev/null
> +++
b/WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py
> @@ -0,0 +1,158 @@
> +#!/usr/bin/env python
> +
> +# Copyright (C) 2010 Google Inc. All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions
> +# are met:
> +# 1. Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +# 2. Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the distribution.
> +#
> +# THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> +# EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> +# PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE COMPUTER, INC. OR
> +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> +# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> +# PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> +# PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
> +# OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +import glob
> +import logging
> +import optparse
> +import os
> +import re
> +import sys
> +import webkitpy.common.checkout.scm as scm
> +
> +_log = logging.getLogger("webkitpy.layout_tests."
> + "update-webgl-conformance-tests")
> +
> +
> +def remove_first_line_comment(text):
> + return re.compile(r'^<!--.*?-->\s*', re.DOTALL).sub('', text)
How does this guarantee that it only matches on the first line of the input, or
not at all? Not all of the conformance tests have the copyright header we're
trying to remove with this regexp, and we don't want to delete comments in the
middle of the tests.
> +
> +
> +def translate_includes(text):
> + # Mapping of single filename to relative path under WebKit root.
> + # Assumption: these filenames are globally unique.
> + include_mapping = {
> + "js-test-style.css": "../../js/resources",
> + "js-test-pre.js": "../../js/resources",
> + "js-test-post.js": "../../js/resources",
> + "desktop-gl-constants.js": "resources",
> + }
> +
> + for filename, path in include_mapping.items():
> + search = r'(?:[^"\'= ]*/)?' + re.escape(filename)
Why are '=' and ' ' included in the complement of the character set here? Would
it be simpler to just hardcode the full source and destination paths in the
include_mapping?
> + replace = os.path.join(path, filename)
> + text = re.sub(search, replace, text)
> +
> + return text
> +
> +
> +def translate_khronos_test(text):
> + """
> + This method translates the contents of a Khronos test to a WebKit test.
> + """
> +
> + translateFuncs = [
> + remove_first_line_comment,
> + translate_includes,
> + ]
> +
> + for f in translateFuncs:
> + text = f(text)
> +
> + return text
> +
> +
> +def update_file(in_filename, out_dir):
> + # check in_filename exists
> + # check out_dir exists
> + out_filename = os.path.join(out_dir, os.path.basename(in_filename))
> +
> + _log.debug("Processing " + in_filename)
> + with open(in_filename, 'r') as in_file:
> + with open(out_filename, 'w') as out_file:
> + out_file.write(translate_khronos_test(in_file.read()))
> +
> +
> +def update_directory(in_dir, out_dir):
> + for filename in glob.glob(os.path.join(in_dir, '*.html')):
> + update_file(os.path.join(in_dir, filename), out_dir)
> +
> +
> +def default_out_dir():
> + current_scm = scm.detect_scm_system(os.path.dirname(sys.argv[0]))
> + if (not current_scm):
> + return os.getcwd()
> + root_dir = current_scm.checkout_root
> + if (not root_dir):
> + return os.getcwd()
> + out_dir = os.path.join(root_dir, "LayoutTests/fast/canvas/webgl")
> + if (os.path.isdir(out_dir)):
> + return out_dir
> + else:
> + return os.getcwd()
> +
> +
> +def configure_logging(options):
> + """Configures the logging system."""
> + log_fmt = '%(levelname)s: %(message)s'
> + log_datefmt = '%y%m%d %H:%M:%S'
> + log_level = logging.INFO
> + if options.verbose:
> + log_fmt = ('%(asctime)s %(filename)s:%(lineno)-4d %(levelname)s '
> + '%(message)s')
> + log_level = logging.DEBUG
> + logging.basicConfig(level=log_level, format=log_fmt,
> + datefmt=log_datefmt)
> +
> +
> +def get_option_parser():
> + usage = "usage: %prog [options] (input file or directory)"
> + option_parser = optparse.OptionParser(usage=usage)
> + option_parser.add_option('-v', '--verbose',
> + action='store_true',
> + default=False,
> + help='include debug-level logging')
> + option_parser.add_option('-o', '--output',
> + action='store',
> + type='string',
> + default=default_out_dir(),
> + metavar='DIR',
> + help='specify an output directory to place
files '
> + 'in [default: %default]')
> + return option_parser
> +
> +
> +def main():
> + option_parser = get_option_parser()
> + (options, args) = option_parser.parse_args()
> + configure_logging(options)
> +
> + if (len(args) == 0):
> + _log.error("Must specify an input directory or filename.")
> + option_parser.print_help()
> + sys.exit(1)
> +
> + in_name = args[0]
> + if (os.path.isfile(in_name)):
> + update_file(in_name, options.output)
> + elif (os.path.isdir(in_name)):
> + update_directory(in_name, options.output)
> + else:
> + _log.error("'" + in_name + "' is not a directory or a file.")
> + sys.exit(2)
> +
> + sys.exit(0)
> +
> +if __name__ == "__main__":
> + main()
> diff --git
a/WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests_unit
test.py
b/WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests_unit
test.py
> new file mode 100644
> index
0000000000000000000000000000000000000000..786c60325794d2dad34890338bc87f6010fe0
fe5
> --- /dev/null
> +++
b/WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests_unit
test.py
> @@ -0,0 +1,104 @@
> +#!/usr/bin/python
> +# Copyright (C) 2010 Google Inc. All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +# * Redistributions in binary form must reproduce the above
> +# copyright notice, this list of conditions and the following disclaimer
> +# in the documentation and/or other materials provided with the
> +# distribution.
> +# * Neither the name of Google Inc. nor the names of its
> +# contributors may be used to endorse or promote products derived from
> +# this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +"""Unit tests for update_webgl_conformance_tests."""
> +
> +import unittest
> +from webkitpy.layout_tests import update_webgl_conformance_tests as webgl
> +
> +
> +def construct_script(name):
> + return "<script src=\"" + name + "\"></script>\n"
> +
> +
> +def construct_style(name):
> + return "<link rel=\"stylesheet\" href=\"" + name + "\">"
> +
> +
> +class TestTranslation(unittest.TestCase):
> + def assert_unchanged(self, text):
> + self.assertEqual(text, webgl.translate_khronos_test(text))
> +
> + def assert_translate(self, input, output):
> + self.assertEqual(output, webgl.translate_khronos_test(input))
> +
> + def test_simple_unchanged(self):
> + self.assert_unchanged("")
> + self.assert_unchanged("<html></html>")
> +
> + def test_header_strip(self):
> + single_line_header = "<!-- single line header. -->"
> + multi_line_header = """<!-- this is a multi-line
> + header. it should all be removed too.
> + -->"""
> + text = "<html></html>"
> + self.assert_translate(single_line_header, "")
> + self.assert_translate(single_line_header + text, text)
> + self.assert_translate(multi_line_header + text, text)
> +
> + def test_include_rewriting(self):
> + # Mappings to None are unchanged
> + styles = {
> + "../resources/js-test-style.css":
"../../js/resources/js-test-style.css",
> + "fail.css": None,
> + "resources/stylesheet.css": None,
> + "../resources/style.css": None,
> + }
> + scripts = {
> + "../resources/js-test-pre.js":
"../../js/resources/js-test-pre.js",
> + "../resources/js-test-post.js":
"../../js/resources/js-test-post.js",
> + "../resources/desktop-gl-constants.js":
"resources/desktop-gl-constants.js",
> +
> + "resources/shadow-offset.js": None,
> + "../resources/js-test-post-async.js": None,
> + }
> +
> + input_text = ""
> + output_text = ""
> + for input, output in styles.items():
> + input_text += construct_style(input)
> + if (output):
> + output_text += construct_style(output)
> + else:
> + output_text += construct_style(input)
> + for input, output in scripts.items():
> + input_text += construct_script(input)
> + if (output):
> + output_text += construct_script(output)
> + else:
> + output_text += construct_script(input)
> +
> + head = '<!DOCTYPE HTML PUBLIC "-//IETF//DTD
HTML//EN">\n<html>\n<head>\n'
> + foot = '</head>\n<body>\n</body>\n</html>'
> + input_text = head + input_text + foot
> + output_text = head + output_text + foot
> + self.assert_translate(input_text, output_text)
> +
> +if __name__ == '__main__':
> + unittest.main()
More information about the webkit-reviews
mailing list