Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1353033003:
Isolate base_unittests_apk. (Closed)
|
Created:
5 years, 3 months ago by ghost stip (do not use) Modified:
5 years, 2 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIsolate base_unittests_apk.
BUG=525873
Committed: https://crrev.com/e70224861766636cb15fe2d33ecd1912f5a9c8da
Cr-Commit-Position: refs/heads/master@{#351906}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Include base_unittests.isolate. #
Total comments: 4
Patch Set 3 : Rebase onto latest master. #Patch Set 4 : A broken patchset. #Patch Set 5 : Back to working. #
Total comments: 2
Patch Set 6 : Remove blank link. #Messages
Total messages: 27 (5 generated)
stip@chromium.org changed reviewers: + jam@chromium.org, jbudorick@chromium.org, maruel@chromium.org
I'm not 100% sure where the build/config and swarming_client isolation should go. Either way the tests ran OK on my device with these files. jbudorick/maruel: review jam: OWNERS ptal! https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... File base/base_unittests_apk.isolate (right): https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... base/base_unittests_apk.isolate:14: '../build/config/', maybe this should go into android.isolate? https://codereview.chromium.org/1353033003/diff/1/build/android/android.isolate File build/android/android.isolate (right): https://codereview.chromium.org/1353033003/diff/1/build/android/android.isola... build/android/android.isolate:12: '../../tools/swarming_client/', I don't know if this is the right place to put it. maybe base_unittests_apk.isolate should include it directly
https://codereview.chromium.org/1353033003/diff/1/build/android/android.isolate File build/android/android.isolate (right): https://codereview.chromium.org/1353033003/diff/1/build/android/android.isola... build/android/android.isolate:12: '../../tools/swarming_client/', On 2015/09/18 01:13:12, stip wrote: > I don't know if this is the right place to put it. maybe > base_unittests_apk.isolate should include it directly Why it is needed?
https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... File base/base_unittests_apk.isolate (right): https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... base/base_unittests_apk.isolate:7: '../build/android/android.isolate', this should be able to include base_unittests.isolate instead of having e.g. test/data/ in files below https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... base/base_unittests_apk.isolate:14: '../build/config/', On 2015/09/18 at 01:13:12, stip wrote: > maybe this should go into android.isolate? please don't tell me this is only in here for //build/config/win/msvs_dependencies.isolate that's included in base.isolate :(
On 2015/09/18 01:17:11, jbudorick wrote: > https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... > File base/base_unittests_apk.isolate (right): > > https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... > base/base_unittests_apk.isolate:7: '../build/android/android.isolate', > this should be able to include base_unittests.isolate instead of having e.g. > test/data/ in files below > > https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... > base/base_unittests_apk.isolate:14: '../build/config/', > On 2015/09/18 at 01:13:12, stip wrote: > > maybe this should go into android.isolate? > > please don't tell me this is only in here for > //build/config/win/msvs_dependencies.isolate that's included in base.isolate :( It is. I was confused myself, but it appears to be part of the remap process.
On 2015/09/18 01:16:45, M-A Ruel wrote: > https://codereview.chromium.org/1353033003/diff/1/build/android/android.isolate > File build/android/android.isolate (right): > > https://codereview.chromium.org/1353033003/diff/1/build/android/android.isola... > build/android/android.isolate:12: '../../tools/swarming_client/', > On 2015/09/18 01:13:12, stip wrote: > > I don't know if this is the right place to put it. maybe > > base_unittests_apk.isolate should include it directly > > Why it is needed? "It didn't work, I added it and it worked." It appears that the tests call isolate.py remap, so swarming_client and build/config/win/msvs_dependencies.isolate is needed.
It's because when the framework uses isolate.py remap, it parses the mapped .isolate files. Oh well, let's just ignore this problem for now, as it's not materially important, just semantically annoying. https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... File base/base_unittests_apk.isolate (right): https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... base/base_unittests_apk.isolate:7: '../build/android/android.isolate', On 2015/09/18 01:17:10, jbudorick wrote: > this should be able to include base_unittests.isolate instead of having e.g. > test/data/ in files below I agree, include base.isolate instead.
stip@chromium.org changed reviewers: + danakj@chromium.org - jam@chromium.org
ptal also actual OWNERS this time (danakj) https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... File base/base_unittests_apk.isolate (right): https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... base/base_unittests_apk.isolate:7: '../build/android/android.isolate', On 2015/09/18 01:17:10, jbudorick wrote: > this should be able to include base_unittests.isolate instead of having e.g. > test/data/ in files below done https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... base/base_unittests_apk.isolate:14: '../build/config/', On 2015/09/18 01:17:11, jbudorick wrote: > On 2015/09/18 at 01:13:12, stip wrote: > > maybe this should go into android.isolate? > > please don't tell me this is only in here for > //build/config/win/msvs_dependencies.isolate that's included in base.isolate :( it is. base.isolate *includes* msvs_dependencies.isolate, it doesn't map it in. the isolate remap functionality needs the .isolate file itself https://codereview.chromium.org/1353033003/diff/1/build/android/android.isolate File build/android/android.isolate (right): https://codereview.chromium.org/1353033003/diff/1/build/android/android.isola... build/android/android.isolate:12: '../../tools/swarming_client/', On 2015/09/18 01:16:45, M-A Ruel wrote: > On 2015/09/18 01:13:12, stip wrote: > > I don't know if this is the right place to put it. maybe > > base_unittests_apk.isolate should include it directly > > Why it is needed? many android tests require isolate.py for remapping https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... File base/base_unittests_apk.isolate (right): https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... base/base_unittests_apk.isolate:21: 'base.isolate', both base.isolate and base_unittests.isolate need to be here as well for isolate remap to work
https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... File base/base_unittests_apk.isolate (right): https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... base/base_unittests_apk.isolate:21: 'base.isolate', On 2015/09/29 at 07:18:48, stip wrote: > both base.isolate and base_unittests.isolate need to be here as well for isolate remap to work Whatever is required for remap shouldn't be in here. It should instead be either in build/android/android.isolate or a separate .isolate for android test suites that have data dependencies (which is not all of them).
https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... File base/base_unittests_apk.isolate (right): https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... base/base_unittests_apk.isolate:21: 'base.isolate', On 2015/09/29 08:43:54, jbudorick wrote: > On 2015/09/29 at 07:18:48, stip wrote: > > both base.isolate and base_unittests.isolate need to be here as well for > isolate remap to work > > Whatever is required for remap shouldn't be in here. It should instead be either > in build/android/android.isolate or a separate .isolate for android test suites > that have data dependencies (which is not all of them). that file will have to include base.isolate, breakpad.isolate, and every other test suite's data. I don't think that's what you want
https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... File base/base_unittests_apk.isolate (right): https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... base/base_unittests_apk.isolate:21: 'base.isolate', On 2015/09/29 at 17:35:11, stip wrote: > On 2015/09/29 08:43:54, jbudorick wrote: > > On 2015/09/29 at 07:18:48, stip wrote: > > > both base.isolate and base_unittests.isolate need to be here as well for > > isolate remap to work > > > > Whatever is required for remap shouldn't be in here. It should instead be either > > in build/android/android.isolate or a separate .isolate for android test suites > > that have data dependencies (which is not all of them). > > that file will have to include base.isolate, breakpad.isolate, and every other test suite's data. I don't think that's what you want Not sure we're on the same page. Why would it require every other test suite's data...?
On 2015/09/29 17:36:25, jbudorick wrote: > https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... > File base/base_unittests_apk.isolate (right): > > https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... > base/base_unittests_apk.isolate:21: 'base.isolate', > On 2015/09/29 at 17:35:11, stip wrote: > > On 2015/09/29 08:43:54, jbudorick wrote: > > > On 2015/09/29 at 07:18:48, stip wrote: > > > > both base.isolate and base_unittests.isolate need to be here as well for > > > isolate remap to work > > > > > > Whatever is required for remap shouldn't be in here. It should instead be > either > > > in build/android/android.isolate or a separate .isolate for android test > suites > > > that have data dependencies (which is not all of them). > > > > that file will have to include base.isolate, breakpad.isolate, and every other > test suite's data. I don't think that's what you want > > Not sure we're on the same page. Why would it require every other test suite's > data...? somewhere base.isolate, base_unittests.isolate, breakpad_unittests.isolate, and a bunch of others need to be isolated themselves (not the files they isolate, the actual .isolates). I *could* put those in android.isolate or whatever, but then it's weird that android.isolate also isolates breakpad_unittests.isolate (and a bunch others) when only one test needs it.
On 2015/09/29 22:07:02, stip wrote: > On 2015/09/29 17:36:25, jbudorick wrote: > > > https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... > > File base/base_unittests_apk.isolate (right): > > > > > https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... > > base/base_unittests_apk.isolate:21: 'base.isolate', > > On 2015/09/29 at 17:35:11, stip wrote: > > > On 2015/09/29 08:43:54, jbudorick wrote: > > > > On 2015/09/29 at 07:18:48, stip wrote: > > > > > both base.isolate and base_unittests.isolate need to be here as well for > > > > isolate remap to work > > > > > > > > Whatever is required for remap shouldn't be in here. It should instead be > > either > > > > in build/android/android.isolate or a separate .isolate for android test > > suites > > > > that have data dependencies (which is not all of them). > > > > > > that file will have to include base.isolate, breakpad.isolate, and every > other > > test suite's data. I don't think that's what you want > > > > Not sure we're on the same page. Why would it require every other test suite's > > data...? > > somewhere base.isolate, base_unittests.isolate, breakpad_unittests.isolate, and > a bunch of others need to be isolated themselves (not the files they isolate, > the actual .isolates). I *could* put those in android.isolate or whatever, but > then it's weird that android.isolate also isolates breakpad_unittests.isolate > (and a bunch others) when only one test needs it. Mike is right, the chromium Android testing framework uses isolate.py remap to create a temporary tree to push to the device. You know that?
On 2015/09/29 at 22:08:56, maruel wrote: > On 2015/09/29 22:07:02, stip wrote: > > On 2015/09/29 17:36:25, jbudorick wrote: > > > > > https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... > > > File base/base_unittests_apk.isolate (right): > > > > > > > > https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... > > > base/base_unittests_apk.isolate:21: 'base.isolate', > > > On 2015/09/29 at 17:35:11, stip wrote: > > > > On 2015/09/29 08:43:54, jbudorick wrote: > > > > > On 2015/09/29 at 07:18:48, stip wrote: > > > > > > both base.isolate and base_unittests.isolate need to be here as well for > > > > > isolate remap to work > > > > > > > > > > Whatever is required for remap shouldn't be in here. It should instead be > > > either > > > > > in build/android/android.isolate or a separate .isolate for android test > > > suites > > > > > that have data dependencies (which is not all of them). > > > > > > > > that file will have to include base.isolate, breakpad.isolate, and every > > other > > > test suite's data. I don't think that's what you want > > > > > > Not sure we're on the same page. Why would it require every other test suite's > > > data...? > > > > somewhere base.isolate, base_unittests.isolate, breakpad_unittests.isolate, and > > a bunch of others need to be isolated themselves (not the files they isolate, > > the actual .isolates). I *could* put those in android.isolate or whatever, but > > then it's weird that android.isolate also isolates breakpad_unittests.isolate > > (and a bunch others) when only one test needs it. > We're definitely on different pages. Perhaps "Whatever is required for remap" was the wrong way to phrase my suggestion. isolate.py remap requires some things to run, not including the .isolate it's remapping. Those things (which, at a glance, at least include parts of //build/config, base.isolate, and //tools/swarming_client) should be somewhere that can be shared. > Mike is right, the chromium Android testing framework uses isolate.py remap to create a temporary tree to push to the device. You know that? Yes, I know how we use remap.
On 2015/09/30 08:59:40, jbudorick wrote: > On 2015/09/29 at 22:08:56, maruel wrote: > > On 2015/09/29 22:07:02, stip wrote: > > > On 2015/09/29 17:36:25, jbudorick wrote: > > > > > > > > https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... > > > > File base/base_unittests_apk.isolate (right): > > > > > > > > > > > > https://codereview.chromium.org/1353033003/diff/20001/base/base_unittests_apk... > > > > base/base_unittests_apk.isolate:21: 'base.isolate', > > > > On 2015/09/29 at 17:35:11, stip wrote: > > > > > On 2015/09/29 08:43:54, jbudorick wrote: > > > > > > On 2015/09/29 at 07:18:48, stip wrote: > > > > > > > both base.isolate and base_unittests.isolate need to be here as well > for > > > > > > isolate remap to work > > > > > > > > > > > > Whatever is required for remap shouldn't be in here. It should instead > be > > > > either > > > > > > in build/android/android.isolate or a separate .isolate for android > test > > > > suites > > > > > > that have data dependencies (which is not all of them). > > > > > > > > > > that file will have to include base.isolate, breakpad.isolate, and every > > > other > > > > test suite's data. I don't think that's what you want > > > > > > > > Not sure we're on the same page. Why would it require every other test > suite's > > > > data...? > > > > > > somewhere base.isolate, base_unittests.isolate, breakpad_unittests.isolate, > and > > > a bunch of others need to be isolated themselves (not the files they > isolate, > > > the actual .isolates). I *could* put those in android.isolate or whatever, > but > > > then it's weird that android.isolate also isolates > breakpad_unittests.isolate > > > (and a bunch others) when only one test needs it. > > > > We're definitely on different pages. Perhaps "Whatever is required for remap" > was the wrong way to phrase my suggestion. > > isolate.py remap requires some things to run, not including the .isolate it's > remapping. Those things (which, at a glance, at least include parts of > //build/config, base.isolate, and //tools/swarming_client) should be somewhere > that can be shared. > > > Mike is right, the chromium Android testing framework uses isolate.py remap to > create a temporary tree to push to the device. You know that? > > Yes, I know how we use remap. The swarming part is already in android.isolate, where you'd like it. That part is being committed in another CL so it will be removed once it lands and I rebase. base.isolate, //build/config and base_unittests.isolate are only used by base_unit tests.
https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... File base/base_unittests_apk.isolate (right): https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... base/base_unittests_apk.isolate:14: '../build/config/', On 2015/09/29 at 07:18:47, stip wrote: > On 2015/09/18 01:17:11, jbudorick wrote: > > On 2015/09/18 at 01:13:12, stip wrote: > > > maybe this should go into android.isolate? > > > > please don't tell me this is only in here for > > //build/config/win/msvs_dependencies.isolate that's included in base.isolate :( > > it is. base.isolate *includes* msvs_dependencies.isolate, it doesn't map it in. the isolate remap functionality needs the .isolate file itself ^ ?
https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... File base/base_unittests_apk.isolate (right): https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... base/base_unittests_apk.isolate:14: '../build/config/', On 2015/09/30 17:32:57, jbudorick wrote: > On 2015/09/29 at 07:18:47, stip wrote: > > On 2015/09/18 01:17:11, jbudorick wrote: > > > On 2015/09/18 at 01:13:12, stip wrote: > > > > maybe this should go into android.isolate? > > > > > > please don't tell me this is only in here for > > > //build/config/win/msvs_dependencies.isolate that's included in base.isolate > :( > > > > it is. base.isolate *includes* msvs_dependencies.isolate, it doesn't map it > in. the isolate remap functionality needs the .isolate file itself > > ^ ? Here is what happens when I build and run patchset 3: http://pastebin.com/bvhh9y56 Here is what happens when I build and run patchset 4: http://pastebin.com/qnncw2Ym. Note that the only difference between 3 and 4 is the removal of '../build/config' The error is "No such file or directory: u'/mnt/ssd/code/chromium-android/isolate-2015-09-30NMj5xJ/build/config/win/msvs_dependencies.isolate'", because without that line the file is not mapped in for remap to use it. What would you like me to do here?
On 2015/09/30 at 18:30:46, stip wrote: > https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... > File base/base_unittests_apk.isolate (right): > > https://codereview.chromium.org/1353033003/diff/1/base/base_unittests_apk.iso... > base/base_unittests_apk.isolate:14: '../build/config/', > On 2015/09/30 17:32:57, jbudorick wrote: > > On 2015/09/29 at 07:18:47, stip wrote: > > > On 2015/09/18 01:17:11, jbudorick wrote: > > > > On 2015/09/18 at 01:13:12, stip wrote: > > > > > maybe this should go into android.isolate? > > > > > > > > please don't tell me this is only in here for > > > > //build/config/win/msvs_dependencies.isolate that's included in base.isolate > > :( > > > > > > it is. base.isolate *includes* msvs_dependencies.isolate, it doesn't map it > > in. the isolate remap functionality needs the .isolate file itself > > > > ^ ? > > Here is what happens when I build and run patchset 3: http://pastebin.com/bvhh9y56 > > Here is what happens when I build and run patchset 4: http://pastebin.com/qnncw2Ym. Note that the only difference between 3 and 4 is the removal of '../build/config' > > The error is "No such file or directory: u'/mnt/ssd/code/chromium-android/isolate-2015-09-30NMj5xJ/build/config/win/msvs_dependencies.isolate'", because without that line the file is not mapped in for remap to use it. > > What would you like me to do here? Sorry, I was mistaken on this one. lgtm
thestig@chromium.org changed reviewers: + thestig@chromium.org
stampy stampy lgtm https://codereview.chromium.org/1353033003/diff/80001/base/base_unittests_apk... File base/base_unittests_apk.isolate (right): https://codereview.chromium.org/1353033003/diff/80001/base/base_unittests_apk... base/base_unittests_apk.isolate:1: extra blank linke
https://codereview.chromium.org/1353033003/diff/80001/base/base_unittests_apk... File base/base_unittests_apk.isolate (right): https://codereview.chromium.org/1353033003/diff/80001/base/base_unittests_apk... base/base_unittests_apk.isolate:1: On 2015/10/01 20:12:16, Lei Zhang wrote: > extra blank linke good catch
The CQ bit was checked by stip@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1353033003/#ps100001 (title: "Remove blank link.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1353033003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1353033003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e70224861766636cb15fe2d33ecd1912f5a9c8da Cr-Commit-Position: refs/heads/master@{#351906} |
