Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2142243003:
Add support for new iossim in Xcode 8. (Closed)
|
Created:
4 years, 5 months ago by justincohen Modified:
4 years, 5 months ago Reviewers:
smut CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/build@master Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionAdd support for new iossim in Xcode 8.
BUG=622717
Committed: https://chromium.googlesource.com/chromium/tools/build/+/342c826c7249d9f355ae45bc81a945a873a743db
Patch Set 1 #
Total comments: 12
Patch Set 2 : Comments #Patch Set 3 : Wipe home directory too #Messages
Total messages: 12 (4 generated)
Description was changed from ========== Add support for new iossim in Xcode 8. BUG= ========== to ========== Add support for new iossim in Xcode 8. BUG=622717 ==========
justincohen@chromium.org changed reviewers: + smut@google.com
PTAL. seems to work with ./run.py -a ~/work/bling/src/out/gn-Debug-iphonesimulator/base_unittests.app -i ~/work/bling/src/out/gn-Debug-iphonesimulator/iossim -p 'iPhone 6s' -v 10.0 but I don't have a ton of confidence that I'm able to confirm this works on a bot.
https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... File scripts/slave/ios/test_runner.py (right): https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... scripts/slave/ios/test_runner.py:490: self.xcode_version = xcode_version Can probably set this in the base class. https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... scripts/slave/ios/test_runner.py:506: ] Can we use -w to wipe the simulator here, so that every time CreateNewHomeDirectory is called we are getting a fresh simulator with a different homedir? https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... scripts/slave/ios/test_runner.py:515: self.homedir = '' I think it should shutil.rmtree even on Xcode 8 so we do the post-test cleanup and free up the disk space used to install and run the test. https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... scripts/slave/ios/test_runner.py:704: cmd.extend(['-w']) I don't think it should include -w here. GetLaunchCommand may be called multiple times to ensure all the test cases run even if one crashes and these should all be in the same simulator so we can get all the test data at the end. Won't -w erase the test data in the test's Documents directory when it wipes the simulator?
ptal. I pulled -w into it's own call instead of rmtree. wdyt? https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... File scripts/slave/ios/test_runner.py (right): https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... scripts/slave/ios/test_runner.py:490: self.xcode_version = xcode_version On 2016/07/12 23:58:59, smut wrote: > Can probably set this in the base class. Done. https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... scripts/slave/ios/test_runner.py:506: ] On 2016/07/12 23:58:59, smut wrote: > Can we use -w to wipe the simulator here, so that every time > CreateNewHomeDirectory is called we are getting a fresh simulator with a > different homedir? See below. https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... scripts/slave/ios/test_runner.py:515: self.homedir = '' On 2016/07/12 23:58:59, smut wrote: > I think it should shutil.rmtree even on Xcode 8 so we do the post-test cleanup > and free up the disk space used to install and run the test. Xcode seems to freak out. How about we just call -w here instead? https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... scripts/slave/ios/test_runner.py:704: cmd.extend(['-w']) On 2016/07/12 23:58:59, smut wrote: > I don't think it should include -w here. GetLaunchCommand may be called multiple > times to ensure all the test cases run even if one crashes and these should all > be in the same simulator so we can get all the test data at the end. Won't -w > erase the test data in the test's Documents directory when it wipes the > simulator? Removed.
https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... File scripts/slave/ios/test_runner.py (right): https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... scripts/slave/ios/test_runner.py:506: ] On 2016/07/14 19:59:50, justincohen wrote: > On 2016/07/12 23:58:59, smut wrote: > > Can we use -w to wipe the simulator here, so that every time > > CreateNewHomeDirectory is called we are getting a fresh simulator with a > > different homedir? > > See below. I think it should wipe here as well. This call that needs to ensure we get a new home directory. e.g. if CreateNewHomeDirectory is called twice in a row for Xcode 7 it will actually get a new home dir the second time, but for Xcode 8 it seems like it would just print the same thing out unless it was explicitly wiped. Am I wrong about that? https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... scripts/slave/ios/test_runner.py:515: self.homedir = '' On 2016/07/14 19:59:50, justincohen wrote: > On 2016/07/12 23:58:59, smut wrote: > > I think it should shutil.rmtree even on Xcode 8 so we do the post-test cleanup > > and free up the disk space used to install and run the test. > > Xcode seems to freak out. How about we just call -w here instead? Ok seems fine. -w should wipe the installed app and any data it wrote to the simulator and free up the disk space anyways, right?
ptal https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... File scripts/slave/ios/test_runner.py (right): https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... scripts/slave/ios/test_runner.py:506: ] On 2016/07/14 21:25:54, smut wrote: > On 2016/07/14 19:59:50, justincohen wrote: > > On 2016/07/12 23:58:59, smut wrote: > > > Can we use -w to wipe the simulator here, so that every time > > > CreateNewHomeDirectory is called we are getting a fresh simulator with a > > > different homedir? > > > > See below. > > I think it should wipe here as well. This call that needs to ensure we get a new > home directory. e.g. if CreateNewHomeDirectory is called twice in a row for > Xcode 7 it will actually get a new home dir the second time, but for Xcode 8 it > seems like it would just print the same thing out unless it was explicitly > wiped. Am I wrong about that? Wipe is very fast, this SGTM, done. https://codereview.chromium.org/2142243003/diff/1/scripts/slave/ios/test_runn... scripts/slave/ios/test_runner.py:515: self.homedir = '' On 2016/07/14 21:25:54, smut wrote: > On 2016/07/14 19:59:50, justincohen wrote: > > On 2016/07/12 23:58:59, smut wrote: > > > I think it should shutil.rmtree even on Xcode 8 so we do the post-test > cleanup > > > and free up the disk space used to install and run the test. > > > > Xcode seems to freak out. How about we just call -w here instead? > > Ok seems fine. -w should wipe the installed app and any data it wrote to the > simulator and free up the disk space anyways, right? yes/
Seems lgtm
The CQ bit was checked by justincohen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add support for new iossim in Xcode 8. BUG=622717 ========== to ========== Add support for new iossim in Xcode 8. BUG=622717 Committed: https://chromium.googlesource.com/chromium/tools/build/+/342c826c7249d9f355ae... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/tools/build/+/342c826c7249d9f355ae... |
