Add switch to update-test-expectations to show removed lines in dashboard

Add switch to update-test-expectations to show removed lines in dashboard

Added a --show-results switch. When added to a run of update-test-expectations,
the script will open the layout test flakiness dashboard with all the tests the
script removed. This allows users to manually verify the removed lines.

BUG=595414
Committed: https://crrev.com/4bec15a584aedce4f4d92033983db81b0c3a8034
Cr-Commit-Position: refs/heads/master@{#423336}

[bokan, 2016-09-30 00:08:49]

Description was changed from

==========
Add switch to update-test-expectations to show removed lines in dashboard

Added a --show-results switch. When added to a run of update-test-expectations,
the script will open the layout test flakiness dashboard with all the tests the
script removed. This allows users to manually verify the removed lines.

BUG=595414
==========

to

==========
Add switch to update-test-expectations to show removed lines in dashboard

Added a --show-results switch. When added to a run of update-test-expectations,
the script will open the layout test flakiness dashboard with all the tests the
script removed. This allows users to manually verify the removed lines.

BUG=595414
==========

[bokan, 2016-09-30 00:08:50]

bokan@chromium.org changed reviewers:
+ qyearsley@chromium.org

[bokan, 2016-09-30 00:09:02]

Here's a small addition I've been meaning to add.

[qyearsley, 2016-09-30 23:26:26]

So far it looks great, but I haven't fully finished looking at this today -- if
I don't respond on Monday feel free to ping to remind me :-)

https://codereview.chromium.org/2381093003/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py
(right):

https://codereview.chromium.org/2381093003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:316:
remove from the TestExpectations file and allowing the user to manually confirm
the
Possible rephrasing:
   "... tests that the script will/did remove ..."
-> "... tests that the script removed ..."
OR "... tests for lines that the script removed ..."

https://codereview.chromium.org/2381093003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:320:
removed_test_names = ','.join([x.name for x in self._expectations_to_remove])
Since str.join can take any iterable (including a generator expression),
expressions like this can also be written without square brackets, as:
  ','.join(x.name for x in self._expectations_to_remove)
This usually wouldn't really make a difference, but may use less memory if the
list is very long.

[bokan, 2016-10-04 16:13:46]

https://codereview.chromium.org/2381093003/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py
(right):

https://codereview.chromium.org/2381093003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:316:
remove from the TestExpectations file and allowing the user to manually confirm
the
On 2016/09/30 23:26:26, qyearsley wrote:
> Possible rephrasing:
>    "... tests that the script will/did remove ..."
> -> "... tests that the script removed ..."
> OR "... tests for lines that the script removed ..." 

Done.

https://codereview.chromium.org/2381093003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:320:
removed_test_names = ','.join([x.name for x in self._expectations_to_remove])
On 2016/09/30 23:26:26, qyearsley wrote:
> Since str.join can take any iterable (including a generator expression),
> expressions like this can also be written without square brackets, as:
>   ','.join(x.name for x in self._expectations_to_remove)
> This usually wouldn't really make a difference, but may use less memory if the
> list is very long.

Neat, thanks. Done.

[bokan, 2016-10-05 20:48:20]

friendly ping

[qyearsley, 2016-10-05 21:06:08]

Thanks for reminding me :-)

LGTM, with some more comments and thoughts to possibly change some things before
committing.

https://codereview.chromium.org/2381093003/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py
(right):

https://codereview.chromium.org/2381093003/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:278:
return
Another possible approach to consider here may be to make
_expectations_to_remove a memoized property (using the memoized decorator in
webkitpy/common/memoized.py, OR by doing something like:

def _expectations_to_remove(self):
    if self._expectations_to_remove is not None:
        return self._expectations_to_remove

    # logic to populate self._expectations_to_remove
    return self._expectations_to_remove

But this as-is is good too.

https://codereview.chromium.org/2381093003/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py
(right):

https://codereview.chromium.org/2381093003/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:6:
import webkitpy.layout_tests.update_test_expectations
Formatting/style nit: Would this still work if this were moved down and changed
to: from webkitpy.layout_tests import update_test_expectations?

https://codereview.chromium.org/2381093003/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:99:
)
One way to refactor to make this more similar to other tests in webkitpy is:
RemoveFlakesOMatic could have a "webbrowser" attribute which is set in the
constructor, and replaced in the tests. What do you think?

[bokan, 2016-10-05 22:35:25]

https://codereview.chromium.org/2381093003/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py
(right):

https://codereview.chromium.org/2381093003/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:278:
return
On 2016/10/05 21:06:08, qyearsley wrote:
> Another possible approach to consider here may be to make
> _expectations_to_remove a memoized property (using the memoized decorator in
> webkitpy/common/memoized.py, OR by doing something like:
> 
> def _expectations_to_remove(self):
>     if self._expectations_to_remove is not None:
>         return self._expectations_to_remove
> 
>     # logic to populate self._expectations_to_remove
>     return self._expectations_to_remove
> 
> But this as-is is good too.

Done.

https://codereview.chromium.org/2381093003/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py
(right):

https://codereview.chromium.org/2381093003/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:6:
import webkitpy.layout_tests.update_test_expectations
On 2016/10/05 21:06:08, qyearsley wrote:
> Formatting/style nit: Would this still work if this were moved down and
changed
> to: from webkitpy.layout_tests import update_test_expectations?

Yes but it's no longer needed due to your suggestion below.

https://codereview.chromium.org/2381093003/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:99:
)
On 2016/10/05 21:06:08, qyearsley wrote:
> One way to refactor to make this more similar to other tests in webkitpy is:
> RemoveFlakesOMatic could have a "webbrowser" attribute which is set in the
> constructor, and replaced in the tests. What do you think?

Done.

[bokan, 2016-10-05 22:35:40]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-10-05 22:35:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
qyearsley@chromium.org
Link to the patchset: https://codereview.chromium.org/2381093003/#ps60001
(title: "Addressed feedback")

[commit-bot: I haz the power, 2016-10-05 22:36:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 23:28:03]

Description was changed from

==========
Add switch to update-test-expectations to show removed lines in dashboard

Added a --show-results switch. When added to a run of update-test-expectations,
the script will open the layout test flakiness dashboard with all the tests the
script removed. This allows users to manually verify the removed lines.

BUG=595414
==========

to

==========
Add switch to update-test-expectations to show removed lines in dashboard

Added a --show-results switch. When added to a run of update-test-expectations,
the script will open the layout test flakiness dashboard with all the tests the
script removed. This allows users to manually verify the removed lines.

BUG=595414
==========

[commit-bot: I haz the power, 2016-10-05 23:28:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-05 23:29:48]

Description was changed from

==========
Add switch to update-test-expectations to show removed lines in dashboard

Added a --show-results switch. When added to a run of update-test-expectations,
the script will open the layout test flakiness dashboard with all the tests the
script removed. This allows users to manually verify the removed lines.

BUG=595414
==========

to

==========
Add switch to update-test-expectations to show removed lines in dashboard

Added a --show-results switch. When added to a run of update-test-expectations,
the script will open the layout test flakiness dashboard with all the tests the
script removed. This allows users to manually verify the removed lines.

BUG=595414

Committed: https://crrev.com/4bec15a584aedce4f4d92033983db81b0c3a8034
Cr-Commit-Position: refs/heads/master@{#423336}
==========

[commit-bot: I haz the power, 2016-10-05 23:29:49]

Patchset 4 (id:??) landed as
https://crrev.com/4bec15a584aedce4f4d92033983db81b0c3a8034
Cr-Commit-Position: refs/heads/master@{#423336}