Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
336373004:
Install support APK together with test APK (Closed)
|
Created:
6 years, 6 months ago by aberent Modified:
6 years, 6 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionInstall support APK together with test APK
The Chrome for Android instrumentation tests (the tests in ChromeTest.apk)
need to start some services to test Chromecast support. This was
originally done within the ChromeTest.apk, but, because of the changes
required for https://code.google.com/p/chromium/issues/detail?id=272790
this is no longer possible. This patch allows the use of a seperate test
support apk.
BUG=384537
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279096
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move where the name of the support APK is calculated #
Total comments: 2
Patch Set 3 : Fix nits #
Messages
Total messages: 25 (0 generated)
For information only at present.
On 2014/06/17 10:06:38, aberent wrote: > For information only at present. Now ready for real review.
https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... File build/android/pylib/instrumentation/test_package.py (right): https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... build/android/pylib/instrumentation/test_package.py:22: self._support_apk_name = "%sSupport%s" % ( I don't think hardcoding this is a good idea. Can we specify the support apk name in another way?
On 2014/06/17 20:17:08, jbudorick wrote: > https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... > File build/android/pylib/instrumentation/test_package.py (right): > > https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... > build/android/pylib/instrumentation/test_package.py:22: self._support_apk_name = > "%sSupport%s" % ( > I don't think hardcoding this is a good idea. Can we specify the support apk > name in another way? I definitely think we should hardcode it. What's the benefit of not hardcoding it? The only thing I see is that it would make it easier to support other names or to support multiple such apks. I really don't think that's worth it (primarily as those are things I don't foresee us needing and we'd be better off implementing it when we need it than now). Maybe if we had a good solution for http://crbug.com/378598 then I would accept using something similar here. I don't think that the way we currently handle isolate file paths is a good solution (though it is solving a harder problem than this), and I wouldn't want to see us duplicate that here. Also, we already hardcode all kinds of things based off of the --test-apk argument (test_apk_path, test_apk_jar_path, etc). Maybe this isn't *where* we should hardcode it, though.
https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... File build/android/pylib/instrumentation/test_package.py (right): https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... build/android/pylib/instrumentation/test_package.py:22: self._support_apk_name = "%sSupport%s" % ( On 2014/06/17 20:17:08, jbudorick wrote: > I don't think hardcoding this is a good idea. Can we specify the support apk > name in another way? We could add it as an extra instrumentation option to test_runner.py , but I don't like this because it makes the command more complicated and hence easier to get wrong (particularly since only some groups of instrumentation tests need this). I am not sure what other options there are.
On 2014/06/17 20:26:21, cjhopman wrote: > On 2014/06/17 20:17:08, jbudorick wrote: > > > https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... > > File build/android/pylib/instrumentation/test_package.py (right): > > > > > https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... > > build/android/pylib/instrumentation/test_package.py:22: self._support_apk_name > = > > "%sSupport%s" % ( > > I don't think hardcoding this is a good idea. Can we specify the support apk > > name in another way? > > I definitely think we should hardcode it. > > What's the benefit of not hardcoding it? The only thing I see is that it would > make it easier to support other names or to support multiple such apks. I really > don't think that's worth it (primarily as those are things I don't foresee us > needing and we'd be better off implementing it when we need it than now). Maybe > if we had a good solution for http://crbug.com/378598 then I would accept using > something similar here. I don't think that the way we currently handle isolate > file paths is a good solution (though it is solving a harder problem than this), > and I wouldn't want to see us duplicate that here. > This magical solution is probably what I'd like this to be. Alas. > Also, we already hardcode all kinds of things based off of the --test-apk > argument (test_apk_path, test_apk_jar_path, etc). > Those two are also related in java_apk.gypi, but point taken. > Maybe this isn't *where* we should hardcode it, though. I think I can agree with this.
On 2014/06/17 20:52:54, jbudorick wrote: > On 2014/06/17 20:26:21, cjhopman wrote: > > On 2014/06/17 20:17:08, jbudorick wrote: > > > > > > https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... > > > File build/android/pylib/instrumentation/test_package.py (right): > > > > > > > > > https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... > > > build/android/pylib/instrumentation/test_package.py:22: > self._support_apk_name > > = > > > "%sSupport%s" % ( > > > I don't think hardcoding this is a good idea. Can we specify the support apk > > > name in another way? > > > > I definitely think we should hardcode it. > > > > What's the benefit of not hardcoding it? The only thing I see is that it would > > make it easier to support other names or to support multiple such apks. I > really > > don't think that's worth it (primarily as those are things I don't foresee us > > needing and we'd be better off implementing it when we need it than now). > Maybe > > if we had a good solution for http://crbug.com/378598 then I would accept > using > > something similar here. I don't think that the way we currently handle isolate > > file paths is a good solution (though it is solving a harder problem than > this), > > and I wouldn't want to see us duplicate that here. > > > > This magical solution is probably what I'd like this to be. Alas. Yeah, I guess I don't really like any of the ways that we are currently finding these things. The isolate_file_paths way I like the least. As a user, it just seems like some magical stuff and when it breaks it is hard to figure out why. We also can't seem to inject that configuration for targets in the internal repo. When I edit build files, it is completely unclear to me what assumptions these scripts make about the build (that problem also exists between the different build scripts). When I edit these scripts, I have the same problem. I'm no owner of this stuff, though, so if you feel strongly don't let me stop you. > > > Also, we already hardcode all kinds of things based off of the --test-apk > > argument (test_apk_path, test_apk_jar_path, etc). > > > > Those two are also related in java_apk.gypi, but point taken. I don't know if it is any safer to rely on the current behavior of java_apk.gypi than it is to rely on whatever documentation we would have that tells users to name their test services apk FooTestSupport.apk. > > > Maybe this isn't *where* we should hardcode it, though. > > I think I can agree with this.
On 2014/06/17 21:09:28, cjhopman wrote: > On 2014/06/17 20:52:54, jbudorick wrote: > > On 2014/06/17 20:26:21, cjhopman wrote: > > > On 2014/06/17 20:17:08, jbudorick wrote: > > > > > > > > > > https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... > > > > File build/android/pylib/instrumentation/test_package.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... > > > > build/android/pylib/instrumentation/test_package.py:22: > > self._support_apk_name > > > = > > > > "%sSupport%s" % ( > > > > I don't think hardcoding this is a good idea. Can we specify the support > apk > > > > name in another way? > > > > > > I definitely think we should hardcode it. > > > > > > What's the benefit of not hardcoding it? The only thing I see is that it > would > > > make it easier to support other names or to support multiple such apks. I > > really > > > don't think that's worth it (primarily as those are things I don't foresee > us > > > needing and we'd be better off implementing it when we need it than now). > > Maybe > > > if we had a good solution for http://crbug.com/378598 then I would accept > > using > > > something similar here. I don't think that the way we currently handle > isolate > > > file paths is a good solution (though it is solving a harder problem than > > this), > > > and I wouldn't want to see us duplicate that here. > > > > > > > This magical solution is probably what I'd like this to be. Alas. > > Yeah, I guess I don't really like any of the ways that we are currently finding > these things. The isolate_file_paths way I like the least. As a user, it just > seems like some magical stuff and when it breaks it is hard to figure out why. > We also can't seem to inject that configuration for targets in the internal > repo. When I edit build files, it is completely unclear to me what assumptions > these scripts make about the build (that problem also exists between the > different build scripts). When I edit these scripts, I have the same problem. > > I'm no owner of this stuff, though, so if you feel strongly don't let me stop > you. > > > > > > Also, we already hardcode all kinds of things based off of the --test-apk > > > argument (test_apk_path, test_apk_jar_path, etc). > > > > > > > Those two are also related in java_apk.gypi, but point taken. > > I don't know if it is any safer to rely on the current behavior of java_apk.gypi > than it is to rely on whatever documentation we would have that tells users to > name their test services apk FooTestSupport.apk. > > > > > > Maybe this isn't *where* we should hardcode it, though. > > > > I think I can agree with this. I am not sure where a better place for it is. I could move it to the Install function itself, but I don't think that is what either of you are suggesting. Putting it anywhere else either means checking for its existence in a different place or passing around its name or path through a number of levels without checking for its existence. Both these would, I think, be more error prone for future maintainers. The first would separate its existence check from that of the test apk, and the second could lead to future maintainers writing code elsewhere that assumes it exists. We could, of course, put the name generation into a function called from here (getSupportApkName(test_apk_name) or similar) but I am not sure where we would put this function. All our other name mangling seems to be done inline. Unfortunately there is, as far as I know, no way of sharing the name generation between the tests and the build system, since, in order to use java_apk.gypi to build the support apk we need its name in a gyp variable, and there is no way of generating gyp variables from python within a build (or of using gyp variables in the tests).
On 2014/06/18 09:20:11, aberent wrote: > On 2014/06/17 21:09:28, cjhopman wrote: > > On 2014/06/17 20:52:54, jbudorick wrote: > > > On 2014/06/17 20:26:21, cjhopman wrote: > > > > On 2014/06/17 20:17:08, jbudorick wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... > > > > > File build/android/pylib/instrumentation/test_package.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... > > > > > build/android/pylib/instrumentation/test_package.py:22: > > > self._support_apk_name > > > > = > > > > > "%sSupport%s" % ( > > > > > I don't think hardcoding this is a good idea. Can we specify the support > > apk > > > > > name in another way? > > > > > > > > I definitely think we should hardcode it. > > > > > > > > What's the benefit of not hardcoding it? The only thing I see is that it > > would > > > > make it easier to support other names or to support multiple such apks. I > > > really > > > > don't think that's worth it (primarily as those are things I don't foresee > > us > > > > needing and we'd be better off implementing it when we need it than now). > > > Maybe > > > > if we had a good solution for http://crbug.com/378598 then I would accept > > > using > > > > something similar here. I don't think that the way we currently handle > > isolate > > > > file paths is a good solution (though it is solving a harder problem than > > > this), > > > > and I wouldn't want to see us duplicate that here. > > > > > > > > > > This magical solution is probably what I'd like this to be. Alas. > > > > Yeah, I guess I don't really like any of the ways that we are currently > finding > > these things. The isolate_file_paths way I like the least. As a user, it just > > seems like some magical stuff and when it breaks it is hard to figure out why. > > We also can't seem to inject that configuration for targets in the internal > > repo. When I edit build files, it is completely unclear to me what assumptions > > these scripts make about the build (that problem also exists between the > > different build scripts). When I edit these scripts, I have the same problem. > > > > I'm no owner of this stuff, though, so if you feel strongly don't let me stop > > you. > > > > > > > > > Also, we already hardcode all kinds of things based off of the --test-apk > > > > argument (test_apk_path, test_apk_jar_path, etc). > > > > > > > > > > Those two are also related in java_apk.gypi, but point taken. > > > > I don't know if it is any safer to rely on the current behavior of > java_apk.gypi > > than it is to rely on whatever documentation we would have that tells users to > > name their test services apk FooTestSupport.apk. > > > > > > > > > Maybe this isn't *where* we should hardcode it, though. > > > > > > I think I can agree with this. > > I am not sure where a better place for it is. I could move it to the Install > function itself, but I don't think that is what either of you are suggesting. > Putting it anywhere else either means checking for its existence in a different > place or passing around its name or path through a number of levels without > checking for its existence. Both these would, I think, be more error prone for > future maintainers. The first would separate its existence check from that of > the test apk, and the second could lead to future maintainers writing code > elsewhere that assumes it exists. > FWIW, the latter is how both test_apk and test_apk_jar are handled - both are generated in b/a/test_runner.py:ParseInstrumentationOptions, but aren't checked for existence until TestPackage.__init__ and TestJar.__init__, respectively. In the absence of a good solution, I think it might be best to be consistent with how those are handled. This would also make it minimally painful to add a --test-support-apk option if one proves necessary, although I would _really_ like to avoid that (or at least, avoid requiring it). > We could, of course, put the name generation into a function called from here > (getSupportApkName(test_apk_name) or similar) but I am not sure where we would > put this function. All our other name mangling seems to be done inline. > > Unfortunately there is, as far as I know, no way of sharing the name generation > between the tests and the build system, since, in order to use java_apk.gypi to > build the support apk we need its name in a gyp variable, and there is no way of > generating gyp variables from python within a build (or of using gyp variables > in the tests). Out of curiosity, how are you handling these support apks in the build system?
On 2014/06/18 15:25:31, jbudorick wrote: > On 2014/06/18 09:20:11, aberent wrote: > > On 2014/06/17 21:09:28, cjhopman wrote: > > > On 2014/06/17 20:52:54, jbudorick wrote: > > > > On 2014/06/17 20:26:21, cjhopman wrote: > > > > > On 2014/06/17 20:17:08, jbudorick wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... > > > > > > File build/android/pylib/instrumentation/test_package.py (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/336373004/diff/1/build/android/pylib/instrume... > > > > > > build/android/pylib/instrumentation/test_package.py:22: > > > > self._support_apk_name > > > > > = > > > > > > "%sSupport%s" % ( > > > > > > I don't think hardcoding this is a good idea. Can we specify the > support > > > apk > > > > > > name in another way? > > > > > > > > > > I definitely think we should hardcode it. > > > > > > > > > > What's the benefit of not hardcoding it? The only thing I see is that it > > > would > > > > > make it easier to support other names or to support multiple such apks. > I > > > > really > > > > > don't think that's worth it (primarily as those are things I don't > foresee > > > us > > > > > needing and we'd be better off implementing it when we need it than > now). > > > > Maybe > > > > > if we had a good solution for http://crbug.com/378598 then I would > accept > > > > using > > > > > something similar here. I don't think that the way we currently handle > > > isolate > > > > > file paths is a good solution (though it is solving a harder problem > than > > > > this), > > > > > and I wouldn't want to see us duplicate that here. > > > > > > > > > > > > > This magical solution is probably what I'd like this to be. Alas. > > > > > > Yeah, I guess I don't really like any of the ways that we are currently > > finding > > > these things. The isolate_file_paths way I like the least. As a user, it > just > > > seems like some magical stuff and when it breaks it is hard to figure out > why. > > > We also can't seem to inject that configuration for targets in the internal > > > repo. When I edit build files, it is completely unclear to me what > assumptions > > > these scripts make about the build (that problem also exists between the > > > different build scripts). When I edit these scripts, I have the same > problem. > > > > > > I'm no owner of this stuff, though, so if you feel strongly don't let me > stop > > > you. > > > > > > > > > > > > Also, we already hardcode all kinds of things based off of the > --test-apk > > > > > argument (test_apk_path, test_apk_jar_path, etc). > > > > > > > > > > > > > Those two are also related in java_apk.gypi, but point taken. > > > > > > I don't know if it is any safer to rely on the current behavior of > > java_apk.gypi > > > than it is to rely on whatever documentation we would have that tells users > to > > > name their test services apk FooTestSupport.apk. > > > > > > > > > > > > Maybe this isn't *where* we should hardcode it, though. > > > > > > > > I think I can agree with this. > > > > I am not sure where a better place for it is. I could move it to the Install > > function itself, but I don't think that is what either of you are suggesting. > > Putting it anywhere else either means checking for its existence in a > different > > place or passing around its name or path through a number of levels without > > checking for its existence. Both these would, I think, be more error prone for > > future maintainers. The first would separate its existence check from that of > > the test apk, and the second could lead to future maintainers writing code > > elsewhere that assumes it exists. > > > > FWIW, the latter is how both test_apk and test_apk_jar are handled - both are > generated in b/a/test_runner.py:ParseInstrumentationOptions, but aren't checked > for existence until TestPackage.__init__ and TestJar.__init__, respectively. In > the absence of a good solution, I think it might be best to be consistent with > how those are handled. > > This would also make it minimally painful to add a --test-support-apk option if > one proves necessary, although I would _really_ like to avoid that (or at least, > avoid requiring it). Okay, I will create a revised patch that does this. Not sure if I will quite have time before I go home tonight (I have to leave in the next 90 minutes). > > > We could, of course, put the name generation into a function called from here > > (getSupportApkName(test_apk_name) or similar) but I am not sure where we would > > put this function. All our other name mangling seems to be done inline. > > > > Unfortunately there is, as far as I know, no way of sharing the name > generation > > between the tests and the build system, since, in order to use java_apk.gypi > to > > build the support apk we need its name in a gyp variable, and there is no way > of > > generating gyp variables from python within a build (or of using gyp variables > > in the tests). > > Out of curiosity, how are you handling these support apks in the build system? See https://chrome-internal-review.googlesource.com/#/c/166395/2/native/framework...
PTAL
lgtm with nits I still don't really like this solution, but I'll admit that it's the best option realistically available. +craigdh for a real lgtm https://codereview.chromium.org/336373004/diff/20001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/336373004/diff/20001/build/android/test_runne... build/android/test_runner.py:280: options.test_support_apk_path = "%sSupport%s" % ( two nits: 1) single quote strings 2) can this just be: options.test_support_apk_path = '%Support%s' % ( os.path.splitext(options.test_apk_path)) ?
lgtm once jbudorick's comments are addressed
https://codereview.chromium.org/336373004/diff/20001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/336373004/diff/20001/build/android/test_runne... build/android/test_runner.py:280: options.test_support_apk_path = "%sSupport%s" % ( Both done. On 2014/06/19 15:35:09, jbudorick wrote: > two nits: > > 1) single quote strings > > 2) can this just be: > > options.test_support_apk_path = '%Support%s' % ( > os.path.splitext(options.test_apk_path)) > > ?
The CQ bit was checked by aberent@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aberent@chromium.org/336373004/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by aberent@chromium.org
On 2014/06/21 00:07:03, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win_chromium_rel on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) The failed jobs appear to be nothing to do with my change, so retrying.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aberent@chromium.org/336373004/40001
Message was sent while issue was closed.
Change committed as 279096 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
