Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10957052:
Adapt python scripts to use the new Forwarder2 (Closed)
|
Created:
8 years, 3 months ago by felipeg Modified:
8 years, 2 months ago CC:
chromium-reviews, peter+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdapt python scripts to use the new Forwarder2
This is a DRAFT change.
I will polish it after running all the tests.
It worked for the integration tests.
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158940
Patch Set 1 #
Total comments: 12
Patch Set 2 : #Patch Set 3 : #
Total comments: 9
Patch Set 4 : #
Total comments: 8
Patch Set 5 : #
Messages
Total messages: 15 (0 generated)
This is just a draft change I will polish it after
thanks felipe! a few comments below: http://codereview.chromium.org/10957052/diff/1/build/android/pylib/base_test_... File build/android/pylib/base_test_runner.py (right): http://codereview.chromium.org/10957052/diff/1/build/android/pylib/base_test_... build/android/pylib/base_test_runner.py:73: Test results returned from RunTests(). TODO(tonyg): It looks like a bit unfair to add somone else's todo :) feel free to put my name on it. http://codereview.chromium.org/10957052/diff/1/build/android/pylib/forwarder.py File build/android/pylib/forwarder.py (right): http://codereview.chromium.org/10957052/diff/1/build/android/pylib/forwarder.... build/android/pylib/forwarder.py:111: # TODO(felipeg): Should I use tool.GetUtilWrapper() here ? bulach must know it. this is on the host, right? the tool.GetUtilWrapper() is required by the clang/asan builder, but only on the target, not the host.. unfortunately we don't have such bot upstream yet, so we'll need to keep an eye after you land/cherry-pick this downstream.. http://codereview.chromium.org/10957052/diff/1/build/android/pylib/run_java_t... File build/android/pylib/run_java_tests.py (right): http://codereview.chromium.org/10957052/diff/1/build/android/pylib/run_java_t... build/android/pylib/run_java_tests.py:43: # TODO(jaydeepmehta): FilterTests should be moved to a common file for ditto :) in fact, I think we can remove this, there has been quite a few refactorings ever since.. http://codereview.chromium.org/10957052/diff/1/build/android/pylib/run_java_t... build/android/pylib/run_java_tests.py:247: screenshot_tool = os.path.join(constants.CHROME_DIR, don't quite understand this diff, you probably need to rebaseline your repository? afaict, this patch has already been applied upstream.. http://codereview.chromium.org/10957052/diff/1/build/android/pylib/run_java_t... build/android/pylib/run_java_tests.py:281: print "====================================== " + str(self.ports_to_forward) spurious? http://codereview.chromium.org/10957052/diff/1/build/android/pylib/run_java_t... build/android/pylib/run_java_tests.py:368: # TODO(tonyg): Is there an error log line to watch for here? :)
Hey Guys, This is working now. It is ready to be submitted. I tested it with both the instrumentation tests and the net_unittests cherrypicked downstream. I also tested sharding it with 3 devices. https://codereview.chromium.org/10957052/diff/1/build/android/pylib/base_test... File build/android/pylib/base_test_runner.py (right): https://codereview.chromium.org/10957052/diff/1/build/android/pylib/base_test... build/android/pylib/base_test_runner.py:73: Test results returned from RunTests(). TODO(tonyg): It looks like On 2012/09/24 10:40:49, bulach wrote: > a bit unfair to add somone else's todo :) feel free to put my name on it. Sorry this was a spurious diff that I copied from downstream. https://codereview.chromium.org/10957052/diff/1/build/android/pylib/forwarder.py File build/android/pylib/forwarder.py (right): https://codereview.chromium.org/10957052/diff/1/build/android/pylib/forwarder... build/android/pylib/forwarder.py:111: # TODO(felipeg): Should I use tool.GetUtilWrapper() here ? bulach must know it. On 2012/09/24 10:40:49, bulach wrote: > this is on the host, right? > the tool.GetUtilWrapper() is required by the clang/asan builder, but only on the > target, not the host.. > unfortunately we don't have such bot upstream yet, so we'll need to keep an eye > after you land/cherry-pick this downstream.. Yes, here is on the host only. https://codereview.chromium.org/10957052/diff/1/build/android/pylib/run_java_... File build/android/pylib/run_java_tests.py (right): https://codereview.chromium.org/10957052/diff/1/build/android/pylib/run_java_... build/android/pylib/run_java_tests.py:43: # TODO(jaydeepmehta): FilterTests should be moved to a common file for On 2012/09/24 10:40:49, bulach wrote: > ditto :) > in fact, I think we can remove this, there has been quite a few refactorings > ever since.. Done. https://codereview.chromium.org/10957052/diff/1/build/android/pylib/run_java_... build/android/pylib/run_java_tests.py:247: screenshot_tool = os.path.join(constants.CHROME_DIR, On 2012/09/24 10:40:49, bulach wrote: > don't quite understand this diff, you probably need to rebaseline your > repository? afaict, this patch has already been applied upstream.. yep, sorry It was some confusion in my git client It must be ok now https://codereview.chromium.org/10957052/diff/1/build/android/pylib/run_java_... build/android/pylib/run_java_tests.py:281: print "====================================== " + str(self.ports_to_forward) On 2012/09/24 10:40:49, bulach wrote: > spurious? Done. https://codereview.chromium.org/10957052/diff/1/build/android/pylib/run_java_... build/android/pylib/run_java_tests.py:368: # TODO(tonyg): Is there an error log line to watch for here? On 2012/09/24 10:40:49, bulach wrote: > :) Done.
lgtm, thanks! the try bot didn't seem very happy, perhaps we need some changes in the forwarder itself to handle that error?
LGTM with minor nits. https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... tools/android/forwarder2/socket.cc:180: if (port_ == 0 && (family_ == AF_INET || family_ == AF_INET6)) { Nit: I think that this could be factored out into a helper method since you use it in multiple places. https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... tools/android/forwarder2/socket.cc:226: fcntl(socket_, F_SETFL, fcntl(socket_, F_GETFL) | O_NONBLOCK); Nit: maybe store the result of fcntl() into a const variable like |non_block_flags| then use it on line 239. Maybe add an extra DCHECK(!(non_block_flags & O_NONBLOCK)) too.
https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... tools/android/forwarder2/socket.cc:231: return false; you should restore the flags before returning. https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... tools/android/forwarder2/socket.cc:239: fcntl(socket_, F_SETFL, fcntl(socket_, F_GETFL) & ~O_NONBLOCK); I agree with Philippe that this kind of assumption can lead to really big surprises. I.e. it's probably simpler to save the old socket flags in a local variable, and restore them on exit. You will have to do that even in case of error (i.e. the "return false" cases above), and preserve the errno.
https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... tools/android/forwarder2/socket.cc:231: return false; On 2012/09/26 00:19:04, digit1 wrote: > you should restore the flags before returning. Good point. This might be a good opportunity to use RAII since we do multiple returns. Something like this at the beginning of the scope: struct RestoreFlags { RestoreFlags(old_flags) : old_flags_(old_flags) {} ~RestoreFlags() { fcntl(socket_, F_SETFL, old_flags_); } const int old_flags_; } restore_flags(fcntl(socket_, F_GETFL)); This might be overkill though :) base/auto_reset.h provides this kind of pattern, unfortunately it doesn't let us specify how we restore the value.
https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... tools/android/forwarder2/socket.cc:180: if (port_ == 0 && (family_ == AF_INET || family_ == AF_INET6)) { On 2012/09/26 00:15:46, Philippe wrote: > Nit: I think that this could be factored out into a helper method since you use > it in multiple places. Done. https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... tools/android/forwarder2/socket.cc:226: fcntl(socket_, F_SETFL, fcntl(socket_, F_GETFL) | O_NONBLOCK); On 2012/09/26 00:15:46, Philippe wrote: > Nit: maybe store the result of fcntl() into a const variable like > |non_block_flags| then use it on line 239. > Maybe add an extra DCHECK(!(non_block_flags & O_NONBLOCK)) too. Done. https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... tools/android/forwarder2/socket.cc:231: return false; On 2012/09/26 00:19:04, digit1 wrote: > you should restore the flags before returning. Done. https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forw... tools/android/forwarder2/socket.cc:239: fcntl(socket_, F_SETFL, fcntl(socket_, F_GETFL) & ~O_NONBLOCK); On 2012/09/26 00:19:04, digit1 wrote: > I agree with Philippe that this kind of assumption can lead to really big > surprises. I.e. it's probably simpler to save the old socket flags in a local > variable, and restore them on exit. You will have to do that even in case of > error (i.e. the "return false" cases above), and preserve the errno. Done.
On Tue, Sep 25, 2012 at 5:30 PM, <pliard@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/10957052/diff/** > 16001/tools/android/**forwarder2/socket.cc<https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forwarder2/socket.cc> > File tools/android/forwarder2/**socket.cc (right): > > https://chromiumcodereview.**appspot.com/10957052/diff/** > 16001/tools/android/**forwarder2/socket.cc#**newcode231<https://chromiumcodereview.appspot.com/10957052/diff/16001/tools/android/forwarder2/socket.cc#newcode231> > tools/android/forwarder2/**socket.cc:231: return false; > On 2012/09/26 00:19:04, digit1 wrote: > >> you should restore the flags before returning. >> > > Good point. > > This might be a good opportunity to use RAII since we do multiple > returns. > > Something like this at the beginning of the scope: > > struct RestoreFlags { > RestoreFlags(old_flags) : old_flags_(old_flags) {} > ~RestoreFlags() { fcntl(socket_, F_SETFL, old_flags_); } > const int old_flags_; > } restore_flags(fcntl(socket_, F_GETFL)); > > This might be overkill though :) > > base/auto_reset.h provides this kind of pattern, unfortunately it > doesn't let us specify how we restore the value. > I think this is overkill for now. Even if I forget to restore on any of the returns, this is OK, since those returns are in case of error, and the flags will not matter after that anyway > > https://chromiumcodereview.**appspot.com/10957052/<https://chromiumcodereview... >
just nits, go ahead once digit and the bots are happy! :) https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/forwa... File build/android/pylib/forwarder.py (right): https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/forwa... build/android/pylib/forwarder.py:63: logging.info('Forwarding ports: %s' % (forward_string)) logging takes a format and vararg as well, so this is preferrable: logging.info('Forwarding ports: %s', forward_string) https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/forwa... build/android/pylib/forwarder.py:100: error_msg)) nit: no need for the extra parens around error_msg https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/run_j... File build/android/pylib/run_java_tests.py (right): https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/run_j... build/android/pylib/run_java_tests.py:279: port_pairs = [] nit: can move this to 281 directly and remove the loop there: port_pairs = [(port, port) for port in self.ports_to_forward] https://codereview.chromium.org/10957052/diff/21001/tools/android/forwarder2/... File tools/android/forwarder2/host_controller.cc (right): https://codereview.chromium.org/10957052/diff/21001/tools/android/forwarder2/... tools/android/forwarder2/host_controller.cc:71: CHECK(SendCommand(command::LISTEN, device_port_, &adb_control_socket_)); nit: it's fairly uncommon to call non-const methods inside CHECKs, i.e., to do work there.. I think a more common idiom is: bool ret = SendCommand(...); CHECK(ret);
lgtm, if Marcus' remarks are fixed (especially the logging.info one).
https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/forwa... File build/android/pylib/forwarder.py (right): https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/forwa... build/android/pylib/forwarder.py:63: logging.info('Forwarding ports: %s' % (forward_string)) On 2012/09/26 15:13:11, bulach wrote: > logging takes a format and vararg as well, so this is preferrable: > logging.info('Forwarding ports: %s', forward_string) Done. https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/forwa... build/android/pylib/forwarder.py:100: error_msg)) On 2012/09/26 15:13:11, bulach wrote: > nit: no need for the extra parens around error_msg Done. https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/run_j... File build/android/pylib/run_java_tests.py (right): https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/run_j... build/android/pylib/run_java_tests.py:279: port_pairs = [] On 2012/09/26 15:13:11, bulach wrote: > nit: can move this to 281 directly and remove the loop there: > > port_pairs = [(port, port) for port in self.ports_to_forward] Done. https://codereview.chromium.org/10957052/diff/21001/tools/android/forwarder2/... File tools/android/forwarder2/host_controller.cc (right): https://codereview.chromium.org/10957052/diff/21001/tools/android/forwarder2/... tools/android/forwarder2/host_controller.cc:71: CHECK(SendCommand(command::LISTEN, device_port_, &adb_control_socket_)); On 2012/09/26 15:13:11, bulach wrote: > nit: it's fairly uncommon to call non-const methods inside CHECKs, i.e., to do > work there.. I think a more common idiom is: > bool ret = SendCommand(...); > CHECK(ret); Done.
Thanks Guys, I am cherry-picking this patch downstream and I will test it downstream first before committing it upstream. On Wed, Sep 26, 2012 at 11:06 AM, <felipeg@chromium.org> wrote: > > https://codereview.chromium.**org/10957052/diff/21001/build/** > android/pylib/forwarder.py<https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/forwarder.py> > File build/android/pylib/forwarder.**py (right): > > https://codereview.chromium.**org/10957052/diff/21001/build/** > android/pylib/forwarder.py#**newcode63<https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/forwarder.py#newcode63> > build/android/pylib/forwarder.**py:63: logging.info('Forwarding ports: %s' > % (forward_string)) > On 2012/09/26 15:13:11, bulach wrote: > >> logging takes a format and vararg as well, so this is preferrable: >> logging.info('Forwarding ports: %s', forward_string) >> > > Done. > > > https://codereview.chromium.**org/10957052/diff/21001/build/** > android/pylib/forwarder.py#**newcode100<https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/forwarder.py#newcode100> > build/android/pylib/forwarder.**py:100: error_msg)) > On 2012/09/26 15:13:11, bulach wrote: > >> nit: no need for the extra parens around error_msg >> > > Done. > > > https://codereview.chromium.**org/10957052/diff/21001/build/** > android/pylib/run_java_tests.**py<https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/run_java_tests.py> > File build/android/pylib/run_java_**tests.py (right): > > https://codereview.chromium.**org/10957052/diff/21001/build/** > android/pylib/run_java_tests.**py#newcode279<https://codereview.chromium.org/10957052/diff/21001/build/android/pylib/run_java_tests.py#newcode279> > build/android/pylib/run_java_**tests.py:279: port_pairs = [] > On 2012/09/26 15:13:11, bulach wrote: > >> nit: can move this to 281 directly and remove the loop there: >> > > port_pairs = [(port, port) for port in self.ports_to_forward] >> > > Done. > > > https://codereview.chromium.**org/10957052/diff/21001/tools/** > android/forwarder2/host_**controller.cc<https://codereview.chromium.org/10957052/diff/21001/tools/android/forwarder2/host_controller.cc> > File tools/android/forwarder2/host_**controller.cc (right): > > https://codereview.chromium.**org/10957052/diff/21001/tools/** > android/forwarder2/host_**controller.cc#newcode71<https://codereview.chromium.org/10957052/diff/21001/tools/android/forwarder2/host_controller.cc#newcode71> > tools/android/forwarder2/host_**controller.cc:71: > CHECK(SendCommand(command::**LISTEN, device_port_, &adb_control_socket_)); > On 2012/09/26 15:13:11, bulach wrote: > >> nit: it's fairly uncommon to call non-const methods inside CHECKs, >> > i.e., to do > >> work there.. I think a more common idiom is: >> bool ret = SendCommand(...); >> CHECK(ret); >> > > Done. > > https://codereview.chromium.**org/10957052/<https://codereview.chromium.org/1... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/10957052/8003
Change committed as 158940 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
