Changeset 285132 in webkit

Timestamp:

Nov 1, 2021, 2:30:38 PM (5 years ago)

Author:

timothy_horton@apple.com

Message:

Add a run-webkit-tests mode to A/B test a given feature
​https://bugs.webkit.org/show_bug.cgi?id=232553

Reviewed by Jonathan Bedard.

Add the argument --self-compare-with-header to run-webkit-tests, which
can be used to test the impact of a given feature (or set of features;
it accepts the standard test features header format).

When tests are run in this mode, all tests are run in the ref-test
style, but with the expected and actual results loading the same
test file (ignoring the usual -expected.html or whatever); they differ
only in the set of features/preferences enabled.

This is especially useful for testing the impact of e.g. platform
graphics features, where the difference between the shipping behavior
and in-development behavior is more interesting than whether or not
it actually makes the tests, as written, fail.

• Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:

(SingleTestRunner):
(SingleTestRunner._run_comparison_test):
Add the comparison test runner, and prefer it if requested.

One note here: the run with the options derived from the given header is
considered the "actual" result and the default configuration the "expected".

• Scripts/webkitpy/layout_tests/run_webkit_tests.py:

(parse_args):

• Scripts/webkitpy/port/driver.py:

(DriverInput.init):
(DriverInput.repr):
(Driver._command_from_driver_input):
Pass the comparison test header along to the test runner.

Also, fix a longstanding error where --dump-jsconsolelog-in-stderr
could get inserted immediately after --pixel-test, causing the test runner
to consume it as the expected image hash! And leave a comment so nobody
else has to debug this again...

• TestRunnerShared/TestCommand.cpp:

(WTR::parseInputLine):

• TestRunnerShared/TestCommand.h:
• TestRunnerShared/TestFeatures.cpp:

(WTR::parseTestHeaderString):
(WTR::parseTestHeader):
(WTR::featureDefaultsFromComparisonTestHeader):
Factor out the parsing of the part of the test header inside the [ ],
since we use this format for the value of --self-compare-with-header as well.

• TestRunnerShared/TestFeatures.h:
• WebKitTestRunner/Options.h:
• WebKitTestRunner/TestController.cpp:

(WTR::TestController::testOptionsForTest const):
Merge the comparison header's options in to the test options before
the test's own header, so that the comparison header wins.

Location:

trunk/Tools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/layout_tests/controllers/single_test_runner.py (modified) (2 diffs)
• Scripts/webkitpy/layout_tests/run_webkit_tests.py (modified) (1 diff)
• Scripts/webkitpy/port/driver.py (modified) (3 diffs)
• TestRunnerShared/TestCommand.cpp (modified) (1 diff)
• TestRunnerShared/TestCommand.h (modified) (1 diff)
• TestRunnerShared/TestFeatures.cpp (modified) (3 diffs)
• TestRunnerShared/TestFeatures.h (modified) (1 diff)
• WebKitTestRunner/TestController.cpp (modified) (1 diff)

trunk/Tools/ChangeLog

@@ +1 @@
+2021-11-01  Tim Horton  <timothy_horton@apple.com>
+
+        Add a run-webkit-tests mode to A/B test a given feature
+        https://bugs.webkit.org/show_bug.cgi?id=232553
+
+        Reviewed by Jonathan Bedard.
+
+        Add the argument --self-compare-with-header to run-webkit-tests, which
+        can be used to test the impact of a given feature (or set of features;
+        it accepts the standard test features header format).
+
+        When tests are run in this mode, all tests are run in the ref-test
+        style, but with the `expected` and `actual` results loading the same
+        test file (ignoring the usual -expected.html or whatever); they differ
+        only in the set of features/preferences enabled.
+
+        This is especially useful for testing the impact of e.g. platform
+        graphics features, where the difference between the shipping behavior
+        and in-development behavior is more interesting than whether or not
+        it actually makes the tests, as written, fail.
+
+        * Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
+        (SingleTestRunner):
+        (SingleTestRunner._run_comparison_test):
+        Add the comparison test runner, and prefer it if requested.
+
+        One note here: the run with the options derived from the given header is
+        considered the "actual" result and the default configuration the "expected".
+
+        * Scripts/webkitpy/layout_tests/run_webkit_tests.py:
+        (parse_args):
+        * Scripts/webkitpy/port/driver.py:
+        (DriverInput.__init__):
+        (DriverInput.__repr__):
+        (Driver._command_from_driver_input):
+        Pass the comparison test header along to the test runner.
+
+        Also, fix a longstanding error where --dump-jsconsolelog-in-stderr
+        could get inserted immediately after --pixel-test, causing the test runner
+        to consume it as the expected image hash! And leave a comment so nobody
+        else has to debug this again...
+
+        * TestRunnerShared/TestCommand.cpp:
+        (WTR::parseInputLine):
+        * TestRunnerShared/TestCommand.h:
+        * TestRunnerShared/TestFeatures.cpp:
+        (WTR::parseTestHeaderString):
+        (WTR::parseTestHeader):
+        (WTR::featureDefaultsFromComparisonTestHeader):
+        Factor out the parsing of the part of the test header inside the [ ],
+        since we use this format for the value of --self-compare-with-header as well.
+
+        * TestRunnerShared/TestFeatures.h:
+        * WebKitTestRunner/Options.h:
+        * WebKitTestRunner/TestController.cpp:
+        (WTR::TestController::testOptionsForTest const):
+        Merge the comparison header's options in to the test options before
+        the test's own header, so that the comparison header wins.
+
 2021-11-01  Jonathan Bedard  <jbedard@apple.com>
 

trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py

@@ -112 +112 @@
 
     def run(self):
+        self_comparison_header = self._port.get_option('self_compare_with_header')
+        if self_comparison_header:
+            return self._run_self_comparison_test(self_comparison_header)
         if self._reference_files:
             if self._port.get_option('no_ref_tests') or self._options.reset_results:
@@ -337 +340 @@
         return TestResult(self._test_input, test_result.failures, total_test_time + test_result.test_run_time, test_result.has_stderr, reftest_type=reftest_type, pid=test_result.pid, references=reference_test_names)
 
+    def _run_self_comparison_test(self, header):
+        driver_input = self._driver_input()
+        driver_input.should_run_pixel_test = True
+
+        reference_output = self._driver.run_test(driver_input, self._stop_when_done)
+        driver_input.self_comparison_header = header
+        test_output = self._driver.run_test(driver_input, self._stop_when_done)
+
+        test_full_path = self._port.abspath_for_test(self._test_name)
+        test_result = self._compare_output_with_reference(reference_output, test_output, test_full_path, False)
+
+        assert(reference_output)
+        test_result_writer.write_test_result(self._filesystem, self._port, self._results_directory, self._test_name, test_output, reference_output, test_result.failures)
+        return TestResult(self._test_input, test_result.failures, test_result.test_run_time, test_result.has_stderr, pid=test_result.pid)
+
     @staticmethod
     def _relative_reference_path(test_full_path, reference_full_path):

trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py

@@ -344 +344 @@
             "--prefer-integrated-gpu", action="store_true", default=False,
             help=("Prefer using the lower-power integrated GPU on a dual-GPU system. Note that other running applications and the tests themselves can override this request.")),
-        optparse.make_option('--show-window', action="store_true", default=False, help="Make the test runner window visible during testing."),
+        optparse.make_option("--show-window", action="store_true", default=False, help="Make the test runner window visible during testing."),
+        optparse.make_option("--self-compare-with-header", help="Run all tests as A/B tests between the default configuration and the given test features header (ignoring expected results)."),
     ]))
 

trunk/Tools/Scripts/webkitpy/port/driver.py

@@ -47 +47 @@
 
 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, args=None, self_comparison_header=None):
         self.test_name = test_name
         self.timeout = timeout  # in ms
@@ -54 +54 @@
         self.should_dump_jsconsolelog_in_stderr = should_dump_jsconsolelog_in_stderr
         self.args = args or []
+        self.self_comparison_header = self_comparison_header
 
     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={}, self_comparison_header={}'".format(self.test_name, self.timeout, self.image_hash, self.should_run_pixel_test, self.should_dump_jsconsolelog_in_stderr, self.self_comparison_header)
 
 
@@ -632 +633 @@
         if self._port.supports_per_test_timeout():
             command += "'--timeout'%s" % driver_input.timeout
+        if driver_input.should_dump_jsconsolelog_in_stderr:
+            command += "'--dump-jsconsolelog-in-stderr"
+        if driver_input.self_comparison_header:
+            command += "'--self-compare-with-header'%s" % driver_input.self_comparison_header
+
+        # --pixel-test must be the last argument, because the hash is optional,
+        # and any argument put in its place will be incorrectly consumed as the hash.
         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.image_hash:
-            command += "'" + driver_input.image_hash
+            if driver_input.image_hash:
+                command += "'" + driver_input.image_hash
         return command + "\n"
 

trunk/Tools/TestRunnerShared/TestCommand.cpp

@@ -99 +99 @@
             if (tokenizer.hasNext())
                 result.expectedPixelHash = tokenizer.next();
+        } else if (arg == "--self-compare-with-header") {
+            if (tokenizer.hasNext())
+                result.selfComparisonHeader = tokenizer.next();
+            else
+                die(inputLine);
         } else if (arg == std::string("--dump-jsconsolelog-in-stderr"))
             result.dumpJSConsoleLogInStdErr = true;

trunk/Tools/TestRunnerShared/TestCommand.h

@@ -36 +36 @@
     std::filesystem::path absolutePath;
     std::string expectedPixelHash;
+    std::string selfComparisonHeader;
     WTF::Seconds timeout;
     bool shouldDumpPixels { false };

trunk/Tools/TestRunnerShared/TestFeatures.cpp

@@ -244 +244 @@
 }
 
-static TestFeatures parseTestHeader(std::filesystem::path path, const std::unordered_map<std::string, TestHeaderKeyType>& keyTypeMap)
+static TestFeatures parseTestHeaderString(const std::string& pairString, std::filesystem::path path, const std::unordered_map<std::string, TestHeaderKeyType>& keyTypeMap)
 {
     TestFeatures features;
-    std::error_code ec;
-    if (!std::filesystem::exists(path, ec))
-        return features;
-
-    std::ifstream file(path);
-    if (!file.good()) {
-        LOG_ERROR("Could not open file to inspect test headers in %s", path.c_str());
-        return features;
-    }
-
-    std::string options;
-    getline(file, options);
-    std::string beginString("webkit-test-runner [ ");
-    std::string endString(" ]");
-    size_t beginLocation = options.find(beginString);
-    if (beginLocation == std::string::npos)
-        return features;
-    size_t endLocation = options.find(endString, beginLocation);
-    if (endLocation == std::string::npos) {
-        LOG_ERROR("Could not find end of test header in %s", path.c_str());
-        return features;
-    }
-    std::string pairString = options.substr(beginLocation + beginString.size(), endLocation - (beginLocation + beginString.size()));
+
     size_t pairStart = 0;
     while (pairStart < pairString.size()) {
@@ -292 +270 @@
 }
 
+static TestFeatures parseTestHeader(std::filesystem::path path, const std::unordered_map<std::string, TestHeaderKeyType>& keyTypeMap)
+{
+    std::error_code ec;
+    if (!std::filesystem::exists(path, ec))
+        return { };
+
+    std::ifstream file(path);
+    if (!file.good()) {
+        LOG_ERROR("Could not open file to inspect test headers in %s", path.c_str());
+        return { };
+    }
+
+    std::string options;
+    getline(file, options);
+    std::string beginString("webkit-test-runner [ ");
+    std::string endString(" ]");
+    size_t beginLocation = options.find(beginString);
+    if (beginLocation == std::string::npos)
+        return { };
+    size_t endLocation = options.find(endString, beginLocation);
+    if (endLocation == std::string::npos) {
+        LOG_ERROR("Could not find end of test header in %s", path.c_str());
+        return { };
+    }
+    std::string pairString = options.substr(beginLocation + beginString.size(), endLocation - (beginLocation + beginString.size()));
+    return parseTestHeaderString(pairString, path, keyTypeMap);
+}
+
 TestFeatures featureDefaultsFromTestHeaderForTest(const TestCommand& command, const std::unordered_map<std::string, TestHeaderKeyType>& keyTypeMap)
 {
@@ -297 +303 @@
 }
 
-}
+TestFeatures featureDefaultsFromSelfComparisonHeader(const TestCommand& command, const std::unordered_map<std::string, TestHeaderKeyType>& keyTypeMap)
+{
+    if (command.selfComparisonHeader.empty())
+        return { };
+    return parseTestHeaderString(command.selfComparisonHeader, command.absolutePath, keyTypeMap);
+}
+
+} // namespace WTF

trunk/Tools/TestRunnerShared/TestFeatures.h

@@ -70 +70 @@
 };
 TestFeatures featureDefaultsFromTestHeaderForTest(const TestCommand&, const std::unordered_map<std::string, TestHeaderKeyType>&);
+TestFeatures featureDefaultsFromSelfComparisonHeader(const TestCommand&, const std::unordered_map<std::string, TestHeaderKeyType>&);
 
 }

trunk/Tools/WebKitTestRunner/TestController.cpp

@@ -1312 +1312 @@
     merge(features, hardcodedFeaturesBasedOnPathForTest(command));
     merge(features, platformSpecificFeatureDefaultsForTest(command));
+    merge(features, featureDefaultsFromSelfComparisonHeader(command, TestOptions::keyTypeMapping()));
     merge(features, featureDefaultsFromTestHeaderForTest(command, TestOptions::keyTypeMapping()));
     merge(features, platformSpecificFeatureOverridesDefaultsForTest(command));