Fill out implementation of UpdateTestExpectations script

Fill out implementation of UpdateTestExpectations script

In r404402 I created the outline of the UpdateTestExpectations script that
removes lines from TestExpectations that are no longer flaky according to
results on the build waterfall. This CL actually implements the functionality
in the script and makes it basically functional.

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

[bokan, 2016-07-05 12:47:55]

Description was changed from

==========
[update_test_expectations.py 3/3] Fill out implementation of
UpdateTestExpectations

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

to

==========
[update_test_expectations.py 3/3] Fill out implementation of
UpdateTestExpectations

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

[bokan, 2016-07-05 12:47:55]

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

[Dirk Pranke, 2016-07-06 21:51:31]

I'll defer this review to qyearsley@, I didn't see anything that jumped out at
me as a weird layering issue, but I didn't look at the core logic.

[qyearsley, 2016-07-08 17:55:04]

Great change, and I couldn't find anything to suggest to improve the tests.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/builder_list.py
(right):

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/builder_list.py:90: def
builder_name_for_specifiers(self, version, build_type):
A docstring here could clarify that version is the OS version specifier (e.g.
"Win7") and build_type is the build configuration type (e.g. "Debug").

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/builder_list.py:91: for
builder_name, info in self._builders.items():
Since dicts are unordered, it's theoretically possible that you could have a
_builders dict like:

    {
        'foo': {'specifiers': ['a', 'b']},
        'bar': {'specifiers': ['a', 'b']},
    }

and then it's not clearly defined whether builder_name_for_specifiers('a', 'b')
should return 'foo' or 'bar'. This likely won't be a problem, but just to make
it clearly defined, you could iterate through `sorted(self._builders.items())`.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/builder_list.py:93: if
specifiers[0].lower() == version.lower() and specifiers[1].lower() ==
build_type.lower():
This is implicitly adding an assumption that specifier is always of the form
[<OS version>, <build config>].

I think this is fine, but the comment on line 44 should probably be updated.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py
(right):

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py:112:
RESULTS_URL_PREFIX =
'http://test-results.appspot.com/testfile?master=ChromiumWebkit&testtype=webkit_tests&name=results-small.json&builder='
Question, unrelated to this CL:

Do you know where "results.json" is produced and uploaded and used? It looks
like it's used and stored by test-results.appspot.com (
http://test-results.appspot.com/testfile), but not output directly by
run-webkit-tests.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py:240:
# Distinct results as non-encoded strings.
Besides this comment, there are a couple ways that I think this could be made
clearer:

1. Add a simple unit test for this method with example output.
2. Add a docstring saying something like "Returns a dict mapping test paths to
lists of to non-encoded result strings (e.g. "TEXT", "CRASH", "PASS").

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py
(right):

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:70:
the bots show that it's not flaky.) There's also some rules about when
There's also some rules -> There are also some rules

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:78:
Returns: True if the line can be removed, False otherwise.
To be consistent with the docstrings below, this could be:
    Returns:
        True if the line can be removed, False otherwise.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:88:
return False
Is "PASS" an "unstrippable" expectation? Would it be possible to put "PASS" in
the list of unstrippable expectations and remove _has_pass_expectations?

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:96:
_log.error('Failed to get builder for config [%s, %s, %s]' % (config.version,
config.architecture, config.build_type))
For logging methods, if you give more than one argument it will use string
formatting to format the string for you, e.g.:

_log.error('Failed to get builder for config [%s, %s, %s]',
           config.version, config.architecture, config.build_type)

This is slightly nicer because it allows you to use one less set of parentheses.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:130:
This method would return [Pass Failure]
Maybe the "flowiness" of this section could be improved if it were put in one
paragraph, and made more like prose:

    For example, if the test expectations line is
    "bug(test) fast/test.html [Crash Failure Pass]"
    and the results are ['TEXT', 'PASS', 'PASS', 'TIMEOUT'],
    then this method would return set(['PASS', 'FAIL']).

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:140:
# TODO(bokan): Does this not exist in a more central place?
Good question, it seems like it should, so if it doesn't, then maybe this should
be put somewhere else in a later CL (somewhere in
layout_tests/models/test_expectations.py?)

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:152:
""" Returns whether any of the given expectations are considered unstrippable.
Extra space character can be removed.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:196:
)
This block of code (lines 181-196), which builds up
self.builder_results_by_path, could be extracted to a separate method.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:214:
index = index - 1
`index = index - 1` can also be written as `index -= 1` if you want.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:225:
test_expectations.pop(index)
This block of code (removing associated comments and whitespace directly above
removed expectations takes a while for me to understand -- although I'm
convinced that it works since I've seen the test.

Possible optional changes that might make it easier to understand:

 1. extract to a separate method (which takes index and test_expectations and
modifies test_expectations)?
 2. add more comments?

What do you think about this possible change:

  
def _remove_associated_comments_and_whitespace(expectations, index):
    if not (index == len(expectations) or expectations[index].is_whitespace() or
expectations[index].is_comment()):
        # No whitespace or comments to remove.
        return

    removed_whitespace = False
    while index > 0 and expectations[index - 1].is_whitespace():
        index -= 1
        expectations.pop(index)
        removed_whitespace = True

    if removed_whitespace:
        # No associated comments; whitespace lines are now already removed.
        return
    
    # First remove all comment lines, then remove whitespace lines before those
comments.
    while index > 0 and expectations[index - 1].is_comment():
        index -= 1
        expectations.pop(index)
    while index > 0 and expectations[index - 1].is_whitespace():
        index -= 1
        expectations.pop(index)

[bokan, 2016-07-08 21:13:00]

Thanks for the detailed review, all comments addressed.

FYI: Running the script produces some 404s and warnings about not finding
builder for a given config. That'll have to be investigated and fixed but I'd
like to get this finally landed and I'll follow up.

FWIW the results of running it does remove a set of tests that, checked on the
flakiness dashboard, are indeed all green so I think it's just being overly
conservative in some cases and doesn't know how to skip bots not on the
waterfall.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/builder_list.py
(right):

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/builder_list.py:90: def
builder_name_for_specifiers(self, version, build_type):
On 2016/07/08 17:55:03, qyearsley wrote:
> A docstring here could clarify that version is the OS version specifier (e.g.
> "Win7") and build_type is the build configuration type (e.g. "Debug").

Done.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/builder_list.py:91: for
builder_name, info in self._builders.items():
On 2016/07/08 17:55:03, qyearsley wrote:
> Since dicts are unordered, it's theoretically possible that you could have a
> _builders dict like:
> 
>     {
>         'foo': {'specifiers': ['a', 'b']},
>         'bar': {'specifiers': ['a', 'b']},
>     }
> 
> and then it's not clearly defined whether builder_name_for_specifiers('a',
'b')
> should return 'foo' or 'bar'. This likely won't be a problem, but just to make
> it clearly defined, you could iterate through
`sorted(self._builders.items())`.

Done.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/builder_list.py:93: if
specifiers[0].lower() == version.lower() and specifiers[1].lower() ==
build_type.lower():
On 2016/07/08 17:55:03, qyearsley wrote:
> This is implicitly adding an assumption that specifier is always of the form
> [<OS version>, <build config>].
> 
> I think this is fine, but the comment on line 44 should probably be updated.

Done.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py
(right):

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py:112:
RESULTS_URL_PREFIX =
'http://test-results.appspot.com/testfile?master=ChromiumWebkit&testtype=webkit_tests&name=results-small.json&builder='
On 2016/07/08 17:55:03, qyearsley wrote:
> Question, unrelated to this CL:
> 
> Do you know where "results.json" is produced and uploaded and used? It looks
> like it's used and stored by http://test-results.appspot.com (
> http://test-results.appspot.com/testfile), but not output directly by
> run-webkit-tests.

Sorry, I don't. Ojan or someone more familiar with this code might know.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py:240:
# Distinct results as non-encoded strings.
On 2016/07/08 17:55:03, qyearsley wrote:
> Besides this comment, there are a couple ways that I think this could be made
> clearer:
> 
> 1. Add a simple unit test for this method with example output.
> 2. Add a docstring saying something like "Returns a dict mapping test paths to
> lists of to non-encoded result strings (e.g. "TEXT", "CRASH", "PASS").

Done.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py
(right):

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:70:
the bots show that it's not flaky.) There's also some rules about when
On 2016/07/08 17:55:03, qyearsley wrote:
> There's also some rules -> There are also some rules

Done.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:78:
Returns: True if the line can be removed, False otherwise.
On 2016/07/08 17:55:04, qyearsley wrote:
> To be consistent with the docstrings below, this could be:
>     Returns:
>         True if the line can be removed, False otherwise.

Done.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:88:
return False
On 2016/07/08 17:55:03, qyearsley wrote:
> Is "PASS" an "unstrippable" expectation? Would it be possible to put "PASS" in
> the list of unstrippable expectations and remove _has_pass_expectations?

No, it's a bit subtle but has_unstrippable_expectations is saying that that line
shouldn't even be looked at (e.g. it has a NeedsRebaseline).

The second condition here is making sure that the line is actually flaky. That
is, we don't want to look at lines that are known to be failing (that seems like
an obvious extension for the future but this script is scoped just for flakiness
for now).

I've added comments here to clarify.

If we made pass unstrippable this script wouldn't do anything :)

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:96:
_log.error('Failed to get builder for config [%s, %s, %s]' % (config.version,
config.architecture, config.build_type))
On 2016/07/08 17:55:03, qyearsley wrote:
> For logging methods, if you give more than one argument it will use string
> formatting to format the string for you, e.g.:
> 
> _log.error('Failed to get builder for config [%s, %s, %s]',
>            config.version, config.architecture, config.build_type)
> 
> This is slightly nicer because it allows you to use one less set of
parentheses.

Done.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:130:
This method would return [Pass Failure]
On 2016/07/08 17:55:03, qyearsley wrote:
> Maybe the "flowiness" of this section could be improved if it were put in one
> paragraph, and made more like prose:
> 
>     For example, if the test expectations line is
>     "bug(test) fast/test.html [Crash Failure Pass]"
>     and the results are ['TEXT', 'PASS', 'PASS', 'TIMEOUT'],
>     then this method would return set(['PASS', 'FAIL']).

Done.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:140:
# TODO(bokan): Does this not exist in a more central place?
On 2016/07/08 17:55:03, qyearsley wrote:
> Good question, it seems like it should, so if it doesn't, then maybe this
should
> be put somewhere else in a later CL (somewhere in
> layout_tests/models/test_expectations.py?)

Ok, I'll leave this TODO here for now and either find an existing method or add
one in a followup.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:152:
""" Returns whether any of the given expectations are considered unstrippable.
On 2016/07/08 17:55:04, qyearsley wrote:
> Extra space character can be removed.

Done.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:196:
)
On 2016/07/08 17:55:04, qyearsley wrote:
> This block of code (lines 181-196), which builds up
> self.builder_results_by_path, could be extracted to a separate method.

Done.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:214:
index = index - 1
On 2016/07/08 17:55:03, qyearsley wrote:
> `index = index - 1` can also be written as `index -= 1` if you want.

Done.

https://codereview.chromium.org/2125633002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:225:
test_expectations.pop(index)
On 2016/07/08 17:55:03, qyearsley wrote:
> This block of code (removing associated comments and whitespace directly above
> removed expectations takes a while for me to understand -- although I'm
> convinced that it works since I've seen the test.
> 
> Possible optional changes that might make it easier to understand:
> 
>  1. extract to a separate method (which takes index and test_expectations and
> modifies test_expectations)?
>  2. add more comments?
> 
> What do you think about this possible change:
> 
>   
> def _remove_associated_comments_and_whitespace(expectations, index):
>     if not (index == len(expectations) or expectations[index].is_whitespace()
or
> expectations[index].is_comment()):
>         # No whitespace or comments to remove.
>         return
> 
>     removed_whitespace = False
>     while index > 0 and expectations[index - 1].is_whitespace():
>         index -= 1
>         expectations.pop(index)
>         removed_whitespace = True
> 
>     if removed_whitespace:
>         # No associated comments; whitespace lines are now already removed.
>         return
>     
>     # First remove all comment lines, then remove whitespace lines before
those
> comments.
>     while index > 0 and expectations[index - 1].is_comment():
>         index -= 1
>         expectations.pop(index)
>     while index > 0 and expectations[index - 1].is_whitespace():
>         index -= 1
>         expectations.pop(index)

Done.

[qyearsley, 2016-07-08 21:20:55]

Excellent, LGTM

[bokan, 2016-07-08 21:21:20]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-07-08 21:21:48]

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

[bokan, 2016-07-08 21:23:08]

Description was changed from

==========
[update_test_expectations.py 3/3] Fill out implementation of
UpdateTestExpectations

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

to

==========
Fill out implementation of UpdateTestExpectations script

In r404402 I created the outline of the UpdateTestExpectations script that
removes lines from TestExpectations that are no longer flaky according to
results on the build waterfall. This CL actually implements the functionality
in the script and makes it basically functional.

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

[commit-bot: I haz the power, 2016-07-08 22:35:27]

Description was changed from

==========
Fill out implementation of UpdateTestExpectations script

In r404402 I created the outline of the UpdateTestExpectations script that
removes lines from TestExpectations that are no longer flaky according to
results on the build waterfall. This CL actually implements the functionality
in the script and makes it basically functional.

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

to

==========
Fill out implementation of UpdateTestExpectations script

In r404402 I created the outline of the UpdateTestExpectations script that
removes lines from TestExpectations that are no longer flaky according to
results on the build waterfall. This CL actually implements the functionality
in the script and makes it basically functional.

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

[commit-bot: I haz the power, 2016-07-08 22:35:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-08 22:35:47]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 22:36:50]

Description was changed from

==========
Fill out implementation of UpdateTestExpectations script

In r404402 I created the outline of the UpdateTestExpectations script that
removes lines from TestExpectations that are no longer flaky according to
results on the build waterfall. This CL actually implements the functionality
in the script and makes it basically functional.

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

to

==========
Fill out implementation of UpdateTestExpectations script

In r404402 I created the outline of the UpdateTestExpectations script that
removes lines from TestExpectations that are no longer flaky according to
results on the build waterfall. This CL actually implements the functionality
in the script and makes it basically functional.

BUG=595414

Committed: https://crrev.com/c7dd0560d9544a15908239bebc177410899851ca
Cr-Commit-Position: refs/heads/master@{#404515}
==========

[commit-bot: I haz the power, 2016-07-08 22:36:53]

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