In rebaseline, include build number information with "test_prefix_list" dicts.

In rebaseline, include build number information with "test_prefix_list" dicts.

When rebaselining, the main argument passed to the rebaseline method is a "test_prefix_list", which is currently a dict mapping test prefixes to builder names to baseline file suffixes.

If we want to change _suffixes_for_actual_failures to fetch actual failures for try jobs, then that method needs to know the build number as well.

This CL would replace the builder in all "test_prefix_list" dicts with a Build object, which includes builder name and build number, but build number is optional.

Planned follow-up: changing build_data() to use a method to fetch and cache results for particular builds, and change _suffixes_for_actual_failures fetch results for particular builds using that method as well. This would make the "skip_checking_actual_results" workaround unnecessary. I could also include that change in this CL.

Committed: https://crrev.com/e5830ddcf7cc0fc87decede44a0a0baa4d5e8c61
Cr-Commit-Position: refs/heads/master@{#405783}

[qyearsley, 2016-07-14 00:22:58]

Description was changed from

==========
Make all places with builder in test_prefix_list into a build pair.
==========

to

==========
In rebaseline, include build number information with "test_prefix_list" dicts.

When rebaselining, the main argument passed to the rebaseline method is a
"test_prefix_list", which is currently a dict mapping test prefixes to builder
names to baseline file suffixes.

If we want to change _suffixes_for_actual_failures to fetch actual failures for
try jobs, then that method needs to know the build number as well.

This CL would replace the builder in all "test_prefix_list" dicts with a
(builder, build number) pair, where None for the build number indicates that the
latest build should be used.


If this change is OK, I wanted to follow up by changing build_data() to use a
method to fetch and cache results for particular builds, and change
_suffixes_for_actual_failures fetch results for particular builds using that
method as well. This would make the "skip_checking_actual_results" workaround
unnecessary. I could also include that change in this CL.


Is there any good way to make this clearer? Does this change seem OK overall?
==========

[qyearsley, 2016-07-14 00:22:58]

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

[wkorman, 2016-07-14 01:01:46]

I'm supportive of small stepwise iterative changes so if you'd like to land this
and do the subsequent work you're describing that's ok with me.

CL description should be updated to remove questions as resolved.

Only a couple of questions.

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
(left):

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:290: def
builder_data(self):
Helpful if we document what this returns key/value wise.

In general at the point where we're passing around a dict whose key is a tuple,
we could consider using a separate object to store things so that we can just
use a method on that object that takes the two params, and the 'None' one can
just be the default value.

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
(right):

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:686:
print(test_prefix_list)
Debugging

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs_unittest.py
(right):

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs_unittest.py:54:
self.maxDiff = None
Why do we need this?

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py
(right):

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py:366:
self.command._rebaseline(options, {"userscripts/first-test.html": {("MOCK Win7",
None): ["txt", "png"]}})
Do we have unit tests where we pass something other than None?

[qyearsley, 2016-07-14 23:53:29]

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
(left):

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:290: def
builder_data(self):
On 2016/07/14 at 01:01:45, wkorman wrote:
> Helpful if we document what this returns key/value wise.
> 
> In general at the point where we're passing around a dict whose key is a
tuple, we could consider using a separate object to store things so that we can
just use a method on that object that takes the two params, and the 'None' one
can just be the default value.

Done -- now added a docstring here, and now instead of a pair, this code uses
"Build" objects, which have two attributes, with a default value for
build_number. How does that seem?

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
(right):

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:686:
print(test_prefix_list)
On 2016/07/14 at 01:01:46, wkorman wrote:
> Debugging

Now removed

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs_unittest.py
(right):

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs_unittest.py:54:
self.maxDiff = None
On 2016/07/14 at 01:01:46, wkorman wrote:
> Why do we need this?

We don't, this was also for debugging. (This attribute makes it so that the
unittest runner prints full diffs when assertEqual fails.)

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py
(right):

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py:366:
self.command._rebaseline(options, {"userscripts/first-test.html": {("MOCK Win7",
None): ["txt", "png"]}})
On 2016/07/14 at 01:01:46, wkorman wrote:
> Do we have unit tests where we pass something other than None?

rebaseline_with_try_jobs_unittest indirectly calls _rebaseline with a build
number other than None, so that should catch if there's anything terribly wrong
that happens when a build number -- but there's no test that directly tests
where the results come from and how rebaseline-test-internal is called when
different Build objects are passed in -- this might be a good idea (if not in
this CL, then in a follow-up).

[wkorman, 2016-07-15 00:12:14]

lgtm

Reminder to also update the CL description.

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py
(right):

https://codereview.chromium.org/2144873004/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py:366:
self.command._rebaseline(options, {"userscripts/first-test.html": {("MOCK Win7",
None): ["txt", "png"]}})
On 2016/07/14 at 23:53:29, qyearsley wrote:
> On 2016/07/14 at 01:01:46, wkorman wrote:
> > Do we have unit tests where we pass something other than None?
> 
> rebaseline_with_try_jobs_unittest indirectly calls _rebaseline with a build
number other than None, so that should catch if there's anything terribly wrong
that happens when a build number -- but there's no test that directly tests
where the results come from and how rebaseline-test-internal is called when
different Build objects are passed in -- this might be a good idea (if not in
this CL, then in a follow-up).

It does seem worth having a test case in rebaseline_unittest that uses a Build
object with a different build number, can add TODO if you'd prefer. Also perhaps
basic tests for Build object itself.

https://codereview.chromium.org/2144873004/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
(right):

https://codereview.chromium.org/2144873004/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:58:
TODO(qyearsley): Move this somewhere else; note it's very similar to TryJob,
There is also already a Build class:

https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

Maybe that's fine, I considered alternate names and nothing great came to mind.

https://codereview.chromium.org/2144873004/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:456:
builders_to_fetch_from = self._builders_to_fetch_from([b.builder_name for b in
test_prefix_list[test]])
Could be worth a helper method for the list comprehension used here and
elsewhere:

[b.builder_name for b in test_prefix_list[test]]

https://codereview.chromium.org/2144873004/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:590:
build: A pair (builder_name, build_number).
Is the description here up to date re: Build class?

[qyearsley, 2016-07-15 15:25:35]

Description was changed from

==========
In rebaseline, include build number information with "test_prefix_list" dicts.

When rebaselining, the main argument passed to the rebaseline method is a
"test_prefix_list", which is currently a dict mapping test prefixes to builder
names to baseline file suffixes.

If we want to change _suffixes_for_actual_failures to fetch actual failures for
try jobs, then that method needs to know the build number as well.

This CL would replace the builder in all "test_prefix_list" dicts with a
(builder, build number) pair, where None for the build number indicates that the
latest build should be used.


If this change is OK, I wanted to follow up by changing build_data() to use a
method to fetch and cache results for particular builds, and change
_suffixes_for_actual_failures fetch results for particular builds using that
method as well. This would make the "skip_checking_actual_results" workaround
unnecessary. I could also include that change in this CL.


Is there any good way to make this clearer? Does this change seem OK overall?
==========

to

==========
In rebaseline, include build number information with "test_prefix_list" dicts.

When rebaselining, the main argument passed to the rebaseline method is a
"test_prefix_list", which is currently a dict mapping test prefixes to builder
names to baseline file suffixes.

If we want to change _suffixes_for_actual_failures to fetch actual failures for
try jobs, then that method needs to know the build number as well.

This CL would replace the builder in all "test_prefix_list" dicts with a Build
object, which includes builder name and build number, but build number is
optional.


Planned follow-up: changing build_data() to use a method to fetch and cache
results for particular builds, and change _suffixes_for_actual_failures fetch
results for particular builds using that method as well. This would make the
"skip_checking_actual_results" workaround unnecessary. I could also include that
change in this CL.
==========

[qyearsley, 2016-07-15 16:28:47]

https://codereview.chromium.org/2144873004/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
(right):

https://codereview.chromium.org/2144873004/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:58:
TODO(qyearsley): Move this somewhere else; note it's very similar to TryJob,
On 2016/07/15 at 00:12:14, wkorman wrote:
> There is also already a Build class:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> 
> Maybe that's fine, I considered alternate names and nothing great came to
mind.

Yesterday after looking at the buildbot module, I was thinking about how it 
could be refactored, and the result was http://crrev.com/2152663003.

One possible way to refactor this would be:
  - Move this Build class to buildbot.
  - In Rietveld, use buildbot.Build instead of TryJob.

I'd like to do this after deciding whether to commit
http://crrev.com/2152663003.

https://codereview.chromium.org/2144873004/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:456:
builders_to_fetch_from = self._builders_to_fetch_from([b.builder_name for b in
test_prefix_list[test]])
On 2016/07/15 at 00:12:14, wkorman wrote:
> Could be worth a helper method for the list comprehension used here and
elsewhere:
> 
> [b.builder_name for b in test_prefix_list[test]]

Extracted a little helper method -- but now, after seeing that there are these
"test_prefix_list" dicts used all over this module, which are a little unwieldy,
it seems like it might be nicer to add another class to represent "a set of
baselines to download across different builders", which is what test_prefix_list
represents now.

If that were done, then this could be perhaps be replaced by a method on that
class.

Added a TODO note for a follow-up change.

https://codereview.chromium.org/2144873004/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:590:
build: A pair (builder_name, build_number).
On 2016/07/15 at 00:12:14, wkorman wrote:
> Is the description here up to date re: Build class?

It was not; now updated.

[qyearsley, 2016-07-15 16:29:04]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-07-15 16:29:04]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2144873004/#ps60001
(title: "Update comments; extract helper function; add TODO notes")

[commit-bot: I haz the power, 2016-07-15 16:29:33]

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-07-15 17:42:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-15 17:42:21]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 17:43:13]

Description was changed from

==========
In rebaseline, include build number information with "test_prefix_list" dicts.

When rebaselining, the main argument passed to the rebaseline method is a
"test_prefix_list", which is currently a dict mapping test prefixes to builder
names to baseline file suffixes.

If we want to change _suffixes_for_actual_failures to fetch actual failures for
try jobs, then that method needs to know the build number as well.

This CL would replace the builder in all "test_prefix_list" dicts with a Build
object, which includes builder name and build number, but build number is
optional.


Planned follow-up: changing build_data() to use a method to fetch and cache
results for particular builds, and change _suffixes_for_actual_failures fetch
results for particular builds using that method as well. This would make the
"skip_checking_actual_results" workaround unnecessary. I could also include that
change in this CL.
==========

to

==========
In rebaseline, include build number information with "test_prefix_list" dicts.

When rebaselining, the main argument passed to the rebaseline method is a
"test_prefix_list", which is currently a dict mapping test prefixes to builder
names to baseline file suffixes.

If we want to change _suffixes_for_actual_failures to fetch actual failures for
try jobs, then that method needs to know the build number as well.

This CL would replace the builder in all "test_prefix_list" dicts with a Build
object, which includes builder name and build number, but build number is
optional.

Planned follow-up: changing build_data() to use a method to fetch and cache
results for particular builds, and change _suffixes_for_actual_failures fetch
results for particular builds using that method as well. This would make the
"skip_checking_actual_results" workaround unnecessary. I could also include that
change in this CL.

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

[commit-bot: I haz the power, 2016-07-15 17:43:14]

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

[wjmaclean, 2016-07-15 18:49:03]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2151253004/ by wjmaclean@chromium.org.

The reason for reverting is: Seems to be causing failures in webkit_python_tests
on multiple bots (e.g.
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build....