Attachment 342129 Details for Bug 186045 – Patch

[patch] Patch

bug-186045-20180607084531.patch (text/plain), 19.64 KB, created by Frédéric Wang (:fredw) on 2018-06-06 23:45:33 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Frédéric Wang (:fredw)

Created: 2018-06-06 23:45:33 PDT

Size: 19.64 KB

patch

obsolete

>Subversion Revision: 232506
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 4e05bdd41cc474d54887466ffc73a675f206bde5..8b3611d252704c132ef3402e72e24455a32f8e26 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,46 @@
>+2018-06-06  Frederic Wang  <fwang@igalia.com>
>+
>+        webkit-test-runner: Add support for the reftest-wait class name
>+        https://bugs.webkit.org/show_bug.cgi?id=186045
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The WPT documentation provides a way for reftest authors to control "when comparison occurs",
>+        which is similar to testRunner's waitUntilDone / notifyDone() but using only public Web APIs.
>+        LayoutTests already contains some reftests that rely on this mechanism and more are likely
>+        to be added but they might be flacky without proper support for reftest-wait. Hence this
>+        commit emulates that support by injecting a helper script to reftest pages, calling
>+        waitUntilDone / notifyDone() according to when the 'load' event is fired and to whether the
>+        'reftest-wait' class name is present, as described in the following paragraph:
>+        https://web-platform-tests.org/writing-tests/reftests.html#controlling-when-comparison-occurs
>+
>+        * Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
>+        (SingleTestRunner._driver_input): Add a check_reftest_wait parameter, disabled by default
>+        and pass it to the constructor of DriverInput.
>+        (SingleTestRunner._run_reftest): For reftests, enable check_reftest_wait.
>+        * Scripts/webkitpy/port/driver.py:
>+        (DriverInput.__init__): Add should_check_reftest_wait option.
>+        (DriverInput.__repr__): Ditto.
>+        (Driver._command_from_driver_input): When should_check_reftest_wait is enabled, pass
>+        --check-reftest-wait to the command line. Note that for --pixel-test, the optional argument
>+        immediately following it is the image hash value, so we reorder a bit the command arguments
>+        to avoid misinterpretation of --check-reftest-wait or --dump-jsconsolelog-in-stderr.
>+        * WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:
>+        (WTR::InjectedBundle::didReceiveMessageToPage): Set m_checkReftestWait according to the
>+        message sent from the TestController.
>+        (WTR::InjectedBundle::beginTesting): When m_checkReftestWait is true, inject a JS script
>+        to support the "reftest-wait" class using testRunner's waitUntilDone / notifyDone().
>+        * WebKitTestRunner/InjectedBundle/InjectedBundle.h: Declare new member m_checkReftestWait.
>+        * WebKitTestRunner/TestController.cpp:
>+        (WTR::parseInputLine): Parse the --check-reftest-wait argument. As said above, this function
>+        assumes that --pixel-test has an optional pixel has argument immediately after.
>+        (WTR::TestController::runTest): Forward checkReftestWait option to TestInvocation.
>+        * WebKitTestRunner/TestController.h: Add checkReftestWait option to TestCommand list.
>+        * WebKitTestRunner/TestInvocation.cpp: Include checkReftestWait option to the message sent
>+        to the InjectedBundle.
>+        (WTR::TestInvocation::createTestSettingsDictionary):
>+        * WebKitTestRunner/TestInvocation.h: Add m_checkReftestWait member and setter.
>+
> 2018-06-04  Frederic Wang  <fwang@igalia.com>
> 
>         import-w3c-tests should rely on <meta name="flags"> to detect CSS manual tests
>diff --git a/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py b/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py
>index c8a119cb625d68b3e8f154cf186baf09b15ba1b4..e9d8edbefa85250c9d14a4de51e4d4195a564d00 100644
>--- a/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py
>+++ b/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py
>@@ -82,7 +82,7 @@ class SingleTestRunner(object):
>     def _should_fetch_expected_checksum(self):
>         return self._should_run_pixel_test and not (self._options.new_baseline or self._options.reset_results)
> 
>-    def _driver_input(self):
>+    def _driver_input(self, check_reftest_wait=None):
>         # The image hash is used to avoid doing an image dump if the
>         # checksums match, so it should be set to a blank value if we
>         # are generating a new baseline.  (Otherwise, an image from a
>@@ -90,7 +90,7 @@ class SingleTestRunner(object):
>         image_hash = None
>         if self._should_fetch_expected_checksum():
>             image_hash = self._port.expected_checksum(self._test_name)
>-        return DriverInput(self._test_name, self._timeout, image_hash, self._should_run_pixel_test, self._should_dump_jsconsolelog_in_stderr)
>+        return DriverInput(self._test_name, self._timeout, image_hash, self._should_run_pixel_test, self._should_dump_jsconsolelog_in_stderr, check_reftest_wait)
> 
>     def run(self):
>         if self._reference_files:
>@@ -290,7 +290,7 @@ class SingleTestRunner(object):
>         return failures
> 
>     def _run_reftest(self):
>-        test_output = self._driver.run_test(self._driver_input(), self._stop_when_done)
>+        test_output = self._driver.run_test(self._driver_input(check_reftest_wait=True), self._stop_when_done)
>         total_test_time = 0
>         reference_output = None
>         test_result = None
>diff --git a/Tools/Scripts/webkitpy/port/driver.py b/Tools/Scripts/webkitpy/port/driver.py
>index f1295caa927e0a0605bf3c7b5d52fc94743afec8..e662f9462d2e8680388e4b5820e20b070bb9db2a 100644
>--- a/Tools/Scripts/webkitpy/port/driver.py
>+++ b/Tools/Scripts/webkitpy/port/driver.py
>@@ -44,16 +44,17 @@ _log = logging.getLogger(__name__)
> 
> 
> class DriverInput(object):
>-    def __init__(self, test_name, timeout, image_hash, should_run_pixel_test, should_dump_jsconsolelog_in_stderr=None, args=None):
>+    def __init__(self, test_name, timeout, image_hash, should_run_pixel_test, should_dump_jsconsolelog_in_stderr=None, should_check_reftest_wait=None, args=None):
>         self.test_name = test_name
>         self.timeout = timeout  # in ms
>         self.image_hash = image_hash
>         self.should_run_pixel_test = should_run_pixel_test
>         self.should_dump_jsconsolelog_in_stderr = should_dump_jsconsolelog_in_stderr
>+        self.should_check_reftest_wait = should_check_reftest_wait
>         self.args = args or []
> 
>     def __repr__(self):
>-        return "DriverInput(test_name='{}', timeout={}, image_hash={}, should_run_pixel_test={}, should_dump_jsconsolelog_in_stderr={}'".format(self.test_name, self.timeout, self.image_hash, self.should_run_pixel_test, self.should_dump_jsconsolelog_in_stderr)
>+        return "DriverInput(test_name='{}', timeout={}, image_hash={}, should_run_pixel_test={}, should_dump_jsconsolelog_in_stderr={}, should_check_reftest_wait={}'".format(self.test_name, self.timeout, self.image_hash, self.should_run_pixel_test, self.should_dump_jsconsolelog_in_stderr, self.should_check_reftest_wait)
> 
> 
> class DriverOutput(object):
>@@ -509,10 +510,14 @@ class Driver(object):
>         # ' is the separator between arguments.
>         if self._port.supports_per_test_timeout():
>             command += "'--timeout'%s" % driver_input.timeout
>-        if driver_input.should_run_pixel_test:
>-            command += "'--pixel-test"
>         if driver_input.should_dump_jsconsolelog_in_stderr:
>             command += "'--dump-jsconsolelog-in-stderr"
>+        if driver_input.should_check_reftest_wait:
>+            command += "'--check-reftest-wait"
>+        # --pixel-test and the optional argument immediately after are put at the end, so that the
>+        # command line parsing function in TestController does not mess up with the other arguments.
>+        if driver_input.should_run_pixel_test:
>+            command += "'--pixel-test"
>         if driver_input.image_hash:
>             command += "'" + driver_input.image_hash
>         return command + "\n"
>diff --git a/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp b/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp
>index da162cbd05e4c8059a30d0da8c234986eead0f89..5317df007a29b2286492b0ded68feaa695dac484 100644
>--- a/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp
>+++ b/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp
>@@ -209,6 +209,9 @@ void InjectedBundle::didReceiveMessageToPage(WKBundlePageRef page, WKStringRef m
>         WKRetainPtr<WKStringRef> dumpJSConsoleLogInStdErrKey(AdoptWK, WKStringCreateWithUTF8CString("DumpJSConsoleLogInStdErr"));
>         m_dumpJSConsoleLogInStdErr = WKBooleanGetValue(static_cast<WKBooleanRef>(WKDictionaryGetItemForKey(messageBodyDictionary, dumpJSConsoleLogInStdErrKey.get())));
> 
>+        WKRetainPtr<WKStringRef> checkReftestWaitKey(AdoptWK, WKStringCreateWithUTF8CString("CheckReftestWait"));
>+        m_checkReftestWait = WKBooleanGetValue(static_cast<WKBooleanRef>(WKDictionaryGetItemForKey(messageBodyDictionary, checkReftestWaitKey.get())));
>+
>         WKRetainPtr<WKStringRef> ackMessageName(AdoptWK, WKStringCreateWithUTF8CString("Ack"));
>         WKRetainPtr<WKStringRef> ackMessageBody(AdoptWK, WKStringCreateWithUTF8CString("BeginTest"));
>         WKBundlePagePostMessage(page, ackMessageName.get(), ackMessageBody.get());
>@@ -395,6 +398,33 @@ bool InjectedBundle::booleanForKey(WKDictionaryRef dictionary, const char* key)
>     return WKBooleanGetValue(static_cast<WKBooleanRef>(value));
> }
> 
>+// Helper JS script to call waitUntilDone() when the 'reftest-wait' class is set on the root element
>+// and notifyDone() once it is removed and the load event has fired.
>+// See https://web-platform-tests.org/writing-tests/reftests.html#controlling-when-comparison-occurs
>+static const char* checkReftestWaitScript =
>+    "(() => {"
>+    "    function checkReftestWait()"
>+    "    {"
>+    "        if (document.documentElement.classList.contains('reftest-wait'))"
>+    "            testRunner.waitUntilDone();"
>+    "        else"
>+    "            testRunner.notifyDone();"
>+    "    };"
>+    "    function init() {"
>+    "        checkReftestWait();"
>+    "        (new MutationObserver((mutationsList) => {"
>+    "            for (var mutation of mutationsList) {"
>+    "               if (mutation.type == 'attributes' && mutation.attributeName == 'class')"
>+    "                   checkReftestWait();"
>+    "            }"
>+    "        })).observe(document.documentElement, { attributes: true });"
>+    "    }"
>+    "    if (document.readyState === 'complete')"
>+    "        init();"
>+    "    else"
>+    "        window.addEventListener('load', init);"
>+    "})();";
>+
> void InjectedBundle::beginTesting(WKDictionaryRef settings, BegingTestingMode testingMode)
> {
>     m_state = Testing;
>@@ -461,6 +491,9 @@ void InjectedBundle::beginTesting(WKDictionaryRef settings, BegingTestingMode te
>     // [WK2] REGRESSION(r128623): It made layout tests extremely slow
>     // https://bugs.webkit.org/show_bug.cgi?id=96862
>     // WKBundleSetDatabaseQuota(m_bundle, 5 * 1024 * 1024);
>+
>+    if (m_checkReftestWait)
>+        queueNonLoadingScript(WTR::toWK(checkReftestWaitScript).get());
> }
> 
> void InjectedBundle::done()
>diff --git a/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h b/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h
>index 7307b6c9ecde2063acee2a918d514b3ce4c44fe1..f32439622c98cfb9fa1987931dbe406cded09280 100644
>--- a/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h
>+++ b/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h
>@@ -193,6 +193,7 @@ private:
>     int m_timeout;
>     bool m_pixelResultIsPending { false };
>     bool m_dumpJSConsoleLogInStdErr { false };
>+    bool m_checkReftestWait { false };
> 
>     WKRetainPtr<WKDataRef> m_audioResult;
>     WKRetainPtr<WKImageRef> m_pixelResult;
>diff --git a/Tools/WebKitTestRunner/TestController.cpp b/Tools/WebKitTestRunner/TestController.cpp
>index 2ac9ec60a7588eca006ab73d1714a6e8b5ff4005..bc67679cffcf70c8285d3759bfc60edec269c043 100644
>--- a/Tools/WebKitTestRunner/TestController.cpp
>+++ b/Tools/WebKitTestRunner/TestController.cpp
>@@ -1234,6 +1234,8 @@ TestCommand parseInputLine(const std::string& inputLine)
>                 result.expectedPixelHash = tokenizer.next();
>         } else if (arg == std::string("--dump-jsconsolelog-in-stderr"))
>             result.dumpJSConsoleLogInStdErr = true;
>+        else if (arg == std::string("--check-reftest-wait"))
>+            result.checkReftestWait = true;
>         else if (arg == std::string("--absolutePath"))
>             result.absolutePath = tokenizer.next();
>         else
>@@ -1261,6 +1263,8 @@ bool TestController::runTest(const char* inputLine)
>     if (command.timeout > 0)
>         m_currentInvocation->setCustomTimeout(command.timeout);
>     m_currentInvocation->setDumpJSConsoleLogInStdErr(command.dumpJSConsoleLogInStdErr || options.dumpJSConsoleLogInStdErr);
>+    if (command.checkReftestWait)
>+        m_currentInvocation->setCheckReftestWait(command.checkReftestWait);
> 
>     platformWillRunTest(*m_currentInvocation);
> 
>diff --git a/Tools/WebKitTestRunner/TestController.h b/Tools/WebKitTestRunner/TestController.h
>index bc10f6cbd0cdf2a0717cb44caeffe80e4a98dcc2..6b4ab9af0b6de6f43c631a534941ee2d3079843a 100644
>--- a/Tools/WebKitTestRunner/TestController.h
>+++ b/Tools/WebKitTestRunner/TestController.h
>@@ -443,6 +443,7 @@ struct TestCommand {
>     std::string expectedPixelHash;
>     int timeout { 0 };
>     bool dumpJSConsoleLogInStdErr { false };
>+    bool checkReftestWait { false };
> };
> 
> } // namespace WTR
>diff --git a/Tools/WebKitTestRunner/TestInvocation.cpp b/Tools/WebKitTestRunner/TestInvocation.cpp
>index 94c8a2d967e5ddf0e82d554167583839d0e89621..29fff7c4c7a2bdab15ab407f13a864485113ca9a 100644
>--- a/Tools/WebKitTestRunner/TestInvocation.cpp
>+++ b/Tools/WebKitTestRunner/TestInvocation.cpp
>@@ -144,6 +144,10 @@ WKRetainPtr<WKMutableDictionaryRef> TestInvocation::createTestSettingsDictionary
>     WKRetainPtr<WKBooleanRef> dumpJSConsoleLogInStdErrValue = adoptWK(WKBooleanCreate(m_dumpJSConsoleLogInStdErr));
>     WKDictionarySetItem(beginTestMessageBody.get(), dumpJSConsoleLogInStdErrKey.get(), dumpJSConsoleLogInStdErrValue.get());
>     
>+    WKRetainPtr<WKStringRef> checkReftestWaitKey = adoptWK(WKStringCreateWithUTF8CString("CheckReftestWait"));
>+    WKRetainPtr<WKBooleanRef> checkReftestWaitValue = adoptWK(WKBooleanCreate(m_checkReftestWait));
>+    WKDictionarySetItem(beginTestMessageBody.get(), checkReftestWaitKey.get(), checkReftestWaitValue.get());
>+
>     return beginTestMessageBody;
> }
> 
>diff --git a/Tools/WebKitTestRunner/TestInvocation.h b/Tools/WebKitTestRunner/TestInvocation.h
>index 940b32495188344f6bc799f805e1936aaa633aa1..278b7215332f078be20571e6677f6d2f126b3d00 100644
>--- a/Tools/WebKitTestRunner/TestInvocation.h
>+++ b/Tools/WebKitTestRunner/TestInvocation.h
>@@ -53,6 +53,7 @@ public:
>     // Milliseconds
>     void setCustomTimeout(int duration) { m_timeout = duration; }
>     void setDumpJSConsoleLogInStdErr(bool value) { m_dumpJSConsoleLogInStdErr = value; }
>+    void setCheckReftestWait(bool value) { m_checkReftestWait = value; }
> 
>     // Seconds
>     double shortTimeout() const;
>@@ -111,6 +112,7 @@ private:
> 
>     int m_timeout { 0 };
>     bool m_dumpJSConsoleLogInStdErr { false };
>+    bool m_checkReftestWait { false };
> 
>     // Invocation state
>     bool m_startedTesting { false };
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 613e2d6a9f80da4714491a9c4ef1af97b670ca6c..84247a12059bf8e2fc06c6d2e8c829ac032133b8 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,22 @@
>+2018-06-06  Frederic Wang  <fwang@igalia.com>
>+
>+        webkit-test-runner: Add support for the reftest-wait class name
>+        https://bugs.webkit.org/show_bug.cgi?id=186045
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add new reftests to check support for the reftest-wait class name.
>+
>+        * testing/reftest-wait-events-expected.html: Added.
>+        * testing/reftest-wait-events.html: Added. This verifies the case when reftest-wait is added
>+        before the 'load' event is fired and removed after it is fired.
>+        * testing/reftest-wait-long-expected.html: Added.
>+        * testing/reftest-wait-long.html: Added. This verifies the case when reftest-wait is removed
>+        after a long wait.
>+        * testing/reftest-wait-other-class-names-expected.html: Added.
>+        * testing/reftest-wait-other-class-names.html: Added. This verifies the case when the root
>+        element has other names in its class list.
>+
> 2018-06-04  Chris Dumez  <cdumez@apple.com>
> 
>         Rename "Cross-Origin-Options" HTTP header to "Cross-Origin-Window-Policy"
>diff --git a/LayoutTests/testing/reftest-wait-events-expected.html b/LayoutTests/testing/reftest-wait-events-expected.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..869d941c3c4d211eeee7a2116fbb61bf9655bb50
>--- /dev/null
>+++ b/LayoutTests/testing/reftest-wait-events-expected.html
>@@ -0,0 +1,6 @@
>+<html>
>+<style>
>+:root {background-color:green}
>+</style>
>+<p>This test passes if background becomes green after page load.</p>
>+</html>
>diff --git a/LayoutTests/testing/reftest-wait-events.html b/LayoutTests/testing/reftest-wait-events.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..29774fe586ae8b6016db12c8819be539ac2a5f92
>--- /dev/null
>+++ b/LayoutTests/testing/reftest-wait-events.html
>@@ -0,0 +1,15 @@
>+<html>
>+<style>
>+:root {background-color:red}
>+</style>
>+<script>
>+window.addEventListener("DOMContentLoaded", () => {
>+    document.documentElement.className = "reftest-wait";
>+});
>+window.addEventListener("load", () => {
>+    document.documentElement.style.backgroundColor = "green";
>+    document.documentElement.className = "";
>+});
>+</script>
>+<p>This test passes if background becomes green after page load.</p>
>+</html>
>diff --git a/LayoutTests/testing/reftest-wait-long-expected.html b/LayoutTests/testing/reftest-wait-long-expected.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..24d62d2a1d675f2e68629d85d1cfe1a77aeed78c
>--- /dev/null
>+++ b/LayoutTests/testing/reftest-wait-long-expected.html
>@@ -0,0 +1,6 @@
>+<html>
>+<style>
>+:root {background-color:green}
>+</style>
>+<p>This test passes if background becomes green after three seconds.</p>
>+</html>
>diff --git a/LayoutTests/testing/reftest-wait-long.html b/LayoutTests/testing/reftest-wait-long.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..b509b212c79d5b7e33923265e063e48ca5f76854
>--- /dev/null
>+++ b/LayoutTests/testing/reftest-wait-long.html
>@@ -0,0 +1,12 @@
>+<html class="reftest-wait">
>+<style>
>+:root {background-color:red}
>+</style>
>+<script>
>+window.setTimeout(() => {
>+  document.documentElement.style.backgroundColor = "green";
>+  document.documentElement.className = "";
>+}, 3000);
>+</script>
>+<p>This test passes if background becomes green after three seconds.</p>
>+</html>
>diff --git a/LayoutTests/testing/reftest-wait-other-class-names-expected.html b/LayoutTests/testing/reftest-wait-other-class-names-expected.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..0649ee7633f0e9a0ca437a9684a5440030f03b2c
>--- /dev/null
>+++ b/LayoutTests/testing/reftest-wait-other-class-names-expected.html
>@@ -0,0 +1,6 @@
>+<html class="other-class-name-1 other-class-name-2">
>+<style>
>+:root {background-color:green}
>+</style>
>+<p>This test passes if background becomes green after one second.</p>
>+</html>
>diff --git a/LayoutTests/testing/reftest-wait-other-class-names.html b/LayoutTests/testing/reftest-wait-other-class-names.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..3c17bbc792d18364a707d7b5beec26e3fd3a547e
>--- /dev/null
>+++ b/LayoutTests/testing/reftest-wait-other-class-names.html
>@@ -0,0 +1,12 @@
>+<html class="other-class-name-1 reftest-wait other-class-name-2">
>+<style>
>+:root {background-color:red}
>+</style>
>+<script>
>+window.setTimeout(() => {
>+  document.documentElement.style.backgroundColor = "green";
>+  document.documentElement.classList.remove("reftest-wait");
>+}, 1000);
>+</script>
>+<p>This test passes if background becomes green after one second.</p>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 186045: 341479 | 341562 | 342053 | 342129 | 342130 | 342134 | 342135 | 342137 | 342140 | 342141 | 342142 | 342143 | 342144 | 342145 | 342146 | 342154 | 342158 | 342193 | 408200 | 408201 | 408361 | 408385