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

[bugzilla-daemon at webkit.org]

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

--- Comment #37 from Sri Vijayaraghavan <svijayaraghavan at apple.com> ---
(In reply to comment #36)
> Comment on attachment 301069 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301069&action=review
> 
> > Tools/Scripts/webkitpy/common/net/jsctestresults.py:32
> > +        intersection_api_tests_passed = first._all_api_tests_passed or second._all_api_tests_passed
> 
> please verify whether 'or' or 'and' should be used for intersection.
I know it looks funny, but it should be 'or'. If there are some API test failures the first time, but they all passed the second time, those failures are flaky results, so the reported value of api_tests_passed should be True. There are some examples in the unittests at webkitpy/common/net/jsctestresults_unittest.py.

> > Tools/Scripts/webkitpy/common/net/jsctestresults.py:44
> > +            return None
> 
> might be a good idea to log a warning as well.
I'll do this.

> > Tools/Scripts/webkitpy/common/net/jsctestresults_unittest.py:31
> > +        empty_string = ""
> 
> might consider renaming empty_string to empty_json to make it more
> consistent.
I'll do this.

> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:45
> > +        self.failure_status_id = 42
> 
> any specific reason to choose 42? would it make more sense to keep failure
> id as -1?
I'll change it to zero. This becomes a part of the URL to the test results, so it can't be -1.

> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:52
> > +        else:
> 
> 'else' is unnecessary since if returns a value.
I'll fix this.

> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:99
> > +        self.assertEqual(return_value, True)
> 
> should use assertTrue(return_value)
> 
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:119
> > +        self.assertEqual(return_value, True)
> 
> same, should use assertTrue(return_value)
> 
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:129
> > +        self.assertEqual(return_value, True)
> 
> same, should use assertTrue(return_value)
> 
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:139
> > +        self.assertEqual(return_value, True)
> 
> same, should use assertTrue(return_value)
> 
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:159
> > +        self.assertEqual(return_value, False)
> 
> same, should use assertFalse(return_value)
> 
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:169
> > +        self.assertEqual(return_value, False)
> 
> same, should use assertFalse(return_value)
> 
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:179
> > +        self.assertEqual(return_value, False)
> 
> same, should use assertFalse(return_value)
> 
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:188
> > +        self.assertEqual(return_value, True)
> 
> same, should use assertTrue(return_value)
> 
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:198
> > +        self.assertEqual(return_value, True)
> 
> same, should use assertTrue(return_value)
>
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:218
> > +        self.assertEqual(return_value, True)
> 
> same, should use assertTrue(return_value)
I'll fix this set of items.

> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:209
> > +        self.assertEqual(task.test_run_count(), 3)
> 
> don't we need to assert the return_value here?
There should be no return value in this case, since we expect the function call to raise an exception and not return. For this case, I'll change "return_value = task._test_patch()" to simply "task._test_patch()" since return_value shouldn't be set anyway.

> > Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:40
> > +    jsc_paths = [
> 
> can consider renaming it to jsc_relevant_paths
I'll do this.

> > Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:47
> > +    def _does_contain_change_in_paths(self, files, patterns):
> 
> _does_contain_change_in_paths is  misleading name (we are not searching for
> change in path here)
I'll rename the method.

> > Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:48
> > +        for item in files:
> 
> renaming item to file would make it more readable.
I'll do this.

-- 
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/20170210/ad057bd6/attachment.html>