Bug 137229 – LayoutTestResults and ExpectedFailures should know about the interrupted flag from the json results file

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

[Jake Nielsen]

results.json has an "interrupted" key that is set when the number of failing tests exceeds the failing test dropout limit. Instead of checking the number of failed tests, LayoutTestResults and ExpectedFailures should simply check this key.

[Jake Nielsen]

Created attachment 238879 [details]
Fixes LayoutTestResults to only have a notion of whether the interrupted flag was set.

[WebKit Commit Bot]

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


ERROR: Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:76:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:113:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:283:  missing whitespace after ':'  [pep8/E231] [5]
ERROR: Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:314:  missing whitespace after ':'  [pep8/E231] [5]
ERROR: Tools/Scripts/webkitpy/tool/bot/layouttestresultsreader_unittest.py:71:  missing whitespace after ':'  [pep8/E231] [5]
ERROR: Tools/Scripts/webkitpy/tool/bot/layouttestresultsreader_unittest.py:84:  missing whitespace after ':'  [pep8/E231] [5]
ERROR: Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:51:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:65:  whitespace after '('  [pep8/E201] [5]
Total errors found: 8 in 9 files


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

[Tim Horton]

Comment on attachment 238879 [details]
Fixes LayoutTestResults to only have a notion of whether the interrupted flag was set.

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

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:156
> +        #Because we want to keep all of the metadata stored in the json dict, but we want already-processed test results,

Needs a space after the '#', but probably the stylebot told you that.

[Jake Nielsen]

Created attachment 238881 [details]
Fixes style issues

[Alexey Proskuryakov]

Comment on attachment 238881 [details]
Fixes style issues

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

> Tools/Scripts/webkitpy/common/net/layouttestresults.py:66
> +    def interrupted(self):
> +        return self._result_dict["interrupted"]
>  
>      def test_results(self):
> -        return self._test_results
> +        return self._result_dict['tests']

It may be nicer to split the dictionary into separate member variables when constructing the LayoutTestResults object. There doesn't seem to be any reason to keep them together.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:156
> +        # Because we want to keep all of the metadata stored in the json dict, but we want already-processed test results,

I'd state this slightly differently:

# We want to keep all of the metadata stored in the json dictionary as is, but we want already-processed test results.

As long as comment explains why something is done, the reader is supposed to read how the code achieves this.

[Daniel Bates]

Comment on attachment 238881 [details]
Fixes style issues

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

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:51
> +            resultsdict = {"tests": results, "interrupted": False}

I take it that it doesn't make sense to incorporate this functionality into LayoutTestResults directly?

Assuming it doesn't make sense to incorporate this functionality into LayoutTestResults then using a dictionary with names for special keys (e.g. "interrupted") that are duplicated each time we want to construct such a dictionary seems error prone, say a caller incorrectly spells the name of a special key. Will such a misspelling manifest itself in a way that makes it straightforward to notice the issue? Is there anything analogous to a C-struct in Python? Otherwise, maybe define a class with public instance variables and optionally a non-empty constructor. At the very least, we should document the convention.

On another note, this local variable is used exactly once and I don't feel that its name improves the readability of the code as it's a portmanteau of the word "results" and its data type. I suggest we either come up a more descriptive name or inline its definition into the line below.

>> Tools/Scripts/webkitpy/common/net/layouttestresults.py:66
>> +        return self._result_dict['tests']
> 
> It may be nicer to split the dictionary into separate member variables when constructing the LayoutTestResults object. There doesn't seem to be any reason to keep them together.

Nit: ' => "

(single quotes) => (double quotes)

> Tools/Scripts/webkitpy/common/net/layouttestresults.py:69
> +        return [result for result in self._result_dict['tests'] if result.has_failure_matching_types(*failure_types)]

Ditto.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:158
> +        json_dict['tests'] = unexpected_failures

Nit: ' => "

> Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:91
> +        results = ResultsJSONParser.parse_results_json(self._example_full_results_json)['tests']

Ditto.

[Daniel Bates]

Comment on attachment 238881 [details]
Fixes style issues

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

>> Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:51
>> +            resultsdict = {"tests": results, "interrupted": False}
> 
> I take it that it doesn't make sense to incorporate this functionality into LayoutTestResults directly?
> 
> Assuming it doesn't make sense to incorporate this functionality into LayoutTestResults then using a dictionary with names for special keys (e.g. "interrupted") that are duplicated each time we want to construct such a dictionary seems error prone, say a caller incorrectly spells the name of a special key. Will such a misspelling manifest itself in a way that makes it straightforward to notice the issue? Is there anything analogous to a C-struct in Python? Otherwise, maybe define a class with public instance variables and optionally a non-empty constructor. At the very least, we should document the convention.
> 
> On another note, this local variable is used exactly once and I don't feel that its name improves the readability of the code as it's a portmanteau of the word "results" and its data type. I suggest we either come up a more descriptive name or inline its definition into the line below.

I take it that it doesn't make sense to incorporate this functionality into LayoutTestResults directly?

Assuming it doesn't make sense to incorporate this functionality into LayoutTestResults then using a dictionary with names for special keys (e.g. "interrupted") that are duplicated each time we want to construct such a dictionary seems error prone, say a caller incorrectly spells the name of a special key. Will such a misspelling manifest itself in a way that makes it straightforward to notice the issue? Is there anything analogous to a C-struct in Python? Otherwise, maybe define a class with public instance variables and optionally a non-empty constructor. At the very least, we should document the convention.

On another note, this local variable is used exactly once and I don't feel that its name improves the readability of the code as it's a portmanteau of the word "results" and its data type. I suggest we either come up a more descriptive name or inline its definition into the line below.

>>> Tools/Scripts/webkitpy/common/net/layouttestresults.py:66
>>> +        return self._result_dict['tests']
>> 
>> It may be nicer to split the dictionary into separate member variables when constructing the LayoutTestResults object. There doesn't seem to be any reason to keep them together.
> 
> Nit: ' => "
> 
> (single quotes) => (double quotes)

Nit: ' => "

(single quotes) => (double quotes)

>> Tools/Scripts/webkitpy/common/net/layouttestresults.py:69
>> +        return [result for result in self._result_dict['tests'] if result.has_failure_matching_types(*failure_types)]
> 
> Ditto.

Ditto.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:-155
>          # FIXME: What's the short sexy python way to filter None?
>          # I would use [foo.bar() for foo in foos if foo.bar()] but bar() is expensive.
>          unexpected_failures = [result.test_result() for result in json_results if not result.did_pass_or_run_as_expected()]
> -        return filter(lambda a: a, unexpected_failures)

From my understanding, the FIXME comment was referring to the use of filter() and implied that result.test_result() may return None. Is it still true that result.test_result() may return None? Does the layout test result machinery still work when json_dict["tests"] contains such None values?

>> Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:91
>> +        results = ResultsJSONParser.parse_results_json(self._example_full_results_json)['tests']
> 
> Ditto.

Ditto.

[Jake Nielsen]

Created attachment 238940 [details]
Addresses points concerning strong coupling with results.son

[Jake Nielsen]

Created attachment 238944 [details]
Makes naming conventions more clear

[Daniel Bates]

Comment on attachment 238944 [details]
Makes naming conventions more clear

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

This patch looks better. I noticed some cosmetic issues. Although these issues are cosmetic, I'm r-'ing this patch since there are many such issues and it would be good to see another iteration.

> Tools/ChangeLog:4
> +        Changes LayoutTestResults to use the interrupted flag instead of
> +        counting failures

This bug description should be moved to be after the Reviewed by line (below) and the bug title should be here such that the top portion of this change log entry has the form:

LayoutTestResults and ExpectedFailures should know about the interrupted flag from the json results file
https://bugs.webkit.org/show_bug.cgi?id=137229

Reviewed by NOBODY (OOPS!).

Changes LayoutTestResults to use the interrupted flag instead of counting failures.

On another note, the description is out-of-date and we should update it to reflect the current state of the patch. You may also want to consider adding per function/per file comments below.

> Tools/ChangeLog:46
> +        * Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:
> +        (BuilderTest._install_fetch_build._mock_fetch_build):
> +        (BuilderTest.test_latest_layout_test_results):
> +        * Scripts/webkitpy/common/net/layouttestresults.py:
> +        (LayoutTestResults.results_from_string):
> +        (LayoutTestResults.__init__):
> +        (LayoutTestResults.interrupted):
> +        (LayoutTestResults.test_results):
> +        (LayoutTestResults.results_matching_failure_types):
> +        (LayoutTestResults): Deleted.
> +        (LayoutTestResults.set_failure_limit_count): Deleted.
> +        (LayoutTestResults.failure_limit_count): Deleted.
> +        * Scripts/webkitpy/common/net/layouttestresults_unittest.py:
> +        (LayoutTestResultsTest.test_set_failure_limit_count): Deleted.
> +        * Scripts/webkitpy/common/net/resultsjsonparser.py:
> +        (ResultsJSONParser.parse_results_json):
> +        * Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:
> +        (test_basic):
> +        * Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:
> +        (MockCommitQueue.test_results):
> +        (FailingTestCommitQueue.test_results):
> +        (test_flaky_test_failure):
> +        (test_failed_archive):
> +        * Scripts/webkitpy/tool/bot/expectedfailures.py:
> +        (ExpectedFailures._should_trust):
> +        * Scripts/webkitpy/tool/bot/expectedfailures_unittest.py:
> +        (MockResults.__init__):
> +        (MockResults.interrupted):
> +        (ExpectedFailuresTest.test_can_trust_results):
> +        (ExpectedFailuresTest.test_unexpected_failures_observed):
> +        (ExpectedFailuresTest.test_unexpected_failures_observed_when_tree_is_hosed):
> +        (MockResults.failure_limit_count): Deleted.
> +        * Scripts/webkitpy/tool/bot/layouttestresultsreader.py:
> +        (LayoutTestResultsReader.results):
> +        * Scripts/webkitpy/tool/bot/layouttestresultsreader_unittest.py:
> +        (test_missing_unit_test_results_path):
> +        (test_layout_test_results):
> +

This listing of added/modified/deleted functions is out-of-date. In particular, the method LayoutTestResults.interrupted was renamed to LayoutTestResults.did_exceed_test_failure_limit. Please generate a new change log entry.

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:51
> +            layout_test_results = LayoutTestResults(results, False)

It's not clear what the purpose of the boolean False is without looking at the definition of class LayoutTestResults. I suggest that we use the named argument syntax so as to make the purpose of this argument clear at the callsite:

layout_test_results = LayoutTestResults(results, did_exceed_test_failure_limit=False)

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:64
> +        self.builder.fetch_layout_test_results = lambda results_url: LayoutTestResults([self._mock_test_result(testname) for testname in ["test1", "test2"]], False)

Ditto.

> Tools/Scripts/webkitpy/common/net/layouttestresults.py:53
> +        full_results = ParsedJSONResults(string)

Maybe a better name for this variable would be results or parsed_results or parsed_json?

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:152
> +

Please remove this empty line as I don't feel that it improves the readability of this code.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:156
> +

Ditto.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:162
> +        self._interrupted = json_dict["interrupted"]

We tend to use the name the instance variable in the name of its getter for consistency. So, I suggest that we rename this instance variable from _interrupted to _did_exceed_test_failure_limit.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:165
> +        return self._interrupted

Ditto.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:92
> +        full_results = ParsedJSONResults(self._example_full_results_json)

As aforementioned, maybe a better name for this variable would be results or parsed_results or parsed_json?

> Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:93
> +        results = full_results.test_results()

This variable is used exactly once and I don't feel the name of this variable improves the readability to the code as the name of the method name "test_results()" in the right hand side expression is sufficiently descriptive. I suggest that we inline the value of this variable into the line below and remove this variable.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:95
> +        self.assertEqual(True, full_results.did_exceed_test_failure_limit())

Notice that the unittest module has a convenience function called assertTrue that asserts that the specified expression evaluates to true in a boolean context. I would use this function instead of unittest.assertEqual(True, ...) and write this line as:

self.assertTrue(full_results.did_exceed_test_failure_limit())

See <https://docs.python.org/2/library/unittest.html> for more details.

> Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:76
> +        return LayoutTestResults([], True)

It's not clear what the purpose of the boolean False is without looking at the definition of class LayoutTestResults. I suggest that we use the named argument syntax so as to make the purpose of this argument clear at the callsite:

return LayoutTestResults([], did_exceed_test_failure_limit=True)

> Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:283
> +        commit_queue.test_results = lambda: LayoutTestResults([], True)

Similarly, I would write this line using a name argument for the second argument to LayoutTestResults().

> Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:314
> +        commit_queue.test_results = lambda: LayoutTestResults([], True)

Ditto.

> Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:43
> +        if results:
> +            return not results.did_exceed_test_failure_limit()
> +        return False

The if-statement isn't necessary. I would have written this as:

return bool(results and not results.did_exceed_test_failure_limit())

> Tools/Scripts/webkitpy/tool/bot/layouttestresultsreader_unittest.py:71
> +        reader._create_layout_test_results = lambda: LayoutTestResults([], False)

It's not clear what the purpose of the boolean False is without looking at the definition of class LayoutTestResults. I suggest that we use the named argument syntax so as to make the purpose of this argument clear at the callsite:

reader._create_layout_test_results = lambda: LayoutTestResults([], did_exceed_test_failure_limit=False)

> Tools/Scripts/webkitpy/tool/bot/layouttestresultsreader_unittest.py:84
> +        reader._create_layout_test_results = lambda: LayoutTestResults([], False)

Ditto

[Jake Nielsen]

Created attachment 238965 [details]
Addresses style points.

[Jake Nielsen]

Created attachment 238966 [details]
Adds periods to sentences in changelog, and adds test case for interrupted: false.

[WebKit Commit Bot]

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


ERROR: Tools/ChangeLog:84:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 10 files


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

[Daniel Bates]

Comment on attachment 238966 [details]
Adds periods to sentences in changelog, and adds test case for interrupted: false.

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

Thanks for updating the patch!

> Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:144
> +    def test_alternate(self):

This is OK as-is. I wish we had a more descriptive name for this function.

[Daniel Bates]

Comment on attachment 238966 [details]
Adds periods to sentences in changelog, and adds test case for interrupted: false.

Clearing cq flag because of comment #13.

[Jake Nielsen]

Created attachment 238969 [details]
Removes tab, and renames test.

[WebKit Commit Bot]

Comment on attachment 238969 [details]
Removes tab, and renames test.

Clearing flags on attachment: 238969

Committed r174130: <http://trac.webkit.org/changeset/174130>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.