[Webkit-unassigned] [Bug 162458] EWS should run JavaScriptCore tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=162458

Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #301667|review?                     |review-
              Flags|                            |

--- Comment #55 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 301667
  --> https://bugs.webkit.org/attachment.cgi?id=301667
Patch

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

> > Tools/Scripts/webkitpy/common/config/ports.py:117
> > +        command.append("--json-output=jsc_test_results.json")
>
> Can we write this as two arguments?

There are other instances of passing arguments with '=' sign in this file, so I'm not sure if/why splitting is preferable.

I feel like AbstractTestResults is getting too skinny and may not be useful. But we can improve this in the future. We'll have a better idea of how the interface should look when working other test types, such as webkitpy.

I think that the next iteration should be ready to get an r+, looks pretty good to me.

> Tools/ChangeLog:39
> +        (JSCTestResults.failing_test_results): Getter. Some of these exist for interopability reasons.

I believe that you got rid of this after Dan's review.

> Tools/QueueStatusServer/handlers/statusbubble.py:194
> +        return bool(status and not status.did_skip())

This is a good pattern to use, but we should migrate other code to it (in a separate patch). Code above just compares to "Pass" and "Fail", so we are being inconsistent.

OK as is for now.

> Tools/Scripts/webkitpy/common/config/ews.json:54
> +    "JSC EWS": {

We need to update status server to not change the case to "Jsc EWS". There is at least one instance of that on your test server. There is code that special cases some strings such as "EWS" for proper display.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:33
> +        self.all_api_tests_passed = all_api_tests_passed

all_api_tests_passed isn't used outside the class, so it should be prefixed with an underscore.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:34
> +        self.stress_test_failures = stress_test_failures

Ditto.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:36
> +        self.results = stress_test_failures[:]

Ditto.

But also, a better name would help here. This is a list of failing test names, so let's just call it _failing_test_names.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:76
> +    def test_results(self):
> +        return self.results
> +
> +    def failing_tests(self):
> +        return self.results

This can't be right. For LayoutTests, one function returns a list of test names, and another returns a list of TestResult objects. Making it so different for JSC tests is very confusing.

It seems like we can just remove test_results() from the interface, and have JSC code call failing_tests() where it now calls test_results().

> Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:38
> +        except (IOError, KeyError):  # File does not exist or can't be read.

This comment doesn't seem useful.

> Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:41
> +    def _create_jsc_test_results(self):

I don't think that extracting this into function helps, please inline it at call site.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:253
> +        consistent_results = JSCTestResults.intersection(first_results, second_results)

I think that this should be "consistently_failing_test_results"

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:339
> +        if hasattr(self._delegate, 'group') and self._delegate.group() == "jsc":

This is because commit queue doesn't have a group. This makes me wonder what to do with it - one might expect it to run JSC tests now.

Not something to fix right now.

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:95
> +class MockEarlyWarningSystemTaskForInconclusiveResults(EarlyWarningSystemTask):
> +    def _test_patch(self):
> +        self._test()
> +        results = self._delegate.test_results()
> +        return bool(results)
> +
> +
> +class MockAbstractEarlyWarningSystemForInconclusiveResults(AbstractEarlyWarningSystem):
> +    def _create_task(self, patch):
> +        task = MockEarlyWarningSystemTaskForInconclusiveResults(self, patch, self._options.run_tests)
> +        return task

These classes are only used for JSC, so we should have that in the name - otherwise it's difficult to tell which delegate is being invoked.

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:148
> +    # This must use the Mock EWS classes defined above

This comment basically says that one of the lines below is important. That's not useful - all of them are important, and if someone accidentally removes one, tests will catch that anyway.

> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:71
> +            _log.info("This patch has relevant changes.")

I think that this would be too noisy. Processing status lines relies on there not being too many. Please remove this.

Logging just for the other case is sufficient.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170216/6fc2cc3f/attachment-0001.html>