Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2544183002:
Make the atrace agent use the correct version of ADB. (Closed)
|
Created:
4 years ago by alexandermont Modified:
4 years ago CC:
catapult-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionMake the atrace agent use the correct version of ADB.
Review-Url: https://codereview.chromium.org/2544183002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/2e579931b25aae856f00ec7b54fa66e1326e4772
Patch Set 1 #Patch Set 2 : fix atrace agent #
Total comments: 2
Patch Set 3 : remove extraneous test #
Messages
Total messages: 36 (11 generated)
alexandermont@chromium.org changed reviewers: + rnephew@chromium.org
On 2016/12/02 00:46:45, alexandermont wrote: I'm actually not an owner here, adding owner.
rnephew@chromium.org changed reviewers: + ccraik@google.com
On 2016/12/02 01:10:28, rnephew (Reviews Here) wrote: Ping
lgtm
On 2016/12/06 at 18:02:18, Chris Craik wrote: > lgtm Sorry about that, missed this first time round
The CQ bit was checked by rnephew@chromium.org
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...)
On 2016/12/06 18:32:58, commit-bot: I haz the power wrote: > 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...) I think the unit tests need to be fixed to reflect these changes still.
On 2016/12/06 18:53:17, rnephew (Reviews Here) wrote: > On 2016/12/06 18:32:58, commit-bot: I haz the power wrote: > > 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...) > > I think the unit tests need to be fixed to reflect these changes still. Alex, can you fix the tests on this and get it landed? It is possibly causing issues with TBMv2 benchmarks that have atrace enabled.
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ccraik@google.com Link to the patchset: https://codereview.chromium.org/2544183002/#ps20001 (title: "fix atrace agent")
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...)
https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... File systrace/systrace/tracing_agents/atrace_agent.py (right): https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... systrace/systrace/tracing_agents/atrace_agent.py:270: if self._device_utils: Do we ever expect to not have this except in the unit tests? If we always expect it to be there, it might be better to mock it out in the unit test.
https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... File systrace/systrace/tracing_agents/atrace_agent.py (right): https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... systrace/systrace/tracing_agents/atrace_agent.py:270: if self._device_utils: On 2016/12/09 at 20:42:55, rnephew (Reviews Here) wrote: > Do we ever expect to not have this except in the unit tests? If we always expect it to be there, it might be better to mock it out in the unit test. Why isn't it present in unit tests?
On 2016/12/09 at 21:35:42, ccraik wrote: > https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... > File systrace/systrace/tracing_agents/atrace_agent.py (right): > > https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... > systrace/systrace/tracing_agents/atrace_agent.py:270: if self._device_utils: > On 2016/12/09 at 20:42:55, rnephew (Reviews Here) wrote: > > Do we ever expect to not have this except in the unit tests? If we always expect it to be there, it might be better to mock it out in the unit test. > > Why isn't it present in unit tests? Because there isn't an actual phone that you're running it on in unit tests.
On 2016/12/09 at 21:45:46, alexandermont wrote: > On 2016/12/09 at 21:35:42, ccraik wrote: > > https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... > > File systrace/systrace/tracing_agents/atrace_agent.py (right): > > > > https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... > > systrace/systrace/tracing_agents/atrace_agent.py:270: if self._device_utils: > > On 2016/12/09 at 20:42:55, rnephew (Reviews Here) wrote: > > > Do we ever expect to not have this except in the unit tests? If we always expect it to be there, it might be better to mock it out in the unit test. > > > > Why isn't it present in unit tests? > > Because there isn't an actual phone that you're running it on in unit tests. Which unit test is hitting this, and from which entry point? Atrace agent doesn't want to run when you don't have a device connected (given the failure to create when from_file = true), so it make make more sense as rnephew says to mock it, or to only run the test when a device is available (downside there is we may lose mac/windows coverage).
On 2016/12/09 at 20:42:55, rnephew wrote: > https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... > File systrace/systrace/tracing_agents/atrace_agent.py (right): > > https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... > systrace/systrace/tracing_agents/atrace_agent.py:270: if self._device_utils: > Do we ever expect to not have this except in the unit tests? If we always expect it to be there, it might be better to mock it out in the unit test. I don't think we can really just "mock it out" in the unit tests, because the problem is that self._device_utils == None, since self._device_utils is normally set in StartAgentTracing and the test doesn't call StartAgentTracing, so when you call self._device_utils.RunShellCommand it says self._device_utils is undefined. To be able to "mock it out", you would have to change the interface to allow a test to pass in a mocked version of _device_utils rather than creating the _device_utils internally to the AtraceAgent class. This seems like a larger disruption to the AtraceAgent code than just adding the if-statement like I did here.
On 2016/12/09 at 21:54:17, alexandermont wrote: > On 2016/12/09 at 20:42:55, rnephew wrote: > > https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... > > File systrace/systrace/tracing_agents/atrace_agent.py (right): > > > > https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... > > systrace/systrace/tracing_agents/atrace_agent.py:270: if self._device_utils: > > Do we ever expect to not have this except in the unit tests? If we always expect it to be there, it might be better to mock it out in the unit test. > > I don't think we can really just "mock it out" in the unit tests, because the problem is that self._device_utils == None, since self._device_utils > is normally set in StartAgentTracing and the test doesn't call StartAgentTracing, so when you call self._device_utils.RunShellCommand it says > self._device_utils is undefined. To be able to "mock it out", you would have to change the interface to allow a test to pass in a mocked version > of _device_utils rather than creating the _device_utils internally to the AtraceAgent class. This seems like a larger disruption to the AtraceAgent > code than just adding the if-statement like I did here. Which test, and from which entry point? Is that entry point justified? This if equates to "if(inTestingMode)", since we should never hit it in practice outside of tests, which is something we should try to avoid.
On 2016/12/09 at 22:18:27, ccraik wrote: > On 2016/12/09 at 21:54:17, alexandermont wrote: > > On 2016/12/09 at 20:42:55, rnephew wrote: > > > https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... > > > File systrace/systrace/tracing_agents/atrace_agent.py (right): > > > > > > https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... > > > systrace/systrace/tracing_agents/atrace_agent.py:270: if self._device_utils: > > > Do we ever expect to not have this except in the unit tests? If we always expect it to be there, it might be better to mock it out in the unit test. > > > > I don't think we can really just "mock it out" in the unit tests, because the problem is that self._device_utils == None, since self._device_utils > > is normally set in StartAgentTracing and the test doesn't call StartAgentTracing, so when you call self._device_utils.RunShellCommand it says > > self._device_utils is undefined. To be able to "mock it out", you would have to change the interface to allow a test to pass in a mocked version > > of _device_utils rather than creating the _device_utils internally to the AtraceAgent class. This seems like a larger disruption to the AtraceAgent > > code than just adding the if-statement like I did here. > > Which test, and from which entry point? Is that entry point justified? This if equates to "if(inTestingMode)", since we should never hit it in practice outside of tests, which is something we should try to avoid. It's in test_preprocess_trace_data, which creates an AtraceAgent() and then calls atrace._preprocess_test_data() on it. Maybe the whole way this test is set up is questionable, since _preprocess_test_data is private to AtraceAgent, so maybe we shouldn't be calling it directly, even from a test?
On 2016/12/09 at 22:25:16, alexandermont wrote: > On 2016/12/09 at 22:18:27, ccraik wrote: > > On 2016/12/09 at 21:54:17, alexandermont wrote: > > > On 2016/12/09 at 20:42:55, rnephew wrote: > > > > https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... > > > > File systrace/systrace/tracing_agents/atrace_agent.py (right): > > > > > > > > https://codereview.chromium.org/2544183002/diff/20001/systrace/systrace/traci... > > > > systrace/systrace/tracing_agents/atrace_agent.py:270: if self._device_utils: > > > > Do we ever expect to not have this except in the unit tests? If we always expect it to be there, it might be better to mock it out in the unit test. > > > > > > I don't think we can really just "mock it out" in the unit tests, because the problem is that self._device_utils == None, since self._device_utils > > > is normally set in StartAgentTracing and the test doesn't call StartAgentTracing, so when you call self._device_utils.RunShellCommand it says > > > self._device_utils is undefined. To be able to "mock it out", you would have to change the interface to allow a test to pass in a mocked version > > > of _device_utils rather than creating the _device_utils internally to the AtraceAgent class. This seems like a larger disruption to the AtraceAgent > > > code than just adding the if-statement like I did here. > > > > Which test, and from which entry point? Is that entry point justified? This if equates to "if(inTestingMode)", since we should never hit it in practice outside of tests, which is something we should try to avoid. > > It's in test_preprocess_trace_data, which creates an AtraceAgent() and then calls atrace._preprocess_test_data() on it. Maybe the whole way this test is set up is questionable, since _preprocess_test_data is private to AtraceAgent, so maybe we shouldn't be calling it directly, even from a test? Testing a private method is potentially fine, but that's trying to be a host-only test while potentially accessing a device. Can you just switch the test to call strip_and_decompress_trace? That seems to be all it wants to validate anyway.
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ccraik@google.com Link to the patchset: https://codereview.chromium.org/2544183002/#ps40001 (title: "remove extraneous test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/19 at 18:01:03, alexandermont wrote: > It looks like there already is a test that tests strip_and_decompress_trace, so I just removed the test_preprocess_trace_data test.
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482170463086200,
"parent_rev": "7ca359550f9531a4c873dd071c73dcc7130eb537", "commit_rev":
"2e579931b25aae856f00ec7b54fa66e1326e4772"}
Message was sent while issue was closed.
Description was changed from ========== Make the atrace agent use the correct version of ADB. ========== to ========== Make the atrace agent use the correct version of ADB. Review-Url: https://codereview.chromium.org/2544183002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
