Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
711113002:
Add option to specify ADB binary in test runner. (Closed)
|
Created:
6 years, 1 month ago by mikecase (-- gone --) Modified:
6 years, 1 month ago Reviewers:
jbudorick CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd option to specify ADB binary in test runner.
BUG=430957
Committed: https://crrev.com/48e16bf554e30c49d51a6a8b68c044cdeac70286
Cr-Commit-Position: refs/heads/master@{#304913}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Rebased onto recent test runner changes. #Patch Set 4 : Fixed issue with Host Forwarder not finding ADB. #Patch Set 5 : Decided to move where ADB added to path to test_runner.py #
Total comments: 13
Patch Set 6 : Made adb_path passed in to AdbInterface in init(). #Patch Set 7 : Removed no longer used SetAdbPath in AdbInterface. #Patch Set 8 : Rebase #Patch Set 9 : Makes tests pass even when absolute path of adb is given. #Patch Set 10 : #
Total comments: 1
Patch Set 11 : Made regex slightly more specific #Patch Set 12 : #Patch Set 13 : Fixed mocking AdbGetPath. #
Total comments: 1
Patch Set 14 : Simplified mocking AdbGetPath. #Patch Set 15 : #
Total comments: 1
Patch Set 16 : Fixed nits. #
Messages
Total messages: 31 (6 generated)
mikecase@chromium.org changed reviewers: + jbudorick@chromium.org
On 2014/11/10 21:10:14, mikecase wrote: By the way, the trybot failed because I don't think it liked trying to patch third_party/. The change works locally though (ran ContentShellTest and the logs showed the correct adb binary being used when I used the new adb_path test runner option)
On 2014/11/10 21:12:09, mikecase wrote: > On 2014/11/10 21:10:14, mikecase wrote: > > By the way, the trybot failed because I don't think it liked trying to patch > third_party/. The change works locally though (ran ContentShellTest and the logs > showed the correct adb binary being used when I used the new adb_path test > runner option) From the trybot: ... Failed to apply patch for build/android/test_runner.py: ... I think you need to rebase.
On 2014/11/10 21:32:51, jbudorick wrote: > On 2014/11/10 21:12:09, mikecase wrote: > > On 2014/11/10 21:10:14, mikecase wrote: > > > > By the way, the trybot failed because I don't think it liked trying to patch > > third_party/. The change works locally though (ran ContentShellTest and the > logs > > showed the correct adb binary being used when I used the new adb_path test > > runner option) > > From the trybot: > > ... > Failed to apply patch for build/android/test_runner.py: > ... > > I think you need to rebase. Rebased and fixed an issue where Forwarder couldn't find ADB due to a change I made. Take a look when you have a chance. Thanks!
Most of these changes look fine. https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/cons... File build/android/pylib/constants.py (right): https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/cons... build/android/pylib/constants.py:245: def GetAdbPath(): Why is this separate from _FindAdbPath? https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... build/android/test_runner.py:103: # Some things such as Forwarder require ADB to be in the environment path. Where is this? https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... build/android/test_runner.py:105: if adb_dir and adb_dir not in os.environ['PATH'].split(os.pathsep): Also, what's up with this...?
Read my comments when you have time. Thanks https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/cons... File build/android/pylib/constants.py (right): https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/cons... build/android/pylib/constants.py:245: def GetAdbPath(): On 2014/11/11 15:27:23, jbudorick wrote: > Why is this separate from _FindAdbPath? So I can keep the @_Memoize decorator on the logic inside _FindAdbPath (saves the result the first time the function is called, and just returns that result from then on). If I had @_Memoize on the whole function, SetAdbPath wouldn't do anything at all after GetAdbPath was called once which I didn't like. And if I got rid of @_Memoize it would work but be a little slower. https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... build/android/test_runner.py:103: # Some things such as Forwarder require ADB to be in the environment path. On 2014/11/11 15:27:23, jbudorick wrote: > Where is this? Forwarder is used inside of base_test_runner.py (calls Forwarder.Map() for example) and maybe some other places. I debated where to add adb to the path, but if I do it inside test_runner.py its just done once, at the beginning of the test run, and next to where the adb path is set which I like. Previously, adb was added to the path inside androidcommands.py for thirdparty/android_testrunner stuff (adding adb to the path here also happened to make the Forwarder happy). Now thirdparty/android_testrunner doesnt need adb in the path. https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... build/android/test_runner.py:105: if adb_dir and adb_dir not in os.environ['PATH'].split(os.pathsep): On 2014/11/11 15:27:23, jbudorick wrote: > Also, what's up with this...? Not sure what you mean. Most of this is logic copied from where adb was previously added to the path inside androidcommands.py. I'll walk you through though. 1. constants.GetAdbPath() is either 'adb' or some full path to an adb binary (it had always worked like this) so adb_dir=dirname(constants.GetAdbPath()) is either '' or the directory adb is in. 2. If adb_dir is a directory and not '', we check if its in os.environ['PATH'] already 3. If its not in the PATH, we add it to the beginning of the path (in case another adb is already in the path, we want to use the correct one)
https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/andr... build/android/pylib/android_commands.py:318: self._adb.SetAdbPath(constants.GetAdbPath()) Add the adb path as a parameter to AdbInterface.__init__ instead. https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/cons... File build/android/pylib/constants.py (right): https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/cons... build/android/pylib/constants.py:245: def GetAdbPath(): On 2014/11/11 17:56:41, mikecase wrote: > On 2014/11/11 15:27:23, jbudorick wrote: > > Why is this separate from _FindAdbPath? > > So I can keep the @_Memoize decorator on the logic inside _FindAdbPath (saves > the result the first time the function is called, and just returns that result > from then on). If I had @_Memoize on the whole function, SetAdbPath wouldn't do > anything at all after GetAdbPath was called once which I didn't like. And if I > got rid of @_Memoize it would work but be a little slower. Urgh, we need to rework these "constants." SetAdbPath _shouldn't_ be called once we call GetAdbPath, but until we do rework these constants, I guess this is ok. https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... build/android/test_runner.py:103: # Some things such as Forwarder require ADB to be in the environment path. On 2014/11/11 17:56:41, mikecase wrote: > On 2014/11/11 15:27:23, jbudorick wrote: > > Where is this? > > Forwarder is used inside of base_test_runner.py (calls Forwarder.Map() for > example) and maybe some other places. I debated where to add adb to the path, > but if I do it inside test_runner.py its just done once, at the beginning of the > test run, and next to where the adb path is set which I like. > > Previously, adb was added to the path inside androidcommands.py for > thirdparty/android_testrunner stuff (adding adb to the path here also happened > to make the Forwarder happy). Now thirdparty/android_testrunner doesnt need adb > in the path. I suppose I should have been specific. Where does forwarder require adb to be in the path? https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... build/android/test_runner.py:105: if adb_dir and adb_dir not in os.environ['PATH'].split(os.pathsep): On 2014/11/11 17:56:41, mikecase wrote: > On 2014/11/11 15:27:23, jbudorick wrote: > > Also, what's up with this...? > > Not sure what you mean. Most of this is logic copied from where adb was > previously added to the path inside androidcommands.py. > This was previously inside android_commands.py because adb_interface.py used 'adb' without the full path specified. Now that you're giving AdbInterface the full path to ADB, do we still need to do this? > I'll walk you through though. > 1. constants.GetAdbPath() is either 'adb' or some full path to an adb binary (it > had always worked like this) so adb_dir=dirname(constants.GetAdbPath()) is > either '' or the directory adb is in. > 2. If adb_dir is a directory and not '', we check if its in os.environ['PATH'] > already > 3. If its not in the PATH, we add it to the beginning of the path (in case > another adb is already in the path, we want to use the correct one) > > > > > >
https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/andr... build/android/pylib/android_commands.py:318: self._adb.SetAdbPath(constants.GetAdbPath()) On 2014/11/11 18:04:03, jbudorick wrote: > Add the adb path as a parameter to AdbInterface.__init__ instead. Done. https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... build/android/test_runner.py:103: # Some things such as Forwarder require ADB to be in the environment path. On 2014/11/11 18:04:03, jbudorick wrote: > On 2014/11/11 17:56:41, mikecase wrote: > > On 2014/11/11 15:27:23, jbudorick wrote: > > > Where is this? > > > > Forwarder is used inside of base_test_runner.py (calls Forwarder.Map() for > > example) and maybe some other places. I debated where to add adb to the path, > > but if I do it inside test_runner.py its just done once, at the beginning of > the > > test run, and next to where the adb path is set which I like. > > > > Previously, adb was added to the path inside androidcommands.py for > > thirdparty/android_testrunner stuff (adding adb to the path here also happened > > to make the Forwarder happy). Now thirdparty/android_testrunner doesnt need > adb > > in the path. > > I suppose I should have been specific. Where does forwarder require adb to be in > the path? Inside pylib/forwarder.py, inside Map() function, it calls... (exit_code, output) = cmd_helper.GetCmdStatusAndOutput( [instance._host_forwarder_path] + redirection_command) ...which calls the host_forwarder exec. This executable needs adb to be in the path. You can check out like... src/tools/android/forwarder2/host_forwarder_main.cc For example... const std::string command = base::StringPrintf( "adb %s forward --remove tcp:%d", serial_part.c_str(), port); const int ret = system(command.c_str()); ...seems to just do a system call to adb. Anyway, here is the trybot error I got when I was not adding adb to the path here. http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes... https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... build/android/test_runner.py:105: if adb_dir and adb_dir not in os.environ['PATH'].split(os.pathsep): On 2014/11/11 18:04:03, jbudorick wrote: > On 2014/11/11 17:56:41, mikecase wrote: > > On 2014/11/11 15:27:23, jbudorick wrote: > > > Also, what's up with this...? > > > > Not sure what you mean. Most of this is logic copied from where adb was > > previously added to the path inside androidcommands.py. > > > > This was previously inside android_commands.py because adb_interface.py used > 'adb' without the full path specified. Now that you're giving AdbInterface the > full path to ADB, do we still need to do this? > > > I'll walk you through though. > > 1. constants.GetAdbPath() is either 'adb' or some full path to an adb binary > (it > > had always worked like this) so adb_dir=dirname(constants.GetAdbPath()) is > > either '' or the directory adb is in. > > 2. If adb_dir is a directory and not '', we check if its in os.environ['PATH'] > > already > > 3. If its not in the PATH, we add it to the beginning of the path (in case > > another adb is already in the path, we want to use the correct one) > > > > > > > > > > > > > adb_interface.py no longer needs ADB to be in the path. However, Forwarder (and who knows, maybe other things too) needs ADB to be in the path.
On 2014/11/11 19:36:01, mikecase wrote: > https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/andr... > File build/android/pylib/android_commands.py (right): > > https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/andr... > build/android/pylib/android_commands.py:318: > self._adb.SetAdbPath(constants.GetAdbPath()) > On 2014/11/11 18:04:03, jbudorick wrote: > > Add the adb path as a parameter to AdbInterface.__init__ instead. > > Done. > > https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... > File build/android/test_runner.py (right): > > https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... > build/android/test_runner.py:103: # Some things such as Forwarder require ADB to > be in the environment path. > On 2014/11/11 18:04:03, jbudorick wrote: > > On 2014/11/11 17:56:41, mikecase wrote: > > > On 2014/11/11 15:27:23, jbudorick wrote: > > > > Where is this? > > > > > > Forwarder is used inside of base_test_runner.py (calls Forwarder.Map() for > > > example) and maybe some other places. I debated where to add adb to the > path, > > > but if I do it inside test_runner.py its just done once, at the beginning of > > the > > > test run, and next to where the adb path is set which I like. > > > > > > Previously, adb was added to the path inside androidcommands.py for > > > thirdparty/android_testrunner stuff (adding adb to the path here also > happened > > > to make the Forwarder happy). Now thirdparty/android_testrunner doesnt need > > adb > > > in the path. > > > > I suppose I should have been specific. Where does forwarder require adb to be > in > > the path? > > Inside pylib/forwarder.py, inside Map() function, it calls... > > (exit_code, output) = cmd_helper.GetCmdStatusAndOutput( > [instance._host_forwarder_path] + redirection_command) > > ...which calls the host_forwarder exec. This executable needs adb to be in the > path. You can check out like... > > src/tools/android/forwarder2/host_forwarder_main.cc > > For example... > > const std::string command = base::StringPrintf( > "adb %s forward --remove tcp:%d", > serial_part.c_str(), > port); > const int ret = system(command.c_str()); > > ...seems to just do a system call to adb. > > Anyway, here is the trybot error I got when I was not adding adb to the path > here. > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes... > > https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... > build/android/test_runner.py:105: if adb_dir and adb_dir not in > os.environ['PATH'].split(os.pathsep): > On 2014/11/11 18:04:03, jbudorick wrote: > > On 2014/11/11 17:56:41, mikecase wrote: > > > On 2014/11/11 15:27:23, jbudorick wrote: > > > > Also, what's up with this...? > > > > > > Not sure what you mean. Most of this is logic copied from where adb was > > > previously added to the path inside androidcommands.py. > > > > > > > This was previously inside android_commands.py because adb_interface.py used > > 'adb' without the full path specified. Now that you're giving AdbInterface the > > full path to ADB, do we still need to do this? > > > > > I'll walk you through though. > > > 1. constants.GetAdbPath() is either 'adb' or some full path to an adb binary > > (it > > > had always worked like this) so adb_dir=dirname(constants.GetAdbPath()) is > > > either '' or the directory adb is in. > > > 2. If adb_dir is a directory and not '', we check if its in > os.environ['PATH'] > > > already > > > 3. If its not in the PATH, we add it to the beginning of the path (in case > > > another adb is already in the path, we want to use the correct one) > > > > > > > > > > > > > > > > > > > > > > adb_interface.py no longer needs ADB to be in the path. However, Forwarder (and > who knows, maybe other things too) needs ADB to be in the path. What if we instead added an option to the forwarder?
On 2014/11/13 18:43:09, jbudorick wrote: > On 2014/11/11 19:36:01, mikecase wrote: > > > https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/andr... > > File build/android/pylib/android_commands.py (right): > > > > > https://codereview.chromium.org/711113002/diff/80001/build/android/pylib/andr... > > build/android/pylib/android_commands.py:318: > > self._adb.SetAdbPath(constants.GetAdbPath()) > > On 2014/11/11 18:04:03, jbudorick wrote: > > > Add the adb path as a parameter to AdbInterface.__init__ instead. > > > > Done. > > > > > https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... > > File build/android/test_runner.py (right): > > > > > https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... > > build/android/test_runner.py:103: # Some things such as Forwarder require ADB > to > > be in the environment path. > > On 2014/11/11 18:04:03, jbudorick wrote: > > > On 2014/11/11 17:56:41, mikecase wrote: > > > > On 2014/11/11 15:27:23, jbudorick wrote: > > > > > Where is this? > > > > > > > > Forwarder is used inside of base_test_runner.py (calls Forwarder.Map() for > > > > example) and maybe some other places. I debated where to add adb to the > > path, > > > > but if I do it inside test_runner.py its just done once, at the beginning > of > > > the > > > > test run, and next to where the adb path is set which I like. > > > > > > > > Previously, adb was added to the path inside androidcommands.py for > > > > thirdparty/android_testrunner stuff (adding adb to the path here also > > happened > > > > to make the Forwarder happy). Now thirdparty/android_testrunner doesnt > need > > > adb > > > > in the path. > > > > > > I suppose I should have been specific. Where does forwarder require adb to > be > > in > > > the path? > > > > Inside pylib/forwarder.py, inside Map() function, it calls... > > > > (exit_code, output) = cmd_helper.GetCmdStatusAndOutput( > > [instance._host_forwarder_path] + redirection_command) > > > > ...which calls the host_forwarder exec. This executable needs adb to be in the > > path. You can check out like... > > > > src/tools/android/forwarder2/host_forwarder_main.cc > > > > For example... > > > > const std::string command = base::StringPrintf( > > "adb %s forward --remove tcp:%d", > > serial_part.c_str(), > > port); > > const int ret = system(command.c_str()); > > > > ...seems to just do a system call to adb. > > > > Anyway, here is the trybot error I got when I was not adding adb to the path > > here. > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes... > > > > > https://codereview.chromium.org/711113002/diff/80001/build/android/test_runne... > > build/android/test_runner.py:105: if adb_dir and adb_dir not in > > os.environ['PATH'].split(os.pathsep): > > On 2014/11/11 18:04:03, jbudorick wrote: > > > On 2014/11/11 17:56:41, mikecase wrote: > > > > On 2014/11/11 15:27:23, jbudorick wrote: > > > > > Also, what's up with this...? > > > > > > > > Not sure what you mean. Most of this is logic copied from where adb was > > > > previously added to the path inside androidcommands.py. > > > > > > > > > > This was previously inside android_commands.py because adb_interface.py used > > > 'adb' without the full path specified. Now that you're giving AdbInterface > the > > > full path to ADB, do we still need to do this? > > > > > > > I'll walk you through though. > > > > 1. constants.GetAdbPath() is either 'adb' or some full path to an adb > binary > > > (it > > > > had always worked like this) so adb_dir=dirname(constants.GetAdbPath()) is > > > > either '' or the directory adb is in. > > > > 2. If adb_dir is a directory and not '', we check if its in > > os.environ['PATH'] > > > > already > > > > 3. If its not in the PATH, we add it to the beginning of the path (in case > > > > another adb is already in the path, we want to use the correct one) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > adb_interface.py no longer needs ADB to be in the path. However, Forwarder > (and > > who knows, maybe other things too) needs ADB to be in the path. > > What if we instead added an option to the forwarder? After looking at this, the minimal gain in clarity and consistency in the scripts isn't worth the effort required. Stick with the PATH-based approach in here for now.
lgtm
The CQ bit was checked by mikecase@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/711113002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/81359) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/86256) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32 on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by mikecase@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/711113002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2014/11/13 23:39:02, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/711113002/140001 Going to have to make changes to some of the device_utils_test.py to get this in... AssertionError: received command: /b/build/slave/linux/build/src/third_party/android_tools/sdk/platform-tools/adb -s 0123456789abcdef shell 'test -e "/fake/storage/path/temp_file-1415922449-0000106501"; echo $?' expected command: adb -s 0123456789abcdef shell 'test -e "/fake/storage/path/temp_file-\d+-\d+"; echo \$\?'
Hey, I want you to make a look at the changes I made to device_utils_test.py before I submit, since the changes I made were a little hacky. Some of the DeviceUtilsOldImplTest were failing because the full path of adb was being used. Thanks.
https://codereview.chromium.org/711113002/diff/180001/build/android/pylib/dev... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/711113002/diff/180001/build/android/pylib/dev... build/android/pylib/device/device_utils_test.py:183: self._comp(expected_cmd, re.sub(r'/.*/adb', 'adb', actual_cmd)), I'm not a fan of this (even if these tests are gradually being deprecated). re.sub seems like overkill, and there are corner cases where this could do the wrong thing. What about mocking out the call to GetAdbPath()?
On 2014/11/17 23:01:07, jbudorick wrote: > https://codereview.chromium.org/711113002/diff/180001/build/android/pylib/dev... > File build/android/pylib/device/device_utils_test.py (right): > > https://codereview.chromium.org/711113002/diff/180001/build/android/pylib/dev... > build/android/pylib/device/device_utils_test.py:183: self._comp(expected_cmd, > re.sub(r'/.*/adb', 'adb', actual_cmd)), > I'm not a fan of this (even if these tests are gradually being deprecated). > re.sub seems like overkill, and there are corner cases where this could do the > wrong thing. > > What about mocking out the call to GetAdbPath()? Done. Take another look at device_utils_test.py when you have a chance.
https://codereview.chromium.org/711113002/diff/240001/build/android/pylib/dev... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/711113002/diff/240001/build/android/pylib/dev... build/android/pylib/device/device_utils_test.py:218: with mock.patch('pylib.constants.GetAdbPath', why not patch it in setUp and unpatch it in tearDown? (which you'd have to add) You wouldn't have to worry about patching it in the tests below.
On 2014/11/19 19:58:46, jbudorick wrote: > https://codereview.chromium.org/711113002/diff/240001/build/android/pylib/dev... > File build/android/pylib/device/device_utils_test.py (right): > > https://codereview.chromium.org/711113002/diff/240001/build/android/pylib/dev... > build/android/pylib/device/device_utils_test.py:218: with > mock.patch('pylib.constants.GetAdbPath', > why not patch it in setUp and unpatch it in tearDown? (which you'd have to add) > > You wouldn't have to worry about patching it in the tests below. Done. Didn't know you could do that! Thanks for the tip.
lgtm w/ nit https://codereview.chromium.org/711113002/diff/280001/build/android/pylib/dev... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/711113002/diff/280001/build/android/pylib/dev... build/android/pylib/device/device_utils_test.py:218: self._getadbpath_patch = mock.patch('pylib.constants.GetAdbPath', nit: self._get_adb_path_patch
The CQ bit was checked by mikecase@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/711113002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/48e16bf554e30c49d51a6a8b68c044cdeac70286 Cr-Commit-Position: refs/heads/master@{#304913} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
