Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2736123003:
[Systrace][BattOr] Simplify BattOr options in systrace. (Closed)
|
Created:
3 years, 9 months ago by rnephew (Reviews Here) Modified:
3 years, 9 months ago CC:
catapult-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Systrace][BattOr] Simplify BattOr options in systrace.
Before it had the complete set of options to do automapping from hub1 to hub2
like the labs use, but these options drastically complicate interacting with
systrace and the BattOr with no benefit gained. This does not fix the already
broken BattOr functionality in systrace. That will be done in this cl:
https://codereview.chromium.org/2712163002/
BUG=catapult:#3262
Review-Url: https://codereview.chromium.org/2736123003
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/4226576b1dc7ef7ff04804e62d526d48d0e883e2
Patch Set 1 #
Total comments: 10
Patch Set 2 : charlies comments #
Total comments: 12
Patch Set 3 : [Systrace][BattOr] Simplify BattOr options in systrace. #
Messages
Total messages: 19 (10 generated)
rnephew@chromium.org changed reviewers: + ccraik@google.com, charliea@chromium.org
The CQ bit was checked by rnephew@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.
On 2017/03/08 17:07:20, rnephew (Reviews Here) wrote: Ping.
Code LGTM, though suggest waiting for Charlie, since I don't know much about battor's usage.
https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... File systrace/systrace/tracing_agents/battor_trace_agent.py (right): https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... systrace/systrace/tracing_agents/battor_trace_agent.py:86: RuntimeError: If trace already in progress. Can't this also raise an AssertionError? https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... systrace/systrace/tracing_agents/battor_trace_agent.py:90: battor = config.battor_path This should probably be battor_path just to be as explicit as possible https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... systrace/systrace/tracing_agents/battor_trace_agent.py:92: assert len(battors) == 1, ('Must specify BattOr path if there is not ' Shouldn't there be a unit test to make sure this assert failing? https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... systrace/systrace/tracing_agents/battor_trace_agent.py:94: battor = battors[0] Given how much is already going on in this function, I'd be in favor of moving this out into a separate function: battor_path = _get_battor_path(config) That lets StartAgentTracing focus on the high-level tasks associated with start tracing and not how the BattOrs map to devices https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... File systrace/systrace/tracing_agents/battor_trace_agent_unittest.py (right): https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... systrace/systrace/tracing_agents/battor_trace_agent_unittest.py:19: mock_opts = namedtuple('mock_opts', ['target', 'device_serial_number', Looks like there might be some stuff in this section that needs to be updated?
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... File systrace/systrace/tracing_agents/battor_trace_agent.py (right): https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... systrace/systrace/tracing_agents/battor_trace_agent.py:86: RuntimeError: If trace already in progress. On 2017/03/14 18:35:54, charliea wrote: > Can't this also raise an AssertionError? Done. https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... systrace/systrace/tracing_agents/battor_trace_agent.py:90: battor = config.battor_path On 2017/03/14 18:35:53, charliea wrote: > This should probably be battor_path just to be as explicit as possible Done. https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... systrace/systrace/tracing_agents/battor_trace_agent.py:92: assert len(battors) == 1, ('Must specify BattOr path if there is not ' On 2017/03/14 18:35:54, charliea wrote: > Shouldn't there be a unit test to make sure this assert failing? Done. https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... systrace/systrace/tracing_agents/battor_trace_agent.py:94: battor = battors[0] On 2017/03/14 18:35:54, charliea wrote: > Given how much is already going on in this function, I'd be in favor of moving > this out into a separate function: > > battor_path = _get_battor_path(config) > > That lets StartAgentTracing focus on the high-level tasks associated with start > tracing and not how the BattOrs map to devices Done. https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... File systrace/systrace/tracing_agents/battor_trace_agent_unittest.py (right): https://codereview.chromium.org/2736123003/diff/1/systrace/systrace/tracing_a... systrace/systrace/tracing_agents/battor_trace_agent_unittest.py:19: mock_opts = namedtuple('mock_opts', ['target', 'device_serial_number', On 2017/03/14 18:35:54, charliea wrote: > Looks like there might be some stuff in this section that needs to be updated? Done.
lgtm w/ comments https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... File systrace/systrace/tracing_agents/battor_trace_agent.py (right): https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent.py:79: def _DetermineBattOrPath(config): supernit: It looks like "_FindBattOrPath() might be a little bit more idiomatic name for a function like this https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent.py:99: than one BattOr is attatched. nit: s/attatched/attached https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... File systrace/systrace/tracing_agents/battor_trace_agent_unittest.py (right): https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent_unittest.py:43: if battors is None: supernit: similar to above, s/battors/battor_paths just to be more explicit https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent_unittest.py:160: def test_no_battor(self): Maybe name this test_trace_error_no_battor given the naming scheme of the other tests? (Generally, I'd go with test_start_agent_tracing_no_battor_raises... but I didn't write these originally.) https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent_unittest.py:170: def test_multiple_battor(self): s/battor/battors https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent_unittest.py:170: def test_multiple_battor(self): I don't think this test name adequately describes what behavior the test is testing: specifically, we're trying to make sure that it throws if there are multiple battors attached but with no explicit battor path given. I think something like: test_trace_error_multiple_battors_no_battor_path might be better
https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... File systrace/systrace/tracing_agents/battor_trace_agent.py (right): https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent.py:79: def _DetermineBattOrPath(config): On 2017/03/15 17:30:28, charliea wrote: > supernit: It looks like "_FindBattOrPath() might be a little bit more idiomatic > name for a function like this Done. https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent.py:99: than one BattOr is attatched. On 2017/03/15 17:30:29, charliea wrote: > nit: s/attatched/attached Done. https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... File systrace/systrace/tracing_agents/battor_trace_agent_unittest.py (right): https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent_unittest.py:43: if battors is None: On 2017/03/15 17:30:29, charliea wrote: > supernit: similar to above, s/battors/battor_paths just to be more explicit Done. https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent_unittest.py:160: def test_no_battor(self): On 2017/03/15 17:30:29, charliea wrote: > Maybe name this test_trace_error_no_battor given the naming scheme of the other > tests? > > (Generally, I'd go with test_start_agent_tracing_no_battor_raises... but I > didn't write these originally.) Done. https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent_unittest.py:170: def test_multiple_battor(self): On 2017/03/15 17:30:29, charliea wrote: > I don't think this test name adequately describes what behavior the test is > testing: specifically, we're trying to make sure that it throws if there are > multiple battors attached but with no explicit battor path given. I think > something like: > > test_trace_error_multiple_battors_no_battor_path > > might be better Done. https://codereview.chromium.org/2736123003/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent_unittest.py:170: def test_multiple_battor(self): On 2017/03/15 17:30:29, charliea wrote: > s/battor/battors Done.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ccraik@google.com, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2736123003/#ps60001 (title: "[Systrace][BattOr] Simplify BattOr options in systrace.")
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": 60001, "attempt_start_ts": 1489599562392490,
"parent_rev": "f5d5d1fe8416da7df4a1acc15065f225d01baaf0", "commit_rev":
"4226576b1dc7ef7ff04804e62d526d48d0e883e2"}
Message was sent while issue was closed.
Description was changed from ========== [Systrace][BattOr] Simplify BattOr options in systrace. Before it had the complete set of options to do automapping from hub1 to hub2 like the labs use, but these options drastically complicate interacting with systrace and the BattOr with no benefit gained. This does not fix the already broken BattOr functionality in systrace. That will be done in this cl: https://codereview.chromium.org/2712163002/ BUG=catapult:#3262 ========== to ========== [Systrace][BattOr] Simplify BattOr options in systrace. Before it had the complete set of options to do automapping from hub1 to hub2 like the labs use, but these options drastically complicate interacting with systrace and the BattOr with no benefit gained. This does not fix the already broken BattOr functionality in systrace. That will be done in this cl: https://codereview.chromium.org/2712163002/ BUG=catapult:#3262 Review-Url: https://codereview.chromium.org/2736123003 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
