Bug 168994 – Bots should run the dashboard tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jason Marcell]

Bots should run the dashboard tests

[Jason Marcell]

Created attachment 302997 [details]
Patch

[Jason Marcell]

Created attachment 302999 [details]
Patch

[WebKit Commit Bot]

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.

[Jason Marcell]

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

[Alexey Proskuryakov]

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.

[Daniel Bates]

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.

[Jason Marcell]

Created attachment 304968 [details]
Patch

[WebKit Commit Bot]

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.

[David Kilzer (:ddkilzer)]

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.

[Jason Marcell]

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

[Alexey Proskuryakov]

Comment on attachment 304968 [details]
Patch

Looks like this was landed in https://trac.webkit.org/r214434

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Filed bug 170158.

[Alexey Proskuryakov]

Jason, is it correct that this bug is still open?

[Jason Marcell]

Nope! I will close. Thanks for pointing it out.