Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1920023002:
[BattOr] Add real device smoke test (Closed)
|
Created:
4 years, 8 months ago by rnephew (Reviews Here) Modified:
4 years, 7 months ago CC:
catapult-reviews_chromium.org, alexandermont Base URL:
git@github.com:catapult-project/catapult@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[BattOr] Add real device smoke test
BUG=catapult:#2219
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/6581f12987851da2c106914350b22565c589e496
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Make test run. #Patch Set 4 : update binary json #
Total comments: 5
Patch Set 5 : #
Total comments: 3
Patch Set 6 : #
Total comments: 27
Patch Set 7 : #
Total comments: 23
Patch Set 8 : #
Total comments: 4
Patch Set 9 : #
Messages
Total messages: 41 (12 generated)
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com
On 2016/04/25 22:26:51, rnephew1 wrote: Sorry but can you also hook up this test on catapult CQ by making the change to catapult_buil/buid_steps.py?
On 2016/04/25 22:43:16, nednguyen wrote: > On 2016/04/25 22:26:51, rnephew1 wrote: > > Sorry but can you also hook up this test on catapult CQ by making the change to > catapult_buil/buid_steps.py? Done, tryjob running now.
On 2016/04/25 22:58:43, rnephew1 wrote: > On 2016/04/25 22:43:16, nednguyen wrote: > > On 2016/04/25 22:26:51, rnephew1 wrote: > > > > Sorry but can you also hook up this test on catapult CQ by making the change > to > > catapult_buil/buid_steps.py? > > Done, tryjob running now. Looks like the tests are failing
On 2016/04/25 23:28:15, nednguyen wrote:
> On 2016/04/25 22:58:43, rnephew1 wrote:
> > On 2016/04/25 22:43:16, nednguyen wrote:
> > > On 2016/04/25 22:26:51, rnephew1 wrote:
> > >
> > > Sorry but can you also hook up this test on catapult CQ by making the
change
> > to
> > > catapult_buil/buid_steps.py?
> >
> > Done, tryjob running now.
>
> Looks like the tests are failing
Hmmm. Any idea why this third party module wouldn't exist on the linux cq bot?
from serial.tools import list_ports
ImportError: No module named tools
On 2016/04/25 23:51:24, rnephew1 wrote: > On 2016/04/25 23:28:15, nednguyen wrote: > > On 2016/04/25 22:58:43, rnephew1 wrote: > > > On 2016/04/25 22:43:16, nednguyen wrote: > > > > On 2016/04/25 22:26:51, rnephew1 wrote: > > > > > > > > Sorry but can you also hook up this test on catapult CQ by making the > change > > > to > > > > catapult_buil/buid_steps.py? > > > > > > Done, tryjob running now. > > > > Looks like the tests are failing > > Hmmm. Any idea why this third party module wouldn't exist on the linux cq bot? > > from serial.tools import list_ports > ImportError: No module named tools Not really. Does this work on your local linux machine?
https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:19: 'third_party', 'pyserial')) fwiw, if we do this, we should move pyserial to catapult/third_party (in a different CL)
https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... common/battor/battor/battor_wrapper_devicetest.py:66: self.assertTrue(clock_sync_found) self.assertTrue(clock_sync_found, 'Battor data:\n %s' % repr(results)) so we know what went wrong.
> > Hmmm. Any idea why this third party module wouldn't exist on the linux cq bot? > > from serial.tools import list_ports > ImportError: No module named tools Yes, I can get this to work locally on linux. On 2016/04/26 00:12:13, nednguyen wrote: > https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... > File common/battor/battor/battor_wrapper_devicetest.py (right): > > https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... > common/battor/battor/battor_wrapper_devicetest.py:66: > self.assertTrue(clock_sync_found) > self.assertTrue(clock_sync_found, 'Battor data:\n %s' % repr(results)) so we > know what went wrong. That is going to be a _LOT_ of spew.
On 2016/04/26 00:53:34, rnephew1 wrote: > > > > Hmmm. Any idea why this third party module wouldn't exist on the linux cq bot? > > > > from serial.tools import list_ports > > ImportError: No module named tools > > Yes, I can get this to work locally on linux. > > On 2016/04/26 00:12:13, nednguyen wrote: > > > https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... > > File common/battor/battor/battor_wrapper_devicetest.py (right): > > > > > https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... > > common/battor/battor/battor_wrapper_devicetest.py:66: > > self.assertTrue(clock_sync_found) > > self.assertTrue(clock_sync_found, 'Battor data:\n %s' % repr(results)) so we > > know what went wrong. > > That is going to be a _LOT_ of spew. Yeah, but it should only happen on failure, which is the case we want enough (or more than enough) data to figure out what went wrong.
https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:21: from serial.tools import list_ports Can you surround this with a try & except & add some logging: try: from serial.tools import list_ports ... except ImportError: print serial.__file__ raise I suspect that there is some other serial module in the sys.path on linux.
Tryjob launched. https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:21: from serial.tools import list_ports On 2016/04/26 02:15:00, nednguyen wrote: > Can you surround this with a try & except & add some logging: > > try: > from serial.tools import list_ports > ... > except ImportError: > print serial.__file__ > raise > > I suspect that there is some other serial module in the sys.path on linux. Done. https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/60001/common/battor/battor/ba... common/battor/battor/battor_wrapper_devicetest.py:66: self.assertTrue(clock_sync_found) On 2016/04/26 00:12:13, nednguyen wrote: > self.assertTrue(clock_sync_found, 'Battor data:\n %s' % repr(results)) so we > know what went wrong. Done.
https://codereview.chromium.org/1920023002/diff/80001/catapult_build/build_st... File catapult_build/build_steps.py (right): https://codereview.chromium.org/1920023002/diff/80001/catapult_build/build_st... catapult_build/build_steps.py:161: 'name': 'BattOr Smoke Tests', For the quicker turnaround & alphabetical. I would put this at the top ;-)
Patchset #6 (id:100001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #6 (id:120001) has been deleted
The linux failure was it importing a different one, so moving the library to the front of the path seems to work. The windows failure was a race condition. If you close the battor connection too soon after issuing the clock sync, it wont flush. I added another 1 second sleep to the test and it seems to work. Im going to run the try job a few times to make sure its not flakey. https://codereview.chromium.org/1920023002/diff/80001/catapult_build/build_st... File catapult_build/build_steps.py (right): https://codereview.chromium.org/1920023002/diff/80001/catapult_build/build_st... catapult_build/build_steps.py:161: 'name': 'BattOr Smoke Tests', On 2016/04/26 02:28:04, nednguyen wrote: > For the quicker turnaround & alphabetical. I would put this at the top ;-) Done.
https://codereview.chromium.org/1920023002/diff/80001/common/battor/battor/ba... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1920023002/diff/80001/common/battor/battor/ba... common/battor/battor/battor_wrapper.py:17: sys.path.append( sys.path.insert(1, ...) It's using some serial lib in /usr/lib/python2.7/dist-packages/serial/__init__.pyc. Guido probably regrets how he designed the sys.path
lgtm But let's wait for Charlie's stamp https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:23: except: We can remove this now :-) https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:21: _SUPPORTED_CQ_PLATFORMS = ['win'] Let's add a TODO to expand this to mac & android https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:66: self.assertTrue(clock_sync_found, 'BattOr Data:%s\n' % repr(results)) nits: 2 blank line after this https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:70: nits: no trailing blank line
https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:143: for (port, desc, _) in serial.tools.list_ports.comports(): Later on, we should probably add a flag to the BattOr Agent on Windows to do this. something like --autodetect-battor-windows. We can't (and shouldn't) do this by default for reasons (something being named "USB serial port" isn't justification enough to start sending it random serial data, just in case it's like an ICBM launcher or something), but it'd be nice if we have an "I know what I'm doing" flag. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:21: _SUPPORTED_CQ_PLATFORMS = ['win'] My opinion: just get rid of this list altogether and do a direct check for 'win' in those places instead. Add a TODO to broaden it later. It's kind of a weird mix of platform agnosticism and windows specific stuff right now. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:27: if 'Win' in test_platform: Should we just choke for now if test_platform != 'Win'? https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:40: if self._platform != 'win' and len(self._battor_list) < 1: I think this can just be: if not self._battor_list: https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:40: if self._platform != 'win' and len(self._battor_list) < 1: Didn't you just check the Windows part of this this above? https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:42: return # No battors, cannot run test. s/battors/BattOr https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:50: if self._platform == 'win': You can already be sure that we're on Windows due to the SUPPORTED_CQ_PLATFORMS check above. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:51: # This is a work around for crbug.com/603971. How is this a workaround for that bug? It looks like that bug has to do w/ autodetection of the right COM port on Windows
https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:143: for (port, desc, _) in serial.tools.list_ports.comports(): On 2016/04/26 13:30:38, charliea wrote: > Later on, we should probably add a flag to the BattOr Agent on Windows to do > this. something like --autodetect-battor-windows. We can't (and shouldn't) do > this by default for reasons (something being named "USB serial port" isn't > justification enough to start sending it random serial data, just in case it's > like an ICBM launcher or something), but it'd be nice if we have an "I know what > I'm doing" flag. There should be way for us to grab the "Battor 3.2.1" name from the serial port, just like how Windows manager display it. We probably just haven't digged deep enough. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:46: time.sleep(1) Can you add comment explaining why we need this 1 second sleep? +Charlie: I think this is could be a battor bug.
https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:23: except: On 2016/04/26 04:02:53, nednguyen wrote: > We can remove this now :-) Done. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:143: for (port, desc, _) in serial.tools.list_ports.comports(): On 2016/04/26 13:30:38, charliea wrote: > Later on, we should probably add a flag to the BattOr Agent on Windows to do > this. something like --autodetect-battor-windows. We can't (and shouldn't) do > this by default for reasons (something being named "USB serial port" isn't > justification enough to start sending it random serial data, just in case it's > like an ICBM launcher or something), but it'd be nice if we have an "I know what > I'm doing" flag. Acknowledged. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:21: _SUPPORTED_CQ_PLATFORMS = ['win'] On 2016/04/26 04:02:53, nednguyen wrote: > Let's add a TODO to expand this to mac & android Android would be linux in this case, and with the next patch it will be enabled. Adding todo for mac. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:27: if 'Win' in test_platform: On 2016/04/26 13:30:38, charliea wrote: > Should we just choke for now if test_platform != 'Win'? Not here we shouldn't, this is just the setup portion. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:40: if self._platform != 'win' and len(self._battor_list) < 1: On 2016/04/26 13:30:38, charliea wrote: > Didn't you just check the Windows part of this this above? Thats for test setup that runs before every test, where it sets variables that will be needed for every test. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:40: if self._platform != 'win' and len(self._battor_list) < 1: On 2016/04/26 13:30:38, charliea wrote: > I think this can just be: if not self._battor_list: It needs the windows part, because on windows we expect the battor_list to be None since the usb discovery tools do not work there. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:42: return # No battors, cannot run test. On 2016/04/26 13:30:38, charliea wrote: > s/battors/BattOr Done. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:50: if self._platform == 'win': On 2016/04/26 13:30:38, charliea wrote: > You can already be sure that we're on Windows due to the SUPPORTED_CQ_PLATFORMS > check above. Not anymore, since I added linux support. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:51: # This is a work around for crbug.com/603971. It was the wrong bug, fixed. It was suppose to be the one for it crashing on windows. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:66: self.assertTrue(clock_sync_found, 'BattOr Data:%s\n' % repr(results)) On 2016/04/26 04:02:53, nednguyen wrote: > nits: 2 blank line after this Done. https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:70: On 2016/04/26 04:02:53, nednguyen wrote: > nits: no trailing blank line Done.
https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:46: time.sleep(1) On 2016/04/26 15:10:57, nednguyen wrote: > Can you add comment explaining why we need this 1 second sleep? > +Charlie: I think this is could be a battor bug. I was finding the clock sync marker would be flaky if there wasn't a sleep there. I couldn't determine if the flakiness was caused by it being called too shortly after starting tracing, or if it was being called too close to the end of tracing.
Patchset #7 (id:180001) has been deleted
https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:46: time.sleep(1) On 2016/04/26 15:18:31, rnephew1 wrote: > On 2016/04/26 15:10:57, nednguyen wrote: > > Can you add comment explaining why we need this 1 second sleep? > > +Charlie: I think this is could be a battor bug. > > I was finding the clock sync marker would be flaky if there wasn't a sleep > there. I couldn't determine if the flakiness was caused by it being called too > shortly after starting tracing, or if it was being called too close to the end > of tracing. Yeah, what I meant was to add code comment.
On 2016/04/27 17:51:58, nednguyen wrote: > https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... > File common/battor/battor/battor_wrapper_devicetest.py (right): > > https://codereview.chromium.org/1920023002/diff/160001/common/battor/battor/b... > common/battor/battor/battor_wrapper_devicetest.py:46: time.sleep(1) > On 2016/04/26 15:18:31, rnephew1 wrote: > > On 2016/04/26 15:10:57, nednguyen wrote: > > > Can you add comment explaining why we need this 1 second sleep? > > > +Charlie: I think this is could be a battor bug. > > > > I was finding the clock sync marker would be flaky if there wasn't a sleep > > there. I couldn't determine if the flakiness was caused by it being called too > > shortly after starting tracing, or if it was being called too close to the end > > of tracing. > > Yeah, what I meant was to add code comment. Done.
Gentle ping @Charlie even though he is marked as slow.
Patchset #7 (id:200001) has been deleted
Patchset #7 (id:220001) has been deleted
https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:17: sys.path.insert( I think that this should be added here: https://cs.corp.google.com/github/catapult-project/catapult/telemetry/telemet... That way, you can just say 'import serial' https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:137: if 'USB Serial Port' in desc: Can you add a comment describing why this is necessary? Something like: Right now, the BattOr agent binary isn't able to automatically detect the BattOr port on Windows. To get around this, we know that the BattOr shows up with a name of 'USB Serial Port', so use the COM port that corresponds to a device with that name. https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:12: if __name__ == '__main__': What's this for? https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:22: _SUPPORTED_CQ_PLATFORMS = ['win', 'linux'] Have we tested this on both Mac and Linux? https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:35: elif 'Darwin' in test_platform: # Mac. I think the "# Mac." is probably unnecessary - it's just as easy to just read one line further down. https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:46: return # No BattOrs, cannot run test. This comment is probably unnecessary - it's just a restatement of the critical log above https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:49: battor = battor_wrapper.BattorWrapper(self._platform, This seems really specific to Linux. Does this work on Windows? https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:53: # Sleep here because clock sync marker will be flaky if not. Have you experienced flakiness here? I don't think that there's any technical reason why it should be. If this is related to https://bugs.chromium.org/p/chromium/issues/detail?id=602266&q=owner%3Acharli..., then we should say something like: TODO(rnephew): This sleep is required for now because crbug.com/602266 causes the BattOr to crash when the trace time is too short. Once that bug is fixed, we should remove this delay. https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:57: time.sleep(1) This shouldn't be required either. It's definitely the case where we need to delay a little bit after the RecordClockSyncMarker call and before calling StopTracing to make sure that we give time for the clock synced sample to be flushed. However, I wrote some code to handle this within the BattOr agent itself: https://code.google.com/p/chromium/codesearch#chromium/src/tools/battor_agent... What kind of behavior are you seeing without this delay in? https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:60: # This is a work around for crbug.com/603309. Could you add some to this comment explaining how sleeping 5 seconds and killing the battor_shell is a workaround for that bug?
Patchset #8 (id:260001) has been deleted
Patchset #8 (id:280001) has been deleted
https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:17: sys.path.insert( On 2016/04/29 15:11:07, charliea (slow) wrote: > I think that this should be added here: > > https://cs.corp.google.com/github/catapult-project/catapult/telemetry/telemet... > > That way, you can just say 'import serial' Will that work, this being part of common? Or do you mean add it to __init__.py in common/battor/battor? Adding there as I assume thats what was intended. https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:137: if 'USB Serial Port' in desc: On 2016/04/29 15:11:07, charliea (slow) wrote: > Can you add a comment describing why this is necessary? Something like: > > Right now, the BattOr agent binary isn't able to automatically detect the BattOr > port on Windows. To get around this, we know that the BattOr shows up with a > name of 'USB Serial Port', so use the COM port that corresponds to a device with > that name. Done. https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:12: if __name__ == '__main__': On 2016/04/29 15:11:07, charliea (slow) wrote: > What's this for? When calling this test directly (like the CQ does) you need to have common/battor put into the python path in order to import battor_wrapper. https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:22: _SUPPORTED_CQ_PLATFORMS = ['win', 'linux'] On 2016/04/29 15:11:07, charliea (slow) wrote: > Have we tested this on both Mac and Linux? Not on mac, I have no mac battor cables. https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:35: elif 'Darwin' in test_platform: # Mac. On 2016/04/29 15:11:07, charliea (slow) wrote: > I think the "# Mac." is probably unnecessary - it's just as easy to just read > one line further down. Done. https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:46: return # No BattOrs, cannot run test. On 2016/04/29 15:11:07, charliea (slow) wrote: > This comment is probably unnecessary - it's just a restatement of the critical > log above Done. https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:49: battor = battor_wrapper.BattorWrapper(self._platform, On 2016/04/29 15:11:07, charliea (slow) wrote: > This seems really specific to Linux. Does this work on Windows? It does, but only because we are passing it win as the platform. Fixed it to be more clear whats going on. https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:53: # Sleep here because clock sync marker will be flaky if not. On 2016/04/29 15:11:07, charliea (slow) wrote: > Have you experienced flakiness here? I don't think that there's any technical > reason why it should be. If this is related to > https://bugs.chromium.org/p/chromium/issues/detail?id=602266&q=owner%3Acharli..., > then we should say something like: > > TODO(rnephew): This sleep is required for now because crbug.com/602266 causes > the BattOr to crash when the trace time is too short. Once that bug is fixed, we > should remove this delay. Added, I have also seen the clock sync not show up if you issue it to close to the starting of tracing. https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:57: time.sleep(1) On 2016/04/29 15:11:07, charliea (slow) wrote: > This shouldn't be required either. > > It's definitely the case where we need to delay a little bit after the > RecordClockSyncMarker call and before calling StopTracing to make sure that we > give time for the clock synced sample to be flushed. However, I wrote some code > to handle this within the BattOr agent itself: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/battor_agent... > > What kind of behavior are you seeing without this delay in? Clock sync will not always be present. https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:60: # This is a work around for crbug.com/603309. On 2016/04/29 15:11:07, charliea (slow) wrote: > Could you add some to this comment explaining how sleeping 5 seconds and killing > the battor_shell is a workaround for that bug? Done.
lgtm w/ nits and assuming that you've tested it on both win and linux https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:17: sys.path.insert( On 2016/04/29 17:05:57, rnephew1 wrote: > On 2016/04/29 15:11:07, charliea (slow) wrote: > > I think that this should be added here: > > > > > https://cs.corp.google.com/github/catapult-project/catapult/telemetry/telemet... > > > > That way, you can just say 'import serial' > > Will that work, this being part of common? Or do you mean add it to __init__.py > in common/battor/battor? > > Adding there as I assume thats what was intended. Uhhh... sure, let's pretend like I was smart enough to suggest that :) https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:22: _SUPPORTED_CQ_PLATFORMS = ['win', 'linux'] On 2016/04/29 17:05:57, rnephew1 wrote: > On 2016/04/29 15:11:07, charliea (slow) wrote: > > Have we tested this on both Mac and Linux? > > Not on mac, I have no mac battor cables. Errr, sorry, I meant both Win and Linux. https://codereview.chromium.org/1920023002/diff/300001/common/battor/battor/b... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/300001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:64: # On such a short trace, 5 seconds is enough to ensure 100% of the time Move this onto previous line https://codereview.chromium.org/1920023002/diff/300001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:65: # that the trace will finish flushing to the file. THe process is then s/THe/The
https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/240001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:22: _SUPPORTED_CQ_PLATFORMS = ['win', 'linux'] On 2016/04/29 17:18:17, charliea wrote: > On 2016/04/29 17:05:57, rnephew1 wrote: > > On 2016/04/29 15:11:07, charliea (slow) wrote: > > > Have we tested this on both Mac and Linux? > > > > Not on mac, I have no mac battor cables. > > Errr, sorry, I meant both Win and Linux. Yep. It has been tried locally on both windows and linux, and on the CQ via tryjob for windows. There are no linux battors on the CQ to teste that with. https://codereview.chromium.org/1920023002/diff/300001/common/battor/battor/b... File common/battor/battor/battor_wrapper_devicetest.py (right): https://codereview.chromium.org/1920023002/diff/300001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:64: # On such a short trace, 5 seconds is enough to ensure 100% of the time On 2016/04/29 17:18:17, charliea wrote: > Move this onto previous line Done. https://codereview.chromium.org/1920023002/diff/300001/common/battor/battor/b... common/battor/battor/battor_wrapper_devicetest.py:65: # that the trace will finish flushing to the file. THe process is then On 2016/04/29 17:18:17, charliea wrote: > s/THe/The Done.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/1920023002/#ps320001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1920023002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920023002/320001
Message was sent while issue was closed.
Description was changed from ========== [BattOr] Add real device smoke test BUG=catapult:#2219 ========== to ========== [BattOr] Add real device smoke test BUG=catapult:#2219 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:320001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
