Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3001163002:
Pinpoint - Surface info from executions for display in UI. (Closed)
DescriptionPinpoint - Surface info from executions for display in UI.
BUG=catapult:#3334
Review-Url: https://codereview.chromium.org/3001163002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/e8b2798057d55b2a5ecab0b39b97ae1ca3da473a
Patch Set 1 #
Total comments: 15
Patch Set 2 : Addressed comments. #
Total comments: 2
Patch Set 3 : Addressed comments. #
Total comments: 4
Patch Set 4 : Addressed comments. #Patch Set 5 : Rebased. #Patch Set 6 : Rebase again. #Messages
Total messages: 53 (37 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by simonhatch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
simonhatch@chromium.org changed reviewers: + dtu@chromium.org
Description was changed from ========== Pinpoint - Surface info from executions for display in UI. BUG=catapult:#3334 ========== to ========== Pinpoint - Surface info from executions for display in UI. BUG=catapult:#3334 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...) Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...) Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
The CQ bit was checked by simonhatch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... File dashboard/dashboard/pinpoint/models/attempt.py (right): https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/attempt.py:51: def AsDictPerQuest(self): nit: put methods after properties https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/attempt.py:51: def AsDictPerQuest(self): It would be more consistent to provide an Attempt.AsDict() that gives 'executions' as a flat list. https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... File dashboard/dashboard/pinpoint/models/job.py (right): https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/job.py:273: execution_details = [] I don't see a clear advantage of this format over a list of attempts for each Change. {'attempts': [[attempt.AsDict() for attempt in self._attempts[change]] for change in self._changes]} result_values is aggregated in this way to provide a flat list for reach (Change, Quest), but it's not clear we can/should do that for every piece of data in execution_details. https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/job.py:289: 'execution_details': execution_details, nit: executions https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... File dashboard/dashboard/pinpoint/models/quest/execution.py (right): https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/quest/execution.py:68: def AsDict(self): Reason I wanted this is so we could expose result_values through here. We can also include result_arguments here. https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/quest/execution.py:72: return {} raise NotImplementedError() https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... File dashboard/dashboard/pinpoint/models/quest/find_isolate.py (right): https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/quest/find_isolate.py:59: self._isolate_hash = isolate_hash Not a big fan of adding additional fields for this. The isolate hash is available in result_values and we can expose that. https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... File dashboard/dashboard/pinpoint/models/quest/run_test.py (right): https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/quest/run_test.py:91: self._output_isolate_hash = '' Similar thought as above. We can expose this by exposing result_arguments.
dtu@chromium.org changed reviewers: + perezju@chromium.org
https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... File dashboard/dashboard/pinpoint/models/attempt.py (right): https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/attempt.py:51: def AsDictPerQuest(self): On 2017/08/21 23:52:52, dtu wrote: > nit: put methods after properties Done. https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/attempt.py:51: def AsDictPerQuest(self): On 2017/08/21 23:52:52, dtu wrote: > It would be more consistent to provide an Attempt.AsDict() that gives > 'executions' as a flat list. Done. https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... File dashboard/dashboard/pinpoint/models/job.py (right): https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/job.py:273: execution_details = [] On 2017/08/21 23:52:52, dtu wrote: > I don't see a clear advantage of this format over a list of attempts for each > Change. > {'attempts': [[attempt.AsDict() for attempt in self._attempts[change]] for > change in self._changes]} > > result_values is aggregated in this way to provide a flat list for reach > (Change, Quest), but it's not clear we can/should do that for every piece of > data in execution_details. Done. https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... File dashboard/dashboard/pinpoint/models/quest/execution.py (right): https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/quest/execution.py:68: def AsDict(self): On 2017/08/21 23:52:52, dtu wrote: > Reason I wanted this is so we could expose result_values through here. > > We can also include result_arguments here. Done. https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/quest/execution.py:72: return {} On 2017/08/21 23:52:53, dtu wrote: > raise NotImplementedError() Done. https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... File dashboard/dashboard/pinpoint/models/quest/find_isolate.py (right): https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/quest/find_isolate.py:59: self._isolate_hash = isolate_hash On 2017/08/21 23:52:53, dtu wrote: > Not a big fan of adding additional fields for this. > The isolate hash is available in result_values and we can expose that. Removed. https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... File dashboard/dashboard/pinpoint/models/quest/run_test.py (right): https://codereview.chromium.org/3001163002/diff/80001/dashboard/dashboard/pin... dashboard/dashboard/pinpoint/models/quest/run_test.py:91: self._output_isolate_hash = '' On 2017/08/21 23:52:53, dtu wrote: > Similar thought as above. We can expose this by exposing result_arguments. Done.
The CQ bit was checked by simonhatch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/nits https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... File dashboard/dashboard/pinpoint/models/quest/find_isolate.py (right): https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... dashboard/dashboard/pinpoint/models/quest/find_isolate.py:50: 'build': self._build or '', nit: maybe it's fine to return None if the build is not set? https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... File dashboard/dashboard/pinpoint/models/quest/run_test.py (right): https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... dashboard/dashboard/pinpoint/models/quest/run_test.py:102: 'bot_id': self._bot_id or '', nit: ditto
On 2017/08/22 09:49:48, perezju wrote: > lgtm w/nits > > https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... > File dashboard/dashboard/pinpoint/models/quest/find_isolate.py (right): > > https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... > dashboard/dashboard/pinpoint/models/quest/find_isolate.py:50: 'build': > self._build or '', > nit: maybe it's fine to return None if the build is not set? > > https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... > File dashboard/dashboard/pinpoint/models/quest/run_test.py (right): > > https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... > dashboard/dashboard/pinpoint/models/quest/run_test.py:102: 'bot_id': > self._bot_id or '', > nit: ditto Isn't None an invalid JSON value? and this is all json.dumps'd for the UI.
On 2017/08/22 12:34:47, shatch wrote: > On 2017/08/22 09:49:48, perezju wrote: > > lgtm w/nits > > > > > https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... > > File dashboard/dashboard/pinpoint/models/quest/find_isolate.py (right): > > > > > https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... > > dashboard/dashboard/pinpoint/models/quest/find_isolate.py:50: 'build': > > self._build or '', > > nit: maybe it's fine to return None if the build is not set? > > > > > https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... > > File dashboard/dashboard/pinpoint/models/quest/run_test.py (right): > > > > > https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... > > dashboard/dashboard/pinpoint/models/quest/run_test.py:102: 'bot_id': > > self._bot_id or '', > > nit: ditto > > Isn't None an invalid JSON value? and this is all json.dumps'd for the UI. Nope, "None" becomes javascript "null" >>> json.dumps({'a': None}) '{"a": null}'
On 2017/08/22 12:39:23, perezju wrote: > On 2017/08/22 12:34:47, shatch wrote: > > On 2017/08/22 09:49:48, perezju wrote: > > > lgtm w/nits > > > > > > > > > https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... > > > File dashboard/dashboard/pinpoint/models/quest/find_isolate.py (right): > > > > > > > > > https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... > > > dashboard/dashboard/pinpoint/models/quest/find_isolate.py:50: 'build': > > > self._build or '', > > > nit: maybe it's fine to return None if the build is not set? > > > > > > > > > https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... > > > File dashboard/dashboard/pinpoint/models/quest/run_test.py (right): > > > > > > > > > https://codereview.chromium.org/3001163002/diff/100001/dashboard/dashboard/pi... > > > dashboard/dashboard/pinpoint/models/quest/run_test.py:102: 'bot_id': > > > self._bot_id or '', > > > nit: ditto > > > > Isn't None an invalid JSON value? and this is all json.dumps'd for the UI. > > Nope, "None" becomes javascript "null" > > >>> json.dumps({'a': None}) > '{"a": null}' Oops you're right! Changed those to return None.
The CQ bit was checked by simonhatch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with comments https://codereview.chromium.org/3001163002/diff/120001/dashboard/dashboard/pi... File dashboard/dashboard/pinpoint/models/attempt.py (right): https://codereview.chromium.org/3001163002/diff/120001/dashboard/dashboard/pi... dashboard/dashboard/pinpoint/models/attempt.py:56: return [e.AsDict() for e in self._executions] {'executions': [foo]} https://codereview.chromium.org/3001163002/diff/120001/dashboard/dashboard/pi... File dashboard/dashboard/pinpoint/models/quest/execution.py (right): https://codereview.chromium.org/3001163002/diff/120001/dashboard/dashboard/pi... dashboard/dashboard/pinpoint/models/quest/execution.py:71: 'result_values': self.result_values if self.completed else [] nit: else null
The CQ bit was checked by simonhatch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/3001163002/diff/120001/dashboard/dashboard/pi... File dashboard/dashboard/pinpoint/models/attempt.py (right): https://codereview.chromium.org/3001163002/diff/120001/dashboard/dashboard/pi... dashboard/dashboard/pinpoint/models/attempt.py:56: return [e.AsDict() for e in self._executions] On 2017/08/22 16:46:19, dtu wrote: > {'executions': [foo]} Done. https://codereview.chromium.org/3001163002/diff/120001/dashboard/dashboard/pi... File dashboard/dashboard/pinpoint/models/quest/execution.py (right): https://codereview.chromium.org/3001163002/diff/120001/dashboard/dashboard/pi... dashboard/dashboard/pinpoint/models/quest/execution.py:71: 'result_values': self.result_values if self.completed else [] On 2017/08/22 16:46:19, dtu wrote: > nit: else null Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by simonhatch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, dtu@chromium.org Link to the patchset: https://codereview.chromium.org/3001163002/#ps140001 (title: "Addressed comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
The CQ bit was checked by simonhatch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, dtu@chromium.org Link to the patchset: https://codereview.chromium.org/3001163002/#ps160001 (title: "Rebased.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...) Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...) Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)
The CQ bit was checked by simonhatch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, dtu@chromium.org Link to the patchset: https://codereview.chromium.org/3001163002/#ps180001 (title: "Rebase again.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1503424333060490,
"parent_rev": "10b772bb112d592fd2f393af628c2195bb93e0fe", "commit_rev":
"e8b2798057d55b2a5ecab0b39b97ae1ca3da473a"}
Message was sent while issue was closed.
Description was changed from ========== Pinpoint - Surface info from executions for display in UI. BUG=catapult:#3334 ========== to ========== Pinpoint - Surface info from executions for display in UI. BUG=catapult:#3334 Review-Url: https://codereview.chromium.org/3001163002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
