Bug 140744 – Make EWS/CQ consider results of test-webkitpy in PatchAnalysisTask

• Status: NEW
• Component: Tools / Tests

[Jake Nielsen]

PatchAnalysisTask._test_patch() only considers the results of layout test, not anything else. It should be smart enough to consider many types of test results (including the results of test-webkitpy).

[Jake Nielsen]

Created attachment 245672 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/Scripts/webkitpy/tool/bot/unittest_resources/layout_test_example_dir/full_results.json:0:  No JSON object could be decoded  [json/syntax] [5]
ERROR: Tools/Scripts/webkitpy/tool/bot/unittest_resources/python_test_malformed_example_dir/results.json:0:  No JSON object could be decoded  [json/syntax] [5]
ERROR: Tools/Scripts/webkitpy/tool/bot/unittest_resources/layout_test_malformed_example_dir/full_results.json:0:  No JSON object could be decoded  [json/syntax] [5]
Total errors found: 3 in 18 files


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

[Jake Nielsen]

(In reply to comment #2)
> Attachment 245672 [details] did not pass style-queue:
> 
> 
> ERROR:
> Tools/Scripts/webkitpy/tool/bot/unittest_resources/layout_test_example_dir/
> full_results.json:0:  No JSON object could be decoded  [json/syntax] [5]
> ERROR:
> Tools/Scripts/webkitpy/tool/bot/unittest_resources/
> python_test_malformed_example_dir/results.json:0:  No JSON object could be
> decoded  [json/syntax] [5]
> ERROR:
> Tools/Scripts/webkitpy/tool/bot/unittest_resources/
> layout_test_malformed_example_dir/full_results.json:0:  No JSON object could
> be decoded  [json/syntax] [5]
> Total errors found: 3 in 18 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

One of these is actually JSONP, and the other two are intentionally malformed.

[Csaba Osztrogonác]

Dow we really needd to check in these huge tests?

[Jake Nielsen]

Created attachment 245985 [details]
Patch

[Jake Nielsen]

Yeah, those were pretty unruly. I've pruned them down.

[WebKit Commit Bot]

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


ERROR: Tools/Scripts/webkitpy/tool/bot/unittest_resources/layout_test_example_dir/full_results.json:0:  No JSON object could be decoded  [json/syntax] [5]
ERROR: Tools/Scripts/webkitpy/tool/bot/unittest_resources/python_test_malformed_example_dir/results.json:14:  Expecting ',' delimiter: line 14 column 5 (char 2071)  [json/syntax] [5]
ERROR: Tools/Scripts/webkitpy/tool/bot/unittest_resources/layout_test_malformed_example_dir/full_results.json:0:  No JSON object could be decoded  [json/syntax] [5]
Total errors found: 3 in 18 files


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

[Daniel Bates]

Comment on attachment 245985 [details]
Patch

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

I'm still looking over this patch. Here are some comments I have so far.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:194
> +        return reduce(lambda first, second: first or second, map(lambda results: results.did_exceed_test_failure_limit, list_of_results), False)

I would take advantage of the builtin any() function simplify this to read:

return any(map(lambda results: results.did_exceed_test_failure_limit, list_of_results))

Event better, we can avoid allocating a new list by using itertools.imap() instead of map() and use operator.attrgetter("did_exceed_test_failure_limit") to avoid using a lambda such that we have:

return any(itertools.imap(operator.attrgetter("did_exceed_test_failure_limit"), list_of_results))

> Tools/Scripts/webkitpy/tool/bot/test_results_archiver.py:1
> +import logging

Please add an Apple license block to the top of this file. You should be able to find such a license block in another webkitpy file. Otherwise, you should create one whose content has the form shown in <http://trac.webkit.org/export/179959/trunk/Source/WebCore/LICENSE-APPLE>.

> Tools/Scripts/webkitpy/tool/bot/test_results_archiver.py:28
> +            if not filesystem.isdir(results_directory):
> +                _log.info("%s does not exist, not archiving test results from that directory." % results_directory)

We should use an early continue style for this if-statement.

> Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:1
> +import json

Please add an Apple license block to the top of this file. You should be able to find such a license block in another webkitpy file. Otherwise, you should create one whose content has the form shown in <http://trac.webkit.org/export/179959/trunk/Source/WebCore/LICENSE-APPLE>.

> Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:7
> +class TestResults(object):

Maybe a better name for this class would be TestSuiteResults since it represents the results of a collection/suite of test cases.

> Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:8
> +

Nit: The existing webkitpy code and the code in this patch is inconsistent with respect to the presence of an empty line between the class statement line and the first class method. I suggest we pick a style and be consistent.

> Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:10
> +        self.name = name

Do we expect a TestResults instance to be mutable? I would assume they are immutable and hence I would make all its instance variables private (prefix their name with an underscore) and expose read-only property getter for them. That is, I would write this line as:

self._name = name

And then add the following property definition to this class:

@property
def name(self):
    return self._name

We should make read-only properties for self.name, self.list_of_failing_tests, self.did_exceed_test_failure_limit and self.did_create_test_results.

See <https://docs.python.org/2/library/functions.html#property> for more details on defining a Python property.

> Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:14
> +        if list_of_failing_tests:
> +            self.list_of_failing_tests = list_of_failing_tests
> +        else:
> +            self.list_of_failing_tests = []

How did you come to the decision to initialize this instance variable in the body of the constructor as opposed to specifying [] as the default value for parameter list_of_failing_tests? I would expect a TestResults object to be immutable and that it does not modify the data passed to it. In particular, I would not expect that TestResults exposes a method(s) to modify self.list_of_failing_tests. So, it seems sufficient to define the default value of parameter list_of_failing_tests as [].

I'm assuming that list_of_failing_tests will also not be modified after it is passed to the constructor of TestResults by code outside of TestResults. If we can't make this assumption then TestResults should make a deep-copy of list_of_failing_tests.

> Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:19
> +        return self.did_create_test_results and len(self.list_of_failing_tests) == 0

Nit: Notice that the empty list evaluates to False in a boolean context. We should take advantage of this fact instead of comparing the length of the list to 0 per section "Programming Recommendations" of the PEP 8 - Style Guide for Python Code, <https://www.python.org/dev/peps/pep-0008/#programming-recommendations>, such that this line reads:

return self.did_create_test_results and self.list_of_failing_tests

> Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:80
> +            with open(results_path) as results_file:
> +                results_string = results_file.read()

We should be using self._port.host.filesystem.read_text_file() instead of directly reading the file at path results_path.

> Tools/Scripts/webkitpy/tool/bot/test_results_reader_and_archiver_unittest.py:113
> +        our_dir = os.path.dirname(os.path.abspath(__file__))

This variable is unused.

> Tools/Scripts/webkitpy/tool/bot/test_results_reader_and_archiver_unittest.py:138
> +        our_dir = os.path.dirname(os.path.abspath(__file__))

Ditto.

[Jake Nielsen]

(In reply to comment #8)
> Comment on attachment 245985 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245985&action=review
> 
> I'm still looking over this patch. Here are some comments I have so far.
> 
> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:194
> > +        return reduce(lambda first, second: first or second, map(lambda results: results.did_exceed_test_failure_limit, list_of_results), False)
> 
> I would take advantage of the builtin any() function simplify this to read:
> 
> return any(map(lambda results: results.did_exceed_test_failure_limit,
> list_of_results))
> 
> Event better, we can avoid allocating a new list by using itertools.imap()
> instead of map() and use
> operator.attrgetter("did_exceed_test_failure_limit") to avoid using a lambda
> such that we have:
> 
> return
> any(itertools.imap(operator.attrgetter("did_exceed_test_failure_limit"),
> list_of_results))
> 
> > Tools/Scripts/webkitpy/tool/bot/test_results_archiver.py:1
> > +import logging
> 
> Please add an Apple license block to the top of this file. You should be
> able to find such a license block in another webkitpy file. Otherwise, you
> should create one whose content has the form shown in
> <http://trac.webkit.org/export/179959/trunk/Source/WebCore/LICENSE-APPLE>.
> 
> > Tools/Scripts/webkitpy/tool/bot/test_results_archiver.py:28
> > +            if not filesystem.isdir(results_directory):
> > +                _log.info("%s does not exist, not archiving test results from that directory." % results_directory)
> 
> We should use an early continue style for this if-statement.
> 
> > Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:1
> > +import json
> 
> Please add an Apple license block to the top of this file. You should be
> able to find such a license block in another webkitpy file. Otherwise, you
> should create one whose content has the form shown in
> <http://trac.webkit.org/export/179959/trunk/Source/WebCore/LICENSE-APPLE>.
> 
> > Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:7
> > +class TestResults(object):
> 
> Maybe a better name for this class would be TestSuiteResults since it
> represents the results of a collection/suite of test cases.
> 
> > Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:8
> > +
> 
> Nit: The existing webkitpy code and the code in this patch is inconsistent
> with respect to the presence of an empty line between the class statement
> line and the first class method. I suggest we pick a style and be consistent.
> 
> > Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:10
> > +        self.name = name
> 
> Do we expect a TestResults instance to be mutable? I would assume they are
> immutable and hence I would make all its instance variables private (prefix
> their name with an underscore) and expose read-only property getter for
> them. That is, I would write this line as:
> 
> self._name = name
> 
> And then add the following property definition to this class:
> 
> @property
> def name(self):
>     return self._name
> 
> We should make read-only properties for self.name,
> self.list_of_failing_tests, self.did_exceed_test_failure_limit and
> self.did_create_test_results.
> 
> See <https://docs.python.org/2/library/functions.html#property> for more
> details on defining a Python property.
> 
> > Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:14
> > +        if list_of_failing_tests:
> > +            self.list_of_failing_tests = list_of_failing_tests
> > +        else:
> > +            self.list_of_failing_tests = []
> 
> How did you come to the decision to initialize this instance variable in the
> body of the constructor as opposed to specifying [] as the default value for
> parameter list_of_failing_tests? I would expect a TestResults object to be
> immutable and that it does not modify the data passed to it. In particular,
> I would not expect that TestResults exposes a method(s) to modify
> self.list_of_failing_tests. So, it seems sufficient to define the default
> value of parameter list_of_failing_tests as [].
> 
> I'm assuming that list_of_failing_tests will also not be modified after it
> is passed to the constructor of TestResults by code outside of TestResults.
> If we can't make this assumption then TestResults should make a deep-copy of
> list_of_failing_tests.
> 
> > Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:19
> > +        return self.did_create_test_results and len(self.list_of_failing_tests) == 0
> 
> Nit: Notice that the empty list evaluates to False in a boolean context. We
> should take advantage of this fact instead of comparing the length of the
> list to 0 per section "Programming Recommendations" of the PEP 8 - Style
> Guide for Python Code,
> <https://www.python.org/dev/peps/pep-0008/#programming-recommendations>,
> such that this line reads:
> 
> return self.did_create_test_results and self.list_of_failing_tests
> 
> > Tools/Scripts/webkitpy/tool/bot/test_results_reader.py:80
> > +            with open(results_path) as results_file:
> > +                results_string = results_file.read()
> 
> We should be using self._port.host.filesystem.read_text_file() instead of
> directly reading the file at path results_path.
> 
> > Tools/Scripts/webkitpy/tool/bot/test_results_reader_and_archiver_unittest.py:113
> > +        our_dir = os.path.dirname(os.path.abspath(__file__))
> 
> This variable is unused.
> 
> > Tools/Scripts/webkitpy/tool/bot/test_results_reader_and_archiver_unittest.py:138
> > +        our_dir = os.path.dirname(os.path.abspath(__file__))
> 
> Ditto.

Sorry it took me so long to circle back to this. I completely agree on all fronts, and I'll make those changes soon.

[Alexey Proskuryakov]

rdar://problem/18231414

[Brent Fulgham]

What's the status on this? I would love to see this move forward -- we've already run into some failures that would have been caught by this enhancement. :-)

[Michael Catanzaro]

Comment on attachment 245985 [details]
Patch

Removing this from request queue; please set r? again after making the changes Dan suggested.

[ayoub jama]

Comment on attachment 245985 [details]
Patch

please remive