Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 11 months ago by jbudorick Modified:
5 years, 10 months ago Reviewers:
klundberg, Ted C, Paweł Hajdan Jr., cjhopman CC:
chromium-reviews, cbentzel+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Add a java version of the test server spawner.
BUG=448626
Committed: https://crrev.com/e1799e662af4e87b1e6d501e3511628f8579174d
Cr-Commit-Position: refs/heads/master@{#313532}
Patch Set 1 #
Total comments: 1
Patch Set 2 : added gn files #Patch Set 3 : #Patch Set 4 : #
Total comments: 19
Patch Set 5 : #
Total comments: 15
Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Patch Set 9 : findbugs #
Total comments: 1
Created: 5 years, 10 months ago
Messages
Total messages: 26 (5 generated)
jbudorick@chromium.org changed reviewers: + klundberg@chromium.org, phajdan.jr@chromium.org, tedchoc@chromium.org
klundberg: build/android/ tedchoc: chrome/android/ and testing/android/ phajdan: net/test/android/ ptal https://codereview.chromium.org/867073002/diff/1/net/test/android/javatests/s... File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java (right): https://codereview.chromium.org/867073002/diff/1/net/test/android/javatests/s... net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. I'm not sure if this belongs in //net or //base (along with BaseTcpTestServer and BaseHttpTestServer). Thoughts welcome.
jbudorick@chromium.org changed reviewers: + cjhopman@chromium.org
+cjhopman for gyp/gn
https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/BaseHttpTestServer.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/BaseHttpTestServer.java:64: conn.close(); Shouldn't this be in a "finally" clause? https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/BaseTcpTestServer.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/BaseTcpTestServer.java:23: mServerSocket.setSoTimeout(5000); Can we avoid a hardcoded timeout here? Please consider having a solution similar to TestTimeouts on the C++ side: centralized, and controlled with command-line flags. https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:52: private String mCertAndKeyFile; It'd be useful to comment each variable. https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:114: // Needs to support all options provided by testserver_base.py and testserver.py Why does it need to? I'm not sure if keeping the two in sync is viable. https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java:67: Log.i(TAG, "Received GET /kill request."); Can we avoid supporting killing the server this way? Why not just kill the process? https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java:82: entity = new StringEntity("ready"); Is the purpose of this to wait for the server to be ready? This'd suggest a polling loop on the client side, which is not optimal. Please consider a way (if possible) to use waiting for an event instead. It'd be much more reliable. This comment is based on *actual* problems with the python http server. Just trying to avoid repeating some known problems in this new code.
LGTM for the build/android pieces
https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/BaseHttpTestServer.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/BaseHttpTestServer.java:64: conn.close(); On 2015/01/23 10:44:49, Paweł Hajdan Jr. wrote: > Shouldn't this be in a "finally" clause? Done. https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/BaseTcpTestServer.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/BaseTcpTestServer.java:23: mServerSocket.setSoTimeout(5000); On 2015/01/23 10:44:49, Paweł Hajdan Jr. wrote: > Can we avoid a hardcoded timeout here? Please consider having a solution similar > to TestTimeouts on the C++ side: centralized, and controlled with command-line > flags. I'm open to not hardcoding the timeout here, but this just times out the call to ServerSocket.accept() so we don't wait there forever. It doesn't cause the tests to time out. https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:21: accept(); Continuing from BaseTcpTestServer: hitting the socket timeout in BaseTcpTestServer just lets us check if the server should be stopped. If not, we try to accept again. https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:52: private String mCertAndKeyFile; On 2015/01/23 10:44:50, Paweł Hajdan Jr. wrote: > It'd be useful to comment each variable. Done. https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:114: // Needs to support all options provided by testserver_base.py and testserver.py On 2015/01/23 10:44:49, Paweł Hajdan Jr. wrote: > Why does it need to? I'm not sure if keeping the two in sync is viable. I guess it doesn't have to (and indeed, there are some we can't support), so I removed the comment. https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java:67: Log.i(TAG, "Received GET /kill request."); On 2015/01/23 10:44:50, Paweł Hajdan Jr. wrote: > Can we avoid supporting killing the server this way? Why not just kill the > process? see below https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java:82: entity = new StringEntity("ready"); On 2015/01/23 10:44:50, Paweł Hajdan Jr. wrote: > Is the purpose of this to wait for the server to be ready? This'd suggest a > polling loop on the client side, which is not optimal. Please consider a way (if > possible) to use waiting for an event instead. It'd be much more reliable. > > This comment is based on *actual* problems with the python http server. Just > trying to avoid repeating some known problems in this new code. (leaving the code as-is given pending discussion) This is what the SpawnerCommunicator is expecting. I had been planning to leave the client side (i.e. SpawnerCommunicator and RemoteTestServer) alone while reimplementing the server side in java, as the existing server implementation has to keep working in the meantime. In that scenario, I'd certainly be open to changing the client side after the existing server implementation is no longer used in Android tests. wdyt?
https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/BaseHttpTestServer.java (right): https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/BaseHttpTestServer.java:67: protected HttpParams getConnectionParams() { all non-private methods should have javadoc https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/BaseTcpTestServer.java (right): https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/BaseTcpTestServer.java:15: public abstract class BaseTcpTestServer extends BaseTestServer { Are there any usages of this class or BaseTestServer directly? Seems like more indirection than needed if we don't see usages of these in the near future (and could be abstracted if an when we do need them). https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java (right): https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:21: class TestServerBuilder { javadoc for what the intended purpose of this class is. https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:182: mCertAndKeyFile = null; null, 0, and false are the defaults in java. And since they're non-final (maybe they should be final), I would inline the default at declaration time. https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:197: mSslBulkCiphers = new Vector<String>(SSL_BULK_CIPHERS); ArrayList is way more common than vector. https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:224: case "basic-auth-proxy": switching on strings is only applicable in KK+, so if this supposed to run all the way back to ICS then this won't work. https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java (right): https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java:98: private static final int ARBITRARY_MAX_JSON_SIZE = 65536; put this up with the other statics.
lgtm
lgtm lgtm
https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/BaseHttpTestServer.java (right): https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/BaseHttpTestServer.java:67: protected HttpParams getConnectionParams() { On 2015/01/26 22:10:10, Ted C wrote: > all non-private methods should have javadoc Done. https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/BaseTcpTestServer.java (right): https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/BaseTcpTestServer.java:15: public abstract class BaseTcpTestServer extends BaseTestServer { On 2015/01/26 22:10:10, Ted C wrote: > Are there any usages of this class or BaseTestServer directly? > > Seems like more indirection than needed if we don't see usages of these in the > near future (and could be abstracted if an when we do need them). I'm planning to implement at least the TCP_ECHO and FTP servers directly on top of this one, and I'll be implementing the UDP_ECHO server on top of BaseTestServer. I'll be working on those CLs immediately after this one lands. https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java (right): https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:21: class TestServerBuilder { On 2015/01/26 22:10:10, Ted C wrote: > javadoc for what the intended purpose of this class is. Done. https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:182: mCertAndKeyFile = null; On 2015/01/26 22:10:10, Ted C wrote: > null, 0, and false are the defaults in java. > (my old c++03 initializer list habits die hard) > And since they're non-final (maybe they should be final), I would inline the > default at declaration time. Done. https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:197: mSslBulkCiphers = new Vector<String>(SSL_BULK_CIPHERS); On 2015/01/26 22:10:10, Ted C wrote: > ArrayList is way more common than vector. Switched to ArrayList. https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:224: case "basic-auth-proxy": On 2015/01/26 22:10:11, Ted C wrote: > switching on strings is only applicable in KK+, so if this supposed to run all > the way back to ICS then this won't work. It's certainly supposed to run pre-KK. Switched. https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java (right): https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java:98: private static final int ARBITRARY_MAX_JSON_SIZE = 65536; On 2015/01/26 22:10:11, Ted C wrote: > put this up with the other statics. Done.
https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java (right): https://codereview.chromium.org/867073002/diff/80001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:224: case "basic-auth-proxy": On 2015/01/27 00:27:56, jbudorick wrote: > On 2015/01/26 22:10:11, Ted C wrote: > > switching on strings is only applicable in KK+, so if this supposed to run all > > the way back to ICS then this won't work. > > It's certainly supposed to run pre-KK. Switched. string switch actually works fine pre-kk. The main java 7 feature that is kk+ only is try-with-resource. See http://tools.android.com/tech-docs/new-build-system/user-guide#TOC-Using-sour...
https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/BaseTcpTestServer.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/BaseTcpTestServer.java:23: mServerSocket.setSoTimeout(5000); On 2015/01/23 19:25:14, jbudorick wrote: > On 2015/01/23 10:44:49, Paweł Hajdan Jr. wrote: > > Can we avoid a hardcoded timeout here? Please consider having a solution > similar > > to TestTimeouts on the C++ side: centralized, and controlled with command-line > > flags. > > I'm open to not hardcoding the timeout here, but this just times out the call to > ServerSocket.accept() so we don't wait there forever. It doesn't cause the tests > to time out. Yes, let's avoid the hardcoded timeout. https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:114: // Needs to support all options provided by testserver_base.py and testserver.py On 2015/01/23 19:25:15, jbudorick wrote: > On 2015/01/23 10:44:49, Paweł Hajdan Jr. wrote: > > Why does it need to? I'm not sure if keeping the two in sync is viable. > > I guess it doesn't have to (and indeed, there are some we can't support), so I > removed the comment. Thanks! Are all of the options needed? They can always be added later, but once they're checked in removal is harder. https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java:82: entity = new StringEntity("ready"); On 2015/01/23 19:25:15, jbudorick wrote: > On 2015/01/23 10:44:50, Paweł Hajdan Jr. wrote: > > Is the purpose of this to wait for the server to be ready? This'd suggest a > > polling loop on the client side, which is not optimal. Please consider a way > (if > > possible) to use waiting for an event instead. It'd be much more reliable. > > > > This comment is based on *actual* problems with the python http server. Just > > trying to avoid repeating some known problems in this new code. > > (leaving the code as-is given pending discussion) > > This is what the SpawnerCommunicator is expecting. I had been planning to leave > the client side (i.e. SpawnerCommunicator and RemoteTestServer) alone while > reimplementing the server side in java, as the existing server implementation > has to keep working in the meantime. In that scenario, I'd certainly be open to > changing the client side after the existing server implementation is no longer > used in Android tests. > > wdyt? Sounds good, how about comments and TODOs?
lgtm https://codereview.chromium.org/867073002/diff/120001/net/test/android/javate... File net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java (right): https://codereview.chromium.org/867073002/diff/120001/net/test/android/javate... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:223: if ("basic-auth-proxy".equals(key)) { sorry for the churn...didn't know string switch was valid...learned something new today.
https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/BaseTcpTestServer.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/BaseTcpTestServer.java:23: mServerSocket.setSoTimeout(5000); On 2015/01/27 15:10:50, Paweł Hajdan Jr. wrote: > On 2015/01/23 19:25:14, jbudorick wrote: > > On 2015/01/23 10:44:49, Paweł Hajdan Jr. wrote: > > > Can we avoid a hardcoded timeout here? Please consider having a solution > > similar > > > to TestTimeouts on the C++ side: centralized, and controlled with > command-line > > > flags. > > > > I'm open to not hardcoding the timeout here, but this just times out the call > to > > ServerSocket.accept() so we don't wait there forever. It doesn't cause the > tests > > to time out. > > Yes, let's avoid the hardcoded timeout. Done. https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:114: // Needs to support all options provided by testserver_base.py and testserver.py On 2015/01/27 15:10:50, Paweł Hajdan Jr. wrote: > On 2015/01/23 19:25:15, jbudorick wrote: > > On 2015/01/23 10:44:49, Paweł Hajdan Jr. wrote: > > > Why does it need to? I'm not sure if keeping the two in sync is viable. > > > > I guess it doesn't have to (and indeed, there are some we can't support), so I > > removed the comment. > > Thanks! Are all of the options needed? They can always be added later, but once > they're checked in removal is harder. I ran the four test suites for which we currently use the test server (unit_tests, net_unittests, content_unittests, and content_browsertests) and removed all options that were not used. I also removed all corresponding members that appear to be noops in their default state. https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... File net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java (right): https://codereview.chromium.org/867073002/diff/60001/net/test/android/javates... net/test/android/javatests/src/org/chromium/net/test/TestServerSpawner.java:82: entity = new StringEntity("ready"); On 2015/01/27 15:10:50, Paweł Hajdan Jr. wrote: > On 2015/01/23 19:25:15, jbudorick wrote: > > On 2015/01/23 10:44:50, Paweł Hajdan Jr. wrote: > > > Is the purpose of this to wait for the server to be ready? This'd suggest a > > > polling loop on the client side, which is not optimal. Please consider a way > > (if > > > possible) to use waiting for an event instead. It'd be much more reliable. > > > > > > This comment is based on *actual* problems with the python http server. Just > > > trying to avoid repeating some known problems in this new code. > > > > (leaving the code as-is given pending discussion) > > > > This is what the SpawnerCommunicator is expecting. I had been planning to > leave > > the client side (i.e. SpawnerCommunicator and RemoteTestServer) alone while > > reimplementing the server side in java, as the existing server implementation > > has to keep working in the meantime. In that scenario, I'd certainly be open > to > > changing the client side after the existing server implementation is no longer > > used in Android tests. > > > > wdyt? > > Sounds good, how about comments and TODOs? Done. https://codereview.chromium.org/867073002/diff/120001/net/test/android/javate... File net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java (right): https://codereview.chromium.org/867073002/diff/120001/net/test/android/javate... net/test/android/javatests/src/org/chromium/net/test/TestServerBuilder.java:223: if ("basic-auth-proxy".equals(key)) { On 2015/01/27 18:26:22, Ted C wrote: > sorry for the churn...didn't know string switch was valid...learned something > new today. np. switched back
LGTM, thanks!
The CQ bit was checked by jbudorick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/867073002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by jbudorick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/867073002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e1799e662af4e87b1e6d501e3511628f8579174d Cr-Commit-Position: refs/heads/master@{#313532}
Message was sent while issue was closed.
https://codereview.chromium.org/867073002/diff/160001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/867073002/diff/160001/build/config/android/ru... build/config/android/rules.gni:1493: "//net/android:net_java_test_support", Please don't. /build should not depend on /net. Test apks that need that dependency can add it themselves.
Message was sent while issue was closed.
On 2015/02/26 15:50:03, ppi wrote: > https://codereview.chromium.org/867073002/diff/160001/build/config/android/ru... > File build/config/android/rules.gni (right): > > https://codereview.chromium.org/867073002/diff/160001/build/config/android/ru... > build/config/android/rules.gni:1493: "//net/android:net_java_test_support", > Please don't. /build should not depend on /net. Test apks that need that > dependency can add it themselves. https://code.google.com/p/chromium/issues/detail?id=462239 |
