EWS should run JavaScriptCore tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Srinivasan Vijayaraghavan]

Make changes to webkitpy to enable JSC EWS.

[Srinivasan Vijayaraghavan]

Created attachment 297210 [details]
Patch

[Srinivasan Vijayaraghavan]

(In reply to comment #1)
> Created attachment 297210 [details]
> Patch

Uploaded before putting in a ChangeLog - sorry!

Todos:
- patchanalysistask.py: Use hasattr() instead of try/except
- Regression tests (on the way!)

[Srinivasan Vijayaraghavan]

Created attachment 297239 [details]
Patch

[WebKit Commit Bot]

Attachment 297239 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:137:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 297551 [details]
Patch

[WebKit Commit Bot]

Attachment 297551 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:138:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Aakash Jain]

Comment on attachment 297551 [details]
Patch

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

overall looks fine, couple of comments below inline. should add tests.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:34
> +        parsed_results = json.loads(string)

is string guaranteed to be a valid json? if not use try/except accordingly.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:36
> +        return cls(parsed_results['allApiTestsPassed'], parsed_results['stressTestFailures'])

can consider checking if parsed_results contains 'allApiTestsPassed' and 'stressTestFailures'.

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

does test_results variable indicate test results or test failures? maybe consider renaming it

> Tools/Scripts/webkitpy/common/net/jsctestresults_unittest.py:34
> +        api_test_failures_string = '{"allApiTestsPassed": false, "stressTestFailures":[]}'

can consider making it a separate unit-test for better readability.

> Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:25
> +from webkitpy.common.net.jsctestresults import JSCTestResults

probably need to add a newline before 'from'. check with other files in webkitpy to confirm.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:135
> +            args.append("--group=%s" % self._delegate.group())

instead of try/finally, it would be better to use something like hasattr(self._delegate, 'group') to check if group is present and then append to args accordingly.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:151
> +            args.append("--group=%s" % self._delegate.group())

instead of try/finally, it would be better to use something like hasattr(self._delegate, 'group') to check if group is present and then append to args accordingly.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:170
> +            args.append("--group=%s" % self._delegate.group())

instead of try/finally, it would be better to use something like hasattr(self._delegate, 'group') to check if group is present and then append to args accordingly.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:189
> +            args.append("--group=%s" % self._delegate.group())

instead of try/finally, it would be better to use something like hasattr(self._delegate, 'group') to check if group is present and then append to args accordingly.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:243
> +            else:

'else' keyword is not required here since if statement has a return.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:244
> +                # TODO [e-mail]: Comment this, and uncomment the below lines so it actually builds without patch

please fix the TODO.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:255
>  

Question: Do we need to check for flakiness of the tests similar to layout tests, and report to flakiness dashboard (if dashboard supports these results)? Maybe something to consider for future.

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:61
> +            dir = os.path.dirname(os.path.dirname(os.path.dirname(self._port.results_directory())))

dir should be renamed to more readable name to indicate whether it is for results or logs. Also we try to avoid abbreviations in variable names, see: https://webkit.org/code-style-guidelines/#names-full-words

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:139
> +        else:

'else' can be removed.

> Tools/Scripts/webkitpy/tool/steps/build.py:73
> +            group = None

can consider moving this line above 'if' statement and removing 'else'

[Srinivasan Vijayaraghavan]

Created attachment 298160 [details]
Patch

[Srinivasan Vijayaraghavan]

(In reply to comment #7)
> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:244
> > +                # TODO [e-mail]: Comment this, and uncomment the below lines so it actually builds without patch
> 
> please fix the TODO.
Just to clarify - the item in the TODO was already done, I just forgot to take out the TODO comment.

[WebKit Commit Bot]

Attachment 298160 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:138:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 298714 [details]
Patch

[Srinivasan Vijayaraghavan]

Created attachment 298739 [details]
Patch

[WebKit Commit Bot]

Attachment 298739 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:137:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 298802 [details]
Patch

[WebKit Commit Bot]

Attachment 298802 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:137:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 298807 [details]
Patch

[WebKit Commit Bot]

Attachment 298807 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:137:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 300176 [details]
Patch

[WebKit Commit Bot]

Attachment 300176 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:140:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 300294 [details]
Patch

[WebKit Commit Bot]

Attachment 300294 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:140:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 300461 [details]
Patch

[WebKit Commit Bot]

Attachment 300461 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:140:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 300474 [details]
Patch

[WebKit Commit Bot]

Attachment 300474 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:140:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 300784 [details]
Patch

[WebKit Commit Bot]

Attachment 300784 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:141:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 300838 [details]
Patch

[WebKit Commit Bot]

Attachment 300838 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:141:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 301020 [details]
Patch

[WebKit Commit Bot]

Attachment 301020 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:145:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 301069 [details]
Patch

[WebKit Commit Bot]

Attachment 301069 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:145:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Johnson]

Comment on attachment 301069 [details]
Patch

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

As a heads up, I didn't do much reviewing of the unit tests. Really happy to see the labor put towards this come to fruition! :)

> Tools/Scripts/webkitpy/common/config/ports.py:167
> +        if (test_suite == "jsc"):

Nit: No need for parens.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:51
> +    def __init__(self, all_api_tests_passed, stress_test_failures):

Nit: __init__ should always be the first method defined for a class when not inherited.

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

This should be renamed to self.test_results since it will be accessed outside of the class (see comments below).

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:61
> +               set(self._stress_test_failures) == set(other._stress_test_failures)

Python style discourages the use of \ to break lines. Instead, wrap the statement with parens ().

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:67
> +        return self._stress_test_failures

This is the logical equivalent of a "getter" and because Python doesn't use public/private scopes you should remove this function and do direct variable access. To do this, rename _stress_test_failures => stress_test_failures and call <some_instantiated_JSCTestResultsobject>.stress_test_failures to retrieve the value.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:70
> +        return self._all_api_tests_passed

Ditto to "getter" comment.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:76
> +        return self.stress_test_failures()

Ditto to "getter" comment.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:79
> +        return self._test_results

Ditto to "getter" comment.

> Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:30
> +class JSCTestResultsReader(object):

Is this entire class really necessary? All it does it open a file and read the contents then pass it through to JSCTestResults. It seems like it'd be better to define an @classmethod that took a results_file_path and maybe had a name like "results_from_file." But if this is the only entrypoint to JSCResults, it might be even better just to merge the functionality of this class and the @classmethod from JSCTestResults::results_from_string.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:143
> +            "Patch was not relevant")

It doesn't seem necessary for line-length concerns to put each argument on its own line. Also, if self._run_command has default arguments then using them like: `return self._run_command(args, success_text="Checked relevance of patch", failure_text="Patch was not relevant")` would be more clear.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:159
> +            "Patch does not build")

Ditto to line-length and default args comment.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:175
> +            "Unable to build without patch")

Ditto to line-length and default args comment.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:213
> +            "Unable to pass tests without patch (tree is red?)")

Ditto to line-length and default args comment.

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

If make_option is used for self._delegate, I don't believe you need the hasattr(self._delegate, 'group') statement here.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:259
> +            first_results = self._delegate.test_results()

I'm unclear on what "first" and "second" are referring to -- can you rename the variables to be more accurate?

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:348
> +            return True

This function is very long... is there a way you can nicely put some of the logic into more isolated functions?

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:68
> +        class TestEWS(AbstractEarlyWarningSystem):

TestEWS should be defined outside of this function (at the top level of the file).

> Tools/Scripts/webkitpy/tool/steps/build.py:71
> +        if hasattr(self._options, 'group'):

group should always be defined given you have a default value defined (as None).

> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:64
> +        if group == "jsc":

Given we know we're going to be adding many more projects here, I recommend you do something like this:

# Define group_to_path_mapping at the same scope as jsc_paths
group_to_paths_mapping = {
    'jsc': jsc_paths,
}

def run(self, state)
    .....
    relevant_paths = self.group_to_paths_mapping.get(group, [])
    relevant = self._does_contain_change_in_paths(change_list, relevant_paths)

> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:72
> +        else:

This else is unnecessary since you return True in the alternative branch.

> Tools/Scripts/webkitpy/tool/steps/runtests.py:61
> +        if hasattr(self._options, "group") and self._options.group == "jsc":

Ditto to hasattr being unnecessary.

[Srinivasan Vijayaraghavan]

(In reply to comment #34)
> 
> Ditto to "getter" comment.
> 
> > Tools/Scripts/webkitpy/common/net/jsctestresults.py:76
> > +        return self.stress_test_failures()
> 
> Ditto to "getter" comment.
> 
> > Tools/Scripts/webkitpy/common/net/jsctestresults.py:79
> > +        return self._test_results
> 
> Ditto to "getter" comment.

failing_test_results(), failing_tests(), and test_results() are all called by other parts of the pre-existing code (for example, the code that posts a comment to Bugzilla containing a list of failing tests, which is _post_reject_message_on_bug()). My objective here was to be able to replace LayoutTestResults with JSCTestResults and have everything else just work.

Getting rid of methods from JSCTestResults will require adding logic elsewhere (eg. the existing code would have to be changed to call test_results() on LayoutTestResults objects, but just look up test_results on JSCTestResults objects). Given that we will have other types of tests in the future I think my method seemed cleaner, but if you're in favor of the other approach please let me know what your reasoning is.

That being said, I'll get rid of the ones that aren't also present in LayoutTestResults.

> > Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:30
> > +class JSCTestResultsReader(object):
> 
> Is this entire class really necessary? All it does it open a file and read
> the contents then pass it through to JSCTestResults. It seems like it'd be
> better to define an @classmethod that took a results_file_path and maybe had
> a name like "results_from_file." But if this is the only entrypoint to
> JSCResults, it might be even better just to merge the functionality of this
> class and the @classmethod from JSCTestResults::results_from_string.

AbstractEarlyWarningSystem requires a reader object which - at the very least - has a .results() method. The situation here is the same - we could either modify the existing code, or have a class that can be easily swapped with an existing class (in this case, LayoutTestResultsReader). Again, I think this is the cleaner tradeoff, but if you prefer the other way please let me know what your reasoning is.

> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:143
> > +            "Patch was not relevant")
> 
> It doesn't seem necessary for line-length concerns to put each argument on
> its own line. Also, if self._run_command has default arguments then using
> them like: `return self._run_command(args, success_text="Checked relevance
> of patch", failure_text="Patch was not relevant")` would be more clear.
> 
> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:159
> > +            "Patch does not build")
> 
> Ditto to line-length and default args comment.
> 
> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:175
> > +            "Unable to build without patch")
> 
> Ditto to line-length and default args comment.
> 
> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:213
> > +            "Unable to pass tests without patch (tree is red?)")
> 
> Ditto to line-length and default args comment.
I'll make these changes.

> 
> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:258
> > +        if hasattr(self._delegate, 'group') and self._delegate.group() == "jsc":
> 
> If make_option is used for self._delegate, I don't believe you need the
> hasattr(self._delegate, 'group') statement here.
That is the case if the _delegate is AbstarctEarlyWarningSystem, but there is a whole bunch of other unrelates queues that also use PatchAnalysisTask. I really didn't want to have this, but I couldn't find a way around it that didn't involve adding extra variables to queues that don't need them.

> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:259
> > +            first_results = self._delegate.test_results()
> 
> I'm unclear on what "first" and "second" are referring to -- can you rename
> the variables to be more accurate?
first_results could be renamed to something like "results_from_first_test_run". I didn't do that because the retry logic for layout tests also uses first_results.

> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:348
> > +            return True
> 
> This function is very long... is there a way you can nicely put some of the
> logic into more isolated functions?
I'll create separate functions.

> 
> > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:68
> > +        class TestEWS(AbstractEarlyWarningSystem):
> 
> TestEWS should be defined outside of this function (at the top level of the
> file).
I'll fix this.

> > Tools/Scripts/webkitpy/tool/steps/build.py:71
> > +        if hasattr(self._options, 'group'):
> 
> group should always be defined given you have a default value defined (as
> None).
I''ll fix this.

> > Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:64
> > +        if group == "jsc":
> 
> Given we know we're going to be adding many more projects here, I recommend
> you do something like this:
> 
> # Define group_to_path_mapping at the same scope as jsc_paths
> group_to_paths_mapping = {
>     'jsc': jsc_paths,
> }
> 
> def run(self, state)
>     .....
>     relevant_paths = self.group_to_paths_mapping.get(group, [])
>     relevant = self._does_contain_change_in_paths(change_list,
> relevant_paths)
This is great, I'll add this in!

> > Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:72
> > +        else:
> 
> This else is unnecessary since you return True in the alternative branch.
I''ll fix this.

> > Tools/Scripts/webkitpy/tool/steps/runtests.py:61
> > +        if hasattr(self._options, "group") and self._options.group == "jsc":
> 
> Ditto to hasattr being unnecessary.
I''ll fix this.

[Aakash Jain]

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.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:44
> +            return None

might be a good idea to log a warning as well.

> Tools/Scripts/webkitpy/common/net/jsctestresults_unittest.py:31
> +        empty_string = ""

might consider renaming empty_string to empty_json to make it more consistent.

> 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?

> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:52
> +        else:

'else' is unnecessary since if returns a value.

> 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:209
> +        self.assertEqual(task.test_run_count(), 3)

don't we need to assert the return_value here?

> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:218
> +        self.assertEqual(return_value, True)

same, should use assertTrue(return_value)

> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:40
> +    jsc_paths = [

can consider renaming it to jsc_relevant_paths

> 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)

> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:48
> +        for item in files:

renaming item to file would make it more readable.

[Srinivasan Vijayaraghavan]

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

[Srinivasan Vijayaraghavan]

Created attachment 301117 [details]
Patch

[WebKit Commit Bot]

Attachment 301117 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:145:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 301117 [details]
Patch

Attachment 301117 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3057354

New failing tests:
compositing/masks/solid-color-masked.html

[Build Bot]

Created attachment 301137 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Daniel Bates]

Comment on attachment 301117 [details]
Patch

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

> Tools/QueueStatusServer/config/queues.py:2
> -# Copyright (C) 2014 Apple Inc. All rights reserved.
> +# Copyright (C) 2014, 2017 Apple Inc. All rights reserved.
>  # Copyright (C) 2013 Google Inc. All rights reserved.

This patch is sufficient large. Towards making it straightforward to review this code for correctness I would appreciate if you would split this patch into three sub-patches: one patch for the changes to QueueStatusServer, one patch for non-test webkitpy changes, and one patch for the webkitpy unit tests. When it comes time to land the patches we can choose to land each sub-patch or combine the sub-patches into one mega patch.

> Tools/QueueStatusServer/handlers/statusbubble.py:198
>              return True
> -        # EWS queues are also shown when complete.
> -        return bool(queue.is_ews() and attachment.status_for_queue(queue))
> +
> +        # EWS queues are also shown when complete
> +        # But JSC EWS bubble is not shown if the patch is not relevant to JSC.
> +        if not queue.is_ews():
> +            return False
> +
> +        status = attachment.status_for_queue(queue)
> +        if queue.name() == "jsc-ews":
> +            return not status.message.startswith("Patch not applicable")
> +        return bool(status)

This is error prone because it depends on the formatting of the status message that the EWS code submits and the EWS code lives in a different directory hierarchy (Tools/Scripts/webkitpy) from this code. I also do not feel that this "do not show bubble if patch is not applicable" needs to be specific to the JSC EWS. Such functionality could be beneficial for other queues so as make the status bubble listing less noisy. Towards making this code less error prone, we need to keep this status message in sync between this code and the code in Tools/Scripts/webkitpy. I suggest that we do the following:

QueueStatusServer changes:

1. Define a dedicated status message constant, say skip_status or non_applicable_status, in Tools/QueueStatusServer/config/messages.py and a similarly named class variable in AbstractQueue (e.g. _skip_status).
2. Add a public member function to QueueStatus, say did_skip(), that returns true if and only if self.message ==  messages.skip_status

webkitpy changes:

1. Implement AbstractPatchQueue._did_skip() similar to how we implemented AbstractPatchQueue._did_pass() substituting self._skip_status for self._pass_status.
2. Modify AbstractEarlyWarningSystem.review_patch() to call AbstractPatchQueue._did_skip() and then return False when it catches the exception PatchIsNotApplicable.

Then we can update this code (in _should_show_bubble_for()) to read:

if not queue.is_ews():
    return False

status = attachment.status_for_queue(queue)
return bool(status and not status.did_skip())

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:29
> +class JSCTestResults(object):

This class needs to conform to the same interface as LayoutTestResults. What is the preferred way to do this in Python? One idea is to define an AbstractTestResults class that implements stub functions that raise an exception. Then have this class and LayoutTestResults extend AbstractTestResults and override the applicable/all methods.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:52
> +            _log.error("Invalid JSON results")

This patch alternatives between using double-quoted string literals and single-quoted string literals. From my understanding we prefer to use single-quoted string literals unless the string literal contains a single quote (then we use a double-quoted string literal). Regardless, we should pick one quoting style and stick with it throughout this patch.

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

How did you come to the decision to return the same result for all of these functions?I know that we designed JSCTestResults to conform to the interface of LayoutTestResults but if methods are not applicable for the JSC EWS and later changes ensure we do not call these functions then I suggest we omit such functions. If we really need these functions then it seems weird that they are return the same result.

[Srinivasan Vijayaraghavan]

Created attachment 301437 [details]
Patch

[WebKit Commit Bot]

Attachment 301437 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:144:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 301437 [details]
Patch

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

r-; with the exception of my second remark in comment 42 all other remarks in comment 42 were neither addressed in this patch nor replied to with a reason for disagreement. Please either address all of my remarks in comment 42 in an updated patch or reply to them with your reason(s) for disagreement. I have some more questions with this iteration of the patch.

> Tools/QueueStatusServer/handlers/statusbubble.py:190
> +        # EWS queues that weren't skipped are also shown when complete.

I do not feel that this comment adds much value. If you want to keep it then at least move it below the if-statement because the code before the if-statement and the if-statement itself are for all queues, and non-EWS queues, respectively. The code that is applicable for EWS queues is after the if-statement (lines 193 thru 195).

> Tools/Scripts/webkitpy/common/config/ports.py:111
> +        if build_style == "debug":
> +            command.append("--debug")
> +        if build_style == "release":
> +            command.append("--release")

I know that you are just mimicking the code from build_webkit_command/run_webkit_tests_command. We should not duplicate this code. We should define a static private helper function, say _append_build_style_flag(), that takes a list and a build style and appends the appropriate build flag. When we move this code into _append_build_style_flag() we should take this opportunity to clean it up and write it using an elif to avoid an unnecessary string comparison when build_style == 'debug'. Then we can write build_jsc_command, build_webkit_command, and run_webkit_tests_command (and run_javascriptcore_tests_command?) in terms of _append_build_style_flag().

> Tools/Scripts/webkitpy/common/config/ports.py:117
> +        command.append("--json-output=jsc_test_results.json")

Can we write this as two arguments?

> Tools/Scripts/webkitpy/common/config/ports.py:119
> +        if (build_style == "debug"):
> +            command.append("--debug")

How did you come to the decision to only support a debug build style?

> Tools/Scripts/webkitpy/common/config/ports.py:169
> +    def run_webkit_tests_command(self, build_style=None, test_suite=None):
> +        if test_suite == "jsc":
> +            return super(MacPort, self).run_javascriptcore_tests_command(build_style)
> +

Where are we invoking run_webkit_tests_command() with a non-None test_suite?

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

When is the exception KeyError raised? Unit tests?

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:61
> +            jsc_results_directory = os.path.dirname(os.path.dirname(os.path.dirname(self._port.results_directory())))

We should not be burying this directory path here. Similar to how we have a Port.result_directory() function, please add a function to the port base class and/or derived classes that returns the appropriate path tot he JSC results directory.

[Daniel Bates]

Comment on attachment 301437 [details]
Patch

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

> Tools/QueueStatusServer/model/queuestatus.py:46
> +        return self.message.startswith(messages.skip_status)

I just noticed that this disagrees with what I wrote in my second remark in comment 42, which implemented this function using a strict string comparision. Unless the EWS code does not emit skip_status (what does it emit when AbstractPatchQueue.did_fail() is invoked?) when skipping a patch by default, I do not feel there is value in having the EWS code (in webkitpy) emits a message of the form "Patch not applicable to %s."

[Srinivasan Vijayaraghavan]

(In reply to comment #42)
> Comment on attachment 301117 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301117&action=review
> 
> > Tools/QueueStatusServer/config/queues.py:2
> > -# Copyright (C) 2014 Apple Inc. All rights reserved.
> > +# Copyright (C) 2014, 2017 Apple Inc. All rights reserved.
> >  # Copyright (C) 2013 Google Inc. All rights reserved.
> 
> This patch is sufficient large. Towards making it straightforward to review
> this code for correctness I would appreciate if you would split this patch
> into three sub-patches: one patch for the changes to QueueStatusServer, one
> patch for non-test webkitpy changes, and one patch for the webkitpy unit
> tests. When it comes time to land the patches we can choose to land each
> sub-patch or combine the sub-patches into one mega patch.
I’ll split up the patch after addressing your suggestions. Should I upload all of them against this bug, or create three bugs?

> [...Define a dedicated status message constant, say skip_status...]
Addressed at the end.

> > Tools/Scripts/webkitpy/common/net/jsctestresults.py:52
> > +            _log.error("Invalid JSON results")
> 
> This patch alternatives between using double-quoted string literals and
> single-quoted string literals. From my understanding we prefer to use
> single-quoted string literals unless the string literal contains a single
> quote (then we use a double-quoted string literal). Regardless, we should
> pick one quoting style and stick with it throughout this patch.
Many existing files use double quotes already. I’ve attempted to keep things consistent within each file, but I’d be happy to use single quotes throughout the patch if you prefer.

> > Tools/Scripts/webkitpy/common/net/jsctestresults.py:77
> > +    def failing_test_results(self):
> > +        return self.results
> > +
> > +    def test_results(self):
> > +        return self.results
> > +
> > +    def failing_tests(self):
> > +        return self.results
> 
> How did you come to the decision to return the same result for all of these
> functions?I know that we designed JSCTestResults to conform to the interface
> of LayoutTestResults but if methods are not applicable for the JSC EWS and
> later changes ensure we do not call these functions then I suggest we omit
> such functions. If we really need these functions then it seems weird that
> they are return the same result.
With JSC, the output from running the tests is just the failures. The layout test output is the archive of all results. This explains why test_results and failing_tests (used in different parts of the rest of the code, such as posting a comment to Bugzilla listing the failures) are different in LayoutTestResults, but the same in JSCTestResults.

It’s a tradeoff between adding “if jsc” conditionals elsewhere in the code (including in methods that this patch doesn’t touch), vs just conforming to the interface here.  I think it’s cleaner to conform to the interface, but if you feel otherwise I’d like to hear your reasoning for it.

What do you mean by “later changes”? If you mean future patches, one of the fixme’s in PatchAnalysisTask is to combine the retry logic JSC and Layout test runs. failing_test_results() isn’t actually required in the current form, but combining the retry logic would probably require it.


(In reply to comment #45)
> Comment on attachment 301437 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301437&action=review
> 
> r-; with the exception of my second remark in comment 42 all other remarks
> in comment 42 were neither addressed in this patch nor replied to with a
> reason for disagreement. Please either address all of my remarks in comment
> 42 in an updated patch or reply to them with your reason(s) for
> disagreement. I have some more questions with this iteration of the patch.
> 
> > Tools/QueueStatusServer/handlers/statusbubble.py:190
> > +        # EWS queues that weren't skipped are also shown when complete.
> 
> I do not feel that this comment adds much value. If you want to keep it then
> at least move it below the if-statement because the code before the
> if-statement and the if-statement itself are for all queues, and non-EWS
> queues, respectively. The code that is applicable for EWS queues is after
> the if-statement (lines 193 thru 195).
I’ll remove the comment.

> > Tools/Scripts/webkitpy/common/config/ports.py:111
> > +        if build_style == "debug":
> > +            command.append("--debug")
> > +        if build_style == "release":
> > +            command.append("--release")
> 
> I know that you are just mimicking the code from
> build_webkit_command/run_webkit_tests_command. We should not duplicate this
> code. We should define a static private helper function, say
> _append_build_style_flag(), that takes a list and a build style and appends
> the appropriate build flag. When we move this code into
> _append_build_style_flag() we should take this opportunity to clean it up
> and write it using an elif to avoid an unnecessary string comparison when
> build_style == 'debug'. Then we can write build_jsc_command,
> build_webkit_command, and run_webkit_tests_command (and
> run_javascriptcore_tests_command?) in terms of _append_build_style_flag().
This is great, I’ll do this.

> > Tools/Scripts/webkitpy/common/config/ports.py:117
> > +        command.append("--json-output=jsc_test_results.json")
> 
> Can we write this as two arguments?
I’m not sure what this would look like in practice - could you give an example of what this might look like?

> > Tools/Scripts/webkitpy/common/config/ports.py:119
> > +        if (build_style == "debug"):
> > +            command.append("--debug")
> 
> How did you come to the decision to only support a debug build style?
The run-javascriptcore-tests script implicitly treats --release as the default option, so passing --release has the same effect as passing no build flag at all. Still, if we’re be using  _append_build_style_flag() in the other methods, I’ll use it here as well.

> > Tools/Scripts/webkitpy/common/config/ports.py:169
> > +    def run_webkit_tests_command(self, build_style=None, test_suite=None):
> > +        if test_suite == "jsc":
> > +            return super(MacPort, self).run_javascriptcore_tests_command(build_style)
> > +
> 
> Where are we invoking run_webkit_tests_command() with a non-None test_suite?
> 
> > Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:38
> > +        except (IOError, KeyError):  # File does not exist or can't be read.
> 
> When is the exception KeyError raised? Unit tests?
Thanks for catching this. 

I added this in when adding unit tests for the behavior when the test results could not be read. In the unit test environment with the mock filesystem, this would throw a KeyError.

However, turns out I later mocked out a little too much, so this line would never actually be hit. I’ll change back the mock methods so it now attempts to read the test results and fails.

> > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:61
> > +            jsc_results_directory = os.path.dirname(os.path.dirname(os.path.dirname(self._port.results_directory())))
> 
> We should not be burying this directory path here. Similar to how we have a
> Port.result_directory() function, please add a function to the port base
> class and/or derived classes that returns the appropriate path tot he JSC
> results directory.
I’ll work on this.


(In reply to comment #46)
> Comment on attachment 301437 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301437&action=review
> 
> > Tools/QueueStatusServer/model/queuestatus.py:46
> > +        return self.message.startswith(messages.skip_status)
> 
> I just noticed that this disagrees with what I wrote in my second remark in
> comment 42, which implemented this function using a strict string
> comparision. Unless the EWS code does not emit skip_status (what does it
> emit when AbstractPatchQueue.did_fail() is invoked?) when skipping a patch
> by default, I do not feel there is value in having the EWS code (in
> webkitpy) emits a message of the form "Patch not applicable to %s."
I’ll change the message emitted by EWS from “Skip: Patch not applicable to %s” to just “Skip”, which would enable a string equality comparison in QueueStatusServer.

[Srinivasan Vijayaraghavan]

Sorry, I forgot to address this:

> > Tools/Scripts/webkitpy/common/config/ports.py:169
> > +    def run_webkit_tests_command(self, build_style=None, test_suite=None):
> > +        if test_suite == "jsc":
> > +            return super(MacPort, self).run_javascriptcore_tests_command(build_style)
> > +
> 
> Where are we invoking run_webkit_tests_command() with a non-None test_suite?
We actually aren't anymore, this was a holdover and I'll remove it.

[Srinivasan Vijayaraghavan]

Created attachment 301572 [details]
Patch

[WebKit Commit Bot]

Attachment 301572 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:144:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

> > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:61
> > +            jsc_results_directory = os.path.dirname(os.path.dirname(os.path.dirname(self._port.results_directory())))
> 
> We should not be burying this directory path here. Similar to how we have a
> Port.result_directory() function, please add a function to the port base
> class and/or derived classes that returns the appropriate path tot he JSC
> results directory.
Overlooked this when uploading, will be included in next upload.

[Alexey Proskuryakov]

Dan, is splitting the patch still desirable? I would personally find it confusing to have test changes separately from others. Server and client side changes are coupled too.

My understanding is that feedback from all reviewers has been fully addressed, please confirm if that's correct.

[Srinivasan Vijayaraghavan]

Created attachment 301667 [details]
Patch

[WebKit Commit Bot]

Attachment 301667 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:143:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 301667 [details]
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.

[Srinivasan Vijayaraghavan]

Created attachment 301848 [details]
Patch

[WebKit Commit Bot]

Attachment 301848 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:143:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 301849 [details]
Patch

[WebKit Commit Bot]

Attachment 301849 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:143:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Should probably land the patch in the morning, to reduce the risk of fixing potential fallout overnight.

[Srinivasan Vijayaraghavan]

Created attachment 301972 [details]
Patch

[WebKit Commit Bot]

Attachment 301972 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:143:  [AbstractEarlyWarningSystem.group] Instance of 'AbstractEarlyWarningSystem' has no '_group' member  [pylint/E1101] [5]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 301972 [details]
Patch

Clearing flags on attachment: 301972

Committed r212579: <http://trac.webkit.org/changeset/212579>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Aakash Jain]

Created attachment 302000 [details]
Screenshot - patch not relevant

[Aakash Jain]

Seems like it broke EWS. Getting "Patch was not relevant" message for most of the queues (including iOS, mac etc.) See attached screenshot.

[Aakash Jain]

landed https://trac.webkit.org/changeset/212591 to fix patch Relevance issue.

[David Kilzer (:ddkilzer)]

<rdar://problem/18231364>