[Dashboard] Get or create ancestors in add_histograms_queue

[Dashboard] Get or create ancestors in add_histograms_queue

BUG=catapult:#3532

Review-Url: https://codereview.chromium.org/2879453002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/132e8a1e8cd142fe244a3744ffb35a3b0b7a83ee

[eakuefner, 2017-05-16 06:57:15]

Description was changed from

==========
[Dashboard] Get or create ancestors in add_histograms_queue

BUG=catapult:#
==========

to

==========
[Dashboard] Get or create ancestors in add_histograms_queue

BUG=catapult:#3532
==========

[eakuefner, 2017-05-16 06:57:15]

eakuefner@chromium.org changed reviewers:
+ simonhatch@chromium.org

[eakuefner, 2017-05-16 06:58:30]

Simon, please take a look.

[shatch, 2017-05-16 15:47:45]

https://codereview.chromium.org/2879453002/diff/20001/dashboard/dashboard/add...
File dashboard/dashboard/add_histograms_queue.py (right):

https://codereview.chromium.org/2879453002/diff/20001/dashboard/dashboard/add...
dashboard/dashboard/add_histograms_queue.py:10: from dashboard import
add_point_queue
Probably want to move these helpers to a common spot, but I'm fine with that as
a followup later, maybe just add a TODO mentioning it.

https://codereview.chromium.org/2879453002/diff/20001/dashboard/dashboard/add...
dashboard/dashboard/add_histograms_queue.py:63: extra_args = {}
Maybe move this to _GetExtraArgs() or something to that effect?

https://codereview.chromium.org/2879453002/diff/20001/dashboard/dashboard/add...
File dashboard/dashboard/add_histograms_queue_test.py (right):

https://codereview.chromium.org/2879453002/diff/20001/dashboard/dashboard/add...
dashboard/dashboard/add_histograms_queue_test.py:33: def testPost(self):
Add tests covering _GetOrCreateAncestors call, verifying params and also the
extra arg generation?

[eakuefner, 2017-05-19 18:51:48]

Simon, please take another quick look. Adding more asserts for the ancestor
creation actually revealed a bug in add_point_queue, which I also fix and cover
in this CL.

https://codereview.chromium.org/2879453002/diff/20001/dashboard/dashboard/add...
File dashboard/dashboard/add_histograms_queue.py (right):

https://codereview.chromium.org/2879453002/diff/20001/dashboard/dashboard/add...
dashboard/dashboard/add_histograms_queue.py:10: from dashboard import
add_point_queue
On 2017/05/16 at 15:47:44, shatch wrote:
> Probably want to move these helpers to a common spot, but I'm fine with that
as a followup later, maybe just add a TODO mentioning it.

Added TODO. It's true that moving these helpers to their own file will
eventually block deleting add_point_queue.

https://codereview.chromium.org/2879453002/diff/20001/dashboard/dashboard/add...
dashboard/dashboard/add_histograms_queue.py:63: extra_args = {}
On 2017/05/16 at 15:47:44, shatch wrote:
> Maybe move this to _GetExtraArgs() or something to that effect?

Done and renamed to unit_args.

https://codereview.chromium.org/2879453002/diff/20001/dashboard/dashboard/add...
File dashboard/dashboard/add_histograms_queue_test.py (right):

https://codereview.chromium.org/2879453002/diff/20001/dashboard/dashboard/add...
dashboard/dashboard/add_histograms_queue_test.py:33: def testPost(self):
On 2017/05/16 at 15:47:44, shatch wrote:
> Add tests covering _GetOrCreateAncestors call, verifying params and also the
extra arg generation?

Done, by adding more asserts here.

[sh, 2017-05-19 19:10:27]

simonhatch@google.com changed reviewers:
+ simonhatch@google.com

[sh, 2017-05-19 19:10:27]

lgtm

https://codereview.chromium.org/2879453002/diff/80001/dashboard/dashboard/add...
File dashboard/dashboard/add_histograms_queue_test.py (right):

https://codereview.chromium.org/2879453002/diff/80001/dashboard/dashboard/add...
dashboard/dashboard/add_histograms_queue_test.py:37: def testPost(self):
Add a test for a diagnostic?

https://codereview.chromium.org/2879453002/diff/80001/dashboard/dashboard/add...
dashboard/dashboard/add_histograms_queue_test.py:50:
self.assertEqual(test.units, 'count_biggerIsBetter')
Tests for the different units cases?

[shatch, 2017-05-19 19:15:05]

wrong account lgtm

[eakuefner, 2017-05-22 21:22:28]

https://codereview.chromium.org/2879453002/diff/80001/dashboard/dashboard/add...
File dashboard/dashboard/add_histograms_queue_test.py (right):

https://codereview.chromium.org/2879453002/diff/80001/dashboard/dashboard/add...
dashboard/dashboard/add_histograms_queue_test.py:37: def testPost(self):
On 2017/05/19 at 19:10:27, sh wrote:
> Add a test for a diagnostic?

Done.

https://codereview.chromium.org/2879453002/diff/80001/dashboard/dashboard/add...
dashboard/dashboard/add_histograms_queue_test.py:50:
self.assertEqual(test.units, 'count_biggerIsBetter')
On 2017/05/19 at 19:10:27, sh wrote:
> Tests for the different units cases?

Added test coverage for GetUnitArgs directly.

[eakuefner, 2017-05-22 21:22:31]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2017-05-22 21:22:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
simonhatch@chromium.org, simonhatch@google.com
Link to the patchset: https://codereview.chromium.org/2879453002/#ps100001
(title: "Address Simon's last round of comments")

[commit-bot: I haz the power, 2017-05-22 21:22:37]

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, 2017-05-22 21:28:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 21:28:27]

Try jobs failed on following builders:
  Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...)

[eakuefner, 2017-05-22 22:42:57]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2017-05-22 22:42:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
simonhatch@chromium.org, simonhatch@google.com
Link to the patchset: https://codereview.chromium.org/2879453002/#ps120001
(title: "fix failing unit add_point test")

[commit-bot: I haz the power, 2017-05-22 22:43:08]

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, 2017-05-22 23:07:10]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495492977749350,
"parent_rev": "1883fc0d8375fd28d7c1c1315ac1e3701373220b", "commit_rev":
"132e8a1e8cd142fe244a3744ffb35a3b0b7a83ee"}

[commit-bot: I haz the power, 2017-05-22 23:07:13]

Description was changed from

==========
[Dashboard] Get or create ancestors in add_histograms_queue

BUG=catapult:#3532
==========

to

==========
[Dashboard] Get or create ancestors in add_histograms_queue

BUG=catapult:#3532

Review-Url: https://codereview.chromium.org/2879453002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-05-22 23:07:14]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...