[webkit-reviews] review denied: [Bug 137229] LayoutTestResults and ExpectedFailures should know about the interrupted flag from the json results file : [Attachment 238944] Makes naming conventions more clear

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Jake Nielsen
<jake.nielsen.webkit at gmail.com>'s request for review:
Bug 137229: LayoutTestResults and ExpectedFailures should know about the
interrupted flag from the json results file
https://bugs.webkit.org/show_bug.cgi?id=137229

Attachment 238944: Makes naming conventions more clear
https://bugs.webkit.org/attachment.cgi?id=238944&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
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