Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
43793002:
[DevTools] Added browser tests for adb_client_socket.cc and android_device.cc (Closed)
|
Created:
7 years, 1 month ago by Dmitry Zvorygin Modified:
7 years, 1 month ago CC:
chromium-reviews, vsevik, yurys, paulirish+reviews_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org, pfeldman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded browser tests for adb_client_socket.cc and android_device.cc.
BUG=308535
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232957
Patch Set 1 #
Total comments: 6
Patch Set 2 : Extracted adb server to separate class. #
Total comments: 2
Patch Set 3 : Fixed nits. Added offline device. #
Total comments: 8
Patch Set 4 : Fixed comments. #
Total comments: 2
Patch Set 5 : Fixed nits. #Patch Set 6 : Fixed compile error. #Patch Set 7 : Fixed typo. #Patch Set 8 : Removed destructor's OVERRIDE specifier. #
Messages
Total messages: 20 (0 generated)
Please take a look.
lgtm https://chromiumcodereview.appspot.com/43793002/diff/1/chrome/browser/devtool... File chrome/browser/devtools/adb_client_socket_browsertest.cc (right): https://chromiumcodereview.appspot.com/43793002/diff/1/chrome/browser/devtool... chrome/browser/devtools/adb_client_socket_browsertest.cc:80: ASSERT_EQ(android_device_->serial(), "1498B321301A00A"); lets test "offline" device as well this will probably require a separate test since if you have two devices connected you cannot send commands such as getprop https://chromiumcodereview.appspot.com/43793002/diff/1/chrome/browser/devtool... chrome/browser/devtools/adb_client_socket_browsertest.cc:82: extra blank line https://chromiumcodereview.appspot.com/43793002/diff/1/chrome/browser/devtool... chrome/browser/devtools/adb_client_socket_browsertest.cc:120: // Adb server implementation Please extract a separate class for the server. You are going to need it, trust me.
lgtm lgtm
On 2013/10/25 15:18:19, Vladislav Kaznacheev wrote: > lgtm > > lgtm sorry, missed the button not lgtm yet
Please take a look. https://chromiumcodereview.appspot.com/43793002/diff/1/chrome/browser/devtool... File chrome/browser/devtools/adb_client_socket_browsertest.cc (right): https://chromiumcodereview.appspot.com/43793002/diff/1/chrome/browser/devtool... chrome/browser/devtools/adb_client_socket_browsertest.cc:80: ASSERT_EQ(android_device_->serial(), "1498B321301A00A"); On 2013/10/25 15:18:19, Vladislav Kaznacheev wrote: > lets test "offline" device as well > this will probably require a separate test since if you have two devices > connected you cannot send commands such as getprop Done. https://chromiumcodereview.appspot.com/43793002/diff/1/chrome/browser/devtool... chrome/browser/devtools/adb_client_socket_browsertest.cc:82: On 2013/10/25 15:18:19, Vladislav Kaznacheev wrote: > extra blank line Done. https://chromiumcodereview.appspot.com/43793002/diff/1/chrome/browser/devtool... chrome/browser/devtools/adb_client_socket_browsertest.cc:120: // Adb server implementation On 2013/10/25 15:18:19, Vladislav Kaznacheev wrote: > Please extract a separate class for the server. You are going to need it, trust > me. Done.
https://chromiumcodereview.appspot.com/43793002/diff/90001/chrome/browser/dev... File chrome/browser/devtools/adb_client_socket_browsertest.cc (right): https://chromiumcodereview.appspot.com/43793002/diff/90001/chrome/browser/dev... chrome/browser/devtools/adb_client_socket_browsertest.cc:106: // Posting is needed not to enter deep recursion in case too synchronous in case OF too ... https://chromiumcodereview.appspot.com/43793002/diff/90001/chrome/browser/dev... chrome/browser/devtools/adb_client_socket_browsertest.cc:142: bool TryProcessCommand() { Lets split it one more time: a generic server that knows nothing about ADB and a delegate that implements TryProcessCommand. https://chromiumcodereview.appspot.com/43793002/diff/120001/chrome/browser/de... File chrome/browser/devtools/adb_client_socket_browsertest.cc (right): https://chromiumcodereview.appspot.com/43793002/diff/120001/chrome/browser/de... chrome/browser/devtools/adb_client_socket_browsertest.cc:220: content::BrowserThread::PostTask( This look terrifying. Please consider using BrowserThread::PostTaskAndReply here and in EndTest https://chromiumcodereview.appspot.com/43793002/diff/120001/chrome/browser/de... chrome/browser/devtools/adb_client_socket_browsertest.cc:267: #if defined(DEBUG_DEVTOOLS) Just to make sure, did you try running this with debug_devtools=1? I am asking because I know that this variable is not propagated to every module of Chrome. https://chromiumcodereview.appspot.com/43793002/diff/120001/chrome/browser/de... chrome/browser/devtools/adb_client_socket_browsertest.cc:307: scoped_refptr<RefCountedAdbThread> adb_thread_; I suspect that only adb_thread_ really needs to be a member (to make sure that the thread exists for long enough). The other 3 scoped_refptr's can be scope variables.
https://chromiumcodereview.appspot.com/43793002/diff/120001/chrome/browser/de... File chrome/browser/devtools/adb_client_socket_browsertest.cc (right): https://chromiumcodereview.appspot.com/43793002/diff/120001/chrome/browser/de... chrome/browser/devtools/adb_client_socket_browsertest.cc:255: void QueryDevices() { Is there a reason not to use DevToolsAdbBridge (like you are doing in the previous test patch?)
Please take a look. https://codereview.chromium.org/43793002/diff/120001/chrome/browser/devtools/... File chrome/browser/devtools/adb_client_socket_browsertest.cc (right): https://codereview.chromium.org/43793002/diff/120001/chrome/browser/devtools/... chrome/browser/devtools/adb_client_socket_browsertest.cc:220: content::BrowserThread::PostTask( On 2013/10/28 14:12:37, Vladislav Kaznacheev wrote: > This look terrifying. > > Please consider using BrowserThread::PostTaskAndReply here and in EndTest Done. https://codereview.chromium.org/43793002/diff/120001/chrome/browser/devtools/... chrome/browser/devtools/adb_client_socket_browsertest.cc:255: void QueryDevices() { On 2013/10/28 18:34:34, Vladislav Kaznacheev wrote: > Is there a reason not to use DevToolsAdbBridge (like you are doing in the > previous test patch?) Done. https://codereview.chromium.org/43793002/diff/120001/chrome/browser/devtools/... chrome/browser/devtools/adb_client_socket_browsertest.cc:267: #if defined(DEBUG_DEVTOOLS) On 2013/10/28 14:12:37, Vladislav Kaznacheev wrote: > Just to make sure, did you try running this with debug_devtools=1? > I am asking because I know that this variable is not propagated to every module > of Chrome. Sure, see gyp(i) file in changelist. https://codereview.chromium.org/43793002/diff/120001/chrome/browser/devtools/... chrome/browser/devtools/adb_client_socket_browsertest.cc:307: scoped_refptr<RefCountedAdbThread> adb_thread_; On 2013/10/28 14:12:37, Vladislav Kaznacheev wrote: > I suspect that only adb_thread_ really needs to be a member (to make sure that > the thread exists for long enough). The other 3 scoped_refptr's can be scope > variables. Done.
LGTM with nits https://chromiumcodereview.appspot.com/43793002/diff/160001/chrome/browser/de... File chrome/browser/devtools/adb_client_socket_browsertest.cc (right): https://chromiumcodereview.appspot.com/43793002/diff/160001/chrome/browser/de... chrome/browser/devtools/adb_client_socket_browsertest.cc:245: virtual ~MockAdbServer() OVERRIDE no line break here https://chromiumcodereview.appspot.com/43793002/diff/160001/chrome/browser/de... chrome/browser/devtools/adb_client_socket_browsertest.cc:325: void EndTest() { Does not need to be public.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zvorygin@chromium.org/43793002/240001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zvorygin@chromium.org/43793002/520001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zvorygin@chromium.org/43793002/730001
Retried try job too often on linux_chromeos for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zvorygin@chromium.org/43793002/940001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zvorygin@chromium.org/43793002/940001
Message was sent while issue was closed.
Change committed as 232957 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
