Bug 168994 - Bots should run the dashboard tests
Summary: Bots should run the dashboard tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-28 15:12 PST by Jason Marcell
Modified: 2017-03-29 09:14 PDT (History)
12 users (show)

See Also:


Attachments
Patch (25.90 KB, patch)
2017-02-28 15:20 PST, Jason Marcell
no flags Details | Formatted Diff | Diff
Patch (25.92 KB, patch)
2017-02-28 15:22 PST, Jason Marcell
no flags Details | Formatted Diff | Diff
Patch (26.59 KB, patch)
2017-03-20 17:04 PDT, Jason Marcell
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Marcell 2017-02-28 15:12:44 PST
Bots should run the dashboard tests
Comment 1 Jason Marcell 2017-02-28 15:20:14 PST
Created attachment 302997 [details]
Patch
Comment 2 Jason Marcell 2017-02-28 15:22:19 PST
Created attachment 302999 [details]
Patch
Comment 3 WebKit Commit Bot 2017-02-28 15:24:35 PST
Attachment 302999 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:368:  whitespace before ':'  [pep8/E203] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Jason Marcell 2017-02-28 15:26:02 PST
Please note, the `whitespace before ':'  [pep8/E203]` style error was submitted intentionally in order to maintain consistency with the style that was already present and pervasive in `mastercfg_unittest.py`.
Comment 5 Alexey Proskuryakov 2017-02-28 15:28:23 PST
Comment on attachment 302999 [details]
Patch

Looks good to me. Since this is mostly buildbot config, I'd like Lucas to review too.

The dashboard needs more modifications to actually have a link to test results when dashboard test fail. I do not think that this will happen automatically.
Comment 6 Daniel Bates 2017-02-28 19:53:43 PST
Comment on attachment 302999 [details]
Patch

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

If we chose to run the dashboard test separate from the rest of the Webkit tests then we should upload the dashboard test results to the Buildbot master and generate a link to the results. This output is easy to human friendly and makes it straightforward to understand the reason for the test failure. To be clear, the generated hyperlink I am referring to is the "view results" hyperlink under the step uploaded results on a Buildbot build page. You can see an example of this at <https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK1%20%28Tests%29/builds/14159>. To do this, we will likely need to modify or mimic the steps ArchiveTestResults, UploadTestResults, and ExtractTestResults.

> Tools/ChangeLog:9
> +        We pull the `--results-directory` out so that `RunDashboardTests` can override it. This way the dashboard tests results won't
> +        overwrite the regular layout test results or get mixed in with them.

It seems reasonable to separate the dashboard tests from the WebKit test. Out of curiosity, would it be terrible if they are mixed in? One benefit is that we can use the existing machinery to upload the test results and link to them.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:346
> +        self.setCommand(self.command + self._results_directory_arguments())

This is OK as-is. We can simplify this patch and avoid the need to add the function _results_directory_arguments() and override it by making use of class variables. I would define a class variable called resultDirectory := "layout-test-results" in class RunWebKitTests and override this class variable in the derived class (as we do for descriptionDone) to be "dashboard-layout-test-results". Then we can update this line to read:

self.setCommand(self.command +["--results-directory", self.resultDirectory])

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:362
> +    def _results_directory_arguments(self):
> +        return ["--results-directory", "layout-test-results"]
> +
>      def _strip_python_logging_prefix(self, line):

We can avoid the need to add this function by making use of a class variable. See my remark above.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:438
> +        self.setCommand(self.command + ['--layout-tests-directory', './Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests'])

I believe we prefer double quoted string literals for OpenSource Buildbot code though it seems we use both :( What style is used in the majority of this code? We should use consistent style.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:442
> +    def _results_directory_arguments(self):
> +        return ["--results-directory", "dashboard-layout-test-results"]

Ditto.
Comment 7 Jason Marcell 2017-03-20 17:04:53 PDT
Created attachment 304968 [details]
Patch
Comment 8 WebKit Commit Bot 2017-03-20 17:07:38 PDT
Attachment 304968 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:368:  whitespace before ':'  [pep8/E203] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 David Kilzer (:ddkilzer) 2017-03-27 11:54:50 PDT
Comment on attachment 304968 [details]
Patch

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

r=me

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:329
> +    resultDirectory = "layout-test-results"

Nit: Would prefer this to be named 'resultsDirectory' (plural 'results') to match the command-line switch.
Comment 10 Jason Marcell 2017-03-27 14:47:01 PDT
Comment on attachment 304968 [details]
Patch

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

>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:329
>> +    resultDirectory = "layout-test-results"
> 
> Nit: Would prefer this to be named 'resultsDirectory' (plural 'results') to match the command-line switch.

I agree and I was going to take your suggestion but it seems that there's already a pattern within this file of 'resultsDirectory'.
Comment 11 Alexey Proskuryakov 2017-03-27 17:35:49 PDT
Comment on attachment 304968 [details]
Patch

Looks like this was landed in https://trac.webkit.org/r214434
Comment 12 Alexey Proskuryakov 2017-03-27 18:35:40 PDT
There are unfortunately several places where "layout-test" is hardcoded. Currently, this is making El Capitan test results yellow, because dashboard tests fail there.

I'm not sure if this is easy to refactor, of if we need to special case "dashboard-tests" too.
Comment 13 Alexey Proskuryakov 2017-03-27 20:25:48 PDT
Filed bug 170158.
Comment 14 Alexey Proskuryakov 2017-03-29 09:00:52 PDT
Jason, is it correct that this bug is still open?
Comment 15 Jason Marcell 2017-03-29 09:14:49 PDT
Nope! I will close. Thanks for pointing it out.