Bug 42336 – Add script to synchronize WebKit and Khronos WebGL tests

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

The synchronization of WebGL tests in the Khronos repository which originated in the WebKit repository is a mechanical process, and should be automated with a script.

[Kenneth Russell]

Created attachment 61608 [details]
Patch

From the ChangeLog:

Wrote script to automate the process of copying WebGL tests which originated in the WebKit repository from a local checkout of the Khronos repository. Verified manually with a dry run of the script that the desired results will be achieved. The script will be run, and its results and rebaseline results checked in, in a following bug. Note that this script is being added to fast/canvas/webgl/ rather than WebKitTools/Scripts to keep it co-located with the files it affects, and the directory in which it must be run.

[Darin Fisher (:fishd, Google)]

Comment on attachment 61608 [details]
Patch

Scripts are ordinarily placed under the WebKitTools/Scripts directory.  Is there
precedent for scripts like this (that help to update tests) living under LayoutTests?

[Kenneth Russell]

(In reply to comment #2)
> (From update of attachment 61608 [details])
> Scripts are ordinarily placed under the WebKitTools/Scripts directory.  Is there
> precedent for scripts like this (that help to update tests) living under LayoutTests?

I don't think there is a precedent, but as I mentioned in the ChangeLog, it will be confusing to put this script in WebKitTools/Scripts for two reasons. The first is that it is intended to affect files in the LayoutTests/fast/canvas/webgl/ directory, and the second is that as written it needs to be run from that directory. I think that the script is likely to be lost or forgotten if it isn't placed alongside the tests.

[Adam Barth]

Comment on attachment 61608 [details]
Patch

Scripts need to go in WebKitTools/Scripts.  Also, we prefer Perl or Python to bash.

[Adam Barth]

I chatted with Kenneth.  Ideally, this business logic would be integrated with webkitpy so we can gain the network effects of the rest of the infrastructure.  For example,

1) it could use the scm module to add new tests working copy,
2) it could generate patches and upload them to bugs.webkit.org,
3) we could write a bot that synced the tests automatically and filed bugs whenever they were out of sync.

I'm not saying that we should do all those things right now, but rather that those things become easy if this script isn't an island.

We also taked about testing.  In general, I'd rather we didn't accept new scripts that don't have testing, or (at a minimum) are designed in a way that can be tested.  Without tests, folks won't be able to improve the script because they'll be afraid of breaking it.

[Kenneth Russell]

I'm removing this bug as a blocker for 42185. I need to get the Khronos and WebKit WebGL tests synchronized today so my colleague can land his outstanding patches before going on vacation. I'll work more on this script in the future.

[Adrienne Walker]

Created attachment 66506 [details]
Patch

[Kenneth Russell]

Comment on attachment 66506 [details]
Patch

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..00e2dfd7bd42638a32b595f70094fc2ef0788690 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,14 @@
> [phone]  Adrienne Walker  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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..847dec79288e16b5f84c0875ad0f2f44d78f1bcd
> --- /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..0000000000000000000000000000000000000000
> --- 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..ecf2b67f0e1158243df9aa9114227773ef453681
> --- /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_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests_unittest.py
> new file mode 100644
> index 0000000000000000000000000000000000000000..786c60325794d2dad34890338bc87f6010fe0fe5
> --- /dev/null
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests_unittest.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()

[Adrienne Walker]

(In reply to comment #8)
> (From update of attachment 66506 [details])
> 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.

Odd.  That an unexpected result from creating and renaming a file.  I can fix it though.

> > +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.

I'll add a test to demonstrate this explicitly, but the '^' means the start of the string rather than the start of a line when you're using the DOTALL option.

> > +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? 

'=' and ' ' were just to catch nonsense like "<script src = foo.js>" which is bad form but still accepted.

> Would it be simpler to just hardcode the full source and destination paths in the include_mapping?

I figured this was equally simple but additionally more robust to files moving around in the Khronos repository in the future.

[Adrienne Walker]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 66506 [details] [details])
> > > +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.
> 
> I'll add a test to demonstrate this explicitly, but the '^' means the start of the string rather than the start of a line when you're using the DOTALL option.

This still works as expected, but ignore my incorrect statement about re.DOTALL.  Except when using re.M, '^' means the start of the string.  In this case, it's the beginning of the file because the string contains the entire file.

[Adrienne Walker]

Created attachment 66550 [details]
Patch

[Kenneth Russell]

Comment on attachment 66550 [details]
Patch

This looks great. Your unit tests are awesome. Thanks again for picking this up.

[WebKit Commit Bot]

Comment on attachment 66550 [details]
Patch

Rejecting patch 66550 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 66550 [details]
Patch

Rejecting patch 66550 from commit-queue.

Failed to run "['WebKitTools/Scripts/test-webkitpy']" exit_code: 1
Last 500 characters of output:
romName
    module = __import__('.'.join(parts_copy))
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests_unittest.py", line 33, in <module>
    from webkitpy.layout_tests import update_webgl_conformance_tests as webgl
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py", line 82
    with open(in_filename, 'r') as in_file:
            ^
SyntaxError: invalid syntax

Full output: http://queues.webkit.org/results/3944104

[Adrienne Walker]

Created attachment 66732 [details]
Patch

[Adrienne Walker]

(In reply to comment #15)
> Created an attachment (id=66732) [details]
> Patch

This is identical to the previous patch except that I imported the with statement from the __future__.  I suspect that's the cause of the above problem.

[Eric Seidel (no email)]

Comment on attachment 66732 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66732&action=prettypatch

Seems like your addition to layout_tests should be a class, not just a bunch of free functions.  I'm also not sure why this belongs in layout_tests (then again layout_tests is already a mess).

In general this looks fine.  Just needs some cleanup.

> WebKitTools/Scripts/update-webgl-conformance-tests:36
> +sys.path.append(os.path.join(webkitpy_directory, "layout_tests"))
No no no.  This is evil and wrong.  The new-run-webkit-tests hack like this is also wrong.  NRWT was attempting to make it possible to run run_chromium_webkit_tests.py directly from chromium w/o depending on python 2.5.  That hack should be removed these days, and not repeated here.

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:143
> +    if (len(args) == 0):
no parens.

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests_unittest.py:92
> +            if (output):
> +                output_text += construct_style(output)
> +            else:
> +                output_text += construct_style(input)
This seems like an overly verbose way of writing this ternary.

[Eric Seidel (no email)]

My comments ended out of order a bit (due to my confusion with the new review tool).  End result: your patch looks OK, but I'd like to see an updated one with a few of the above mentioned cleanups.

The overarching goal with our python in WebKit is to design for re-use.  Not to just be a pile of scripts (like our perl is/was).  So classes, and testing, and good factoring of logic into the various common re-use places are important.

Thanks for the patch!

[Eric Seidel (no email)]

Comment on attachment 66732 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66732&action=prettypatch

About making it a class, it just helps separate the logic.  You make it SyncKronosTests(object) and then instantiate it in your __main__ and call SyncKronosTests(object).main(args), etc.

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:96
> +    if (not current_scm):
no parens.

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:99
> +    if (not root_dir):
again

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:102
> +    if (os.path.isdir(out_dir)):
again

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:121
> +def get_option_parser():
I'm pretty sure that pep8 is anti-"get" prefixes.  WebKit C++ code certainly is.

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:149
> +    if (os.path.isfile(in_name)):
no parens.

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:151
> +    elif (os.path.isdir(in_name)):
I think we have a python lint tool somewhere to catch this stuff.  I'm surprised check-webkit-style didn't catch it, it has a pep8 checker iirc.

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:154
> +        _log.error("'" + in_name + "' is not a directory or a file.")
I would have used %s.

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:157
> +    sys.exit(0)
You could just "return 0" from main if you set up your caller to exit with main's return.  I think that's slighly more common, but I coudl be wrong.

[Adrienne Walker]

Created attachment 66785 [details]
Patch

[Kenneth Russell]

Comment on attachment 66785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66785&action=prettypatch

I think this basically looks fine and since there haven't been any updates on the bug from the original reviewer for a week I'm marking it r+ since we need this script to incorporate more layout tests from the Khronos site.

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:151
> +    elif (os.path.isdir(in_name)):
According to the previous review this shouldn't have parentheses.

[Eric Seidel (no email)]

Comment on attachment 66785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66785&action=prettypatch

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:105
> +    else:
> +        return os.getcwd()
WebKit C++ code avoids else after return.  Python probably should too.

> WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests_unittest.py:100
> +        self.assert_translate(input_text, output_text)
> +
> +if __name__ == '__main__':
I think pep8 requires 2 lines here.

Looks fine.  Sorry I forgot about you.

[Adrienne Walker]

Created attachment 67761 [details]
Patch

[Adrienne Walker]

(In reply to comment #23)
> Created an attachment (id=67761) [details]
> Patch

I've fixed the style issues listed above.

[Kenneth Russell]

Comment on attachment 67761 [details]
Patch

Verified that the style issues mentioned above are fixed. r=me

[WebKit Commit Bot]

Comment on attachment 67761 [details]
Patch

Rejecting patch 67761 from commit-queue.

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Last 500 characters of output:
MLInputElement.h
	M	WebCore/rendering/RenderFileUploadControl.cpp
	M	WebCore/css/CSSStyleSelector.cpp
r67653 = a6cb255b7102ae73f9141e4c7b36ead6f20c8af5 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
error: Untracked working tree file 'LayoutTests/platform/mac/fast/images/gif-large-checkerboard-expected.txt' would be overwritten by merge.
could not detach HEAD
rebase refs/remotes/trunk: command returned error: 1

Died at WebKitTools/Scripts/update-webkit line 129.

Full output: http://queues.webkit.org/results/4030036

[Eric Seidel (no email)]

(In reply to comment #26)
> (From update of attachment 67761 [details])
> Rejecting patch 67761 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
> Last 500 characters of output:
> MLInputElement.h
>     M    WebCore/rendering/RenderFileUploadControl.cpp
>     M    WebCore/css/CSSStyleSelector.cpp
> r67653 = a6cb255b7102ae73f9141e4c7b36ead6f20c8af5 (refs/remotes/trunk)
> First, rewinding head to replay your work on top of it...
> error: Untracked working tree file 'LayoutTests/platform/mac/fast/images/gif-large-checkerboard-expected.txt' would be overwritten by merge.
> could not detach HEAD
> rebase refs/remotes/trunk: command returned error: 1
> 
> Died at WebKitTools/Scripts/update-webkit line 129.
> 
> Full output: http://queues.webkit.org/results/4030036

That looks like a bug in the commit-queue.  It should have run git clean -f before this.  I suspect this came from our recent re-organization of code inside the queue.  @abarth.

[WebKit Commit Bot]

Comment on attachment 67761 [details]
Patch

Clearing flags on attachment: 67761

Committed r67669: <http://trac.webkit.org/changeset/67669>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.