Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18358005:
Adds modules_unittest to ndk-apk script. (Closed)
|
Created:
7 years, 5 months ago by hellner1 Modified:
7 years, 5 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org, Isaac (away), kjellander_chromium Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWebRTC: adds webrtc specific unit tests to test_runner.py and gtest_config.py for running them in a chromium workspace.
BUG=https://code.google.com/p/webrtc/issues/detail?id=1882
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210534
Patch Set 1 #
Total comments: 6
Patch Set 2 : Updates according to comments. #Patch Set 3 : Updates according to comments #
Total comments: 1
Patch Set 4 : Updates according to comments. #Patch Set 5 : Reverted to old implementation as the new implementation depended on a now reverted cl (18233018). #Patch Set 6 : I changed the location of the resource files according to a comment by andrew. This cl matches that. #Messages
Total messages: 19 (0 generated)
Once http://webrtc-codereview.appspot.com/1738004/ (or equivalent) is committed, WebRTC unit tests should be build-able and run-able. This cl makes sure that they are also available as targets for the ndk-apk framework.
fsamuel can you please review this cl?
https://codereview.chromium.org/18358005/diff/1/build/android/pylib/gtest/tes... File build/android/pylib/gtest/test_runner.py (right): https://codereview.chromium.org/18358005/diff/1/build/android/pylib/gtest/tes... build/android/pylib/gtest/test_runner.py:193: elif test_suite_basename == 'modules_unittests': We're moving to using isolate files instead of hardcoding deps here. Are these tests run on other platforms?
Yes, the tests are run on other platforms. However, for non-Android we run all tests locally in https://code.google.com/p/webrtc/. We are planning to run the tests in a Chromium workspace since we can't pull out what is needed for the ndk-apk testing in a nice way (due to dependencies on base etc).
https://codereview.chromium.org/18358005/diff/1/build/android/pylib/gtest/tes... File build/android/pylib/gtest/test_runner.py (right): https://codereview.chromium.org/18358005/diff/1/build/android/pylib/gtest/tes... build/android/pylib/gtest/test_runner.py:193: elif test_suite_basename == 'modules_unittests': So the goal is to run these tests on Android buildbots on chromium waterfall right? If so, it makes sense to create an isolate file for it (See https://codereview.chromium.org/17267004/). On 2013/07/01 18:14:47, frankf wrote: > We're moving to using isolate files instead of hardcoding deps here. Are these > tests run on other platforms? https://codereview.chromium.org/18358005/diff/1/build/android/pylib/gtest/tes... build/android/pylib/gtest/test_runner.py:196: 'third_party/data', I don't see these thirdparty directories in my chromium checkout? Also, how large are these directories?
I'm not sure why I'm listed as a reviewer here. I haven't been involved in WebRTC development.
So something like patch set 2 + https://webrtc-codereview.appspot.com/1750004? fsamuel: sorry, added you since you were the last to review the file.
On 2013/07/01 19:10:37, hellner1 wrote: > So something like patch set 2 + https://webrtc-codereview.appspot.com/1750004? > > fsamuel: sorry, added you since you were the last to review the file. Yes, that looks good. Could you address my other questions though?
Sorry for the slow reply. I had to prioritize http://webrtc-codereview.appspot.com/1738004/ as the reviewer is going on leave very soon. Please let me know if any questions remain unanswered and thanks for the speedy reviews! https://codereview.chromium.org/18358005/diff/1/build/android/pylib/gtest/tes... File build/android/pylib/gtest/test_runner.py (right): https://codereview.chromium.org/18358005/diff/1/build/android/pylib/gtest/tes... build/android/pylib/gtest/test_runner.py:193: elif test_suite_basename == 'modules_unittests': On 2013/07/01 18:32:38, frankf wrote: > So the goal is to run these tests on Android buildbots on chromium waterfall > right? If so, it makes sense to create an isolate file for it (See > https://codereview.chromium.org/17267004/). .isolate-file in http://webrtc-codereview.appspot.com/1750004/. Regarding being in the waterfall: we want to run it for every cl submitted to WebRTC but we don't want it to flag a break in Chrome. We make cuts from WebRTC to Chromium on a weekly basis. I.e. the tests we want to run are on a modified Chromium workspace where we replace the WebRTC cut (= stable) with ToT (= trunk, I think e.g. http://chromegw/i/chromium.webrtc/waterfall should have similar setup for some test cases). Henrik Kjellander will help out with the bot side of things. Here is pretty much how the workspace would need to be configured (or something equivalent. There might be some minor changes as I'm landing http://webrtc-codereview.appspot.com/1738004/ in WebRTC): * Download chromium android workspace using the normal steps. * Remove webrtc from .DEPS.git or DEPS (depending on how the workspace was set up git/svn) * Run ". build/android/envsetup.sh" * GYP_DEFINES=$GYP_DEFINES' include_tests=1' (this will ensure that the third_party/resources will be downloaded and the WebRTC tests will be available) * Add the following lines to .gclient: " { "name" : "src/third_party/webrtc", "url" : "https://webrtc.googlecode.com/svn/trunk/webrtc", "deps_file" : "DEPS", "managed" : True, "custom_deps" : { }, "safesync_url": "", }, { "name" : "src/third_party/data", "url" : "https://webrtc.googlecode.com/svn/trunk/data", "deps_file" : "DEPS", "managed" : True, "custom_deps" : { }, "safesync_url": "", }, { "name" : "src/third_party/google-gflags", "url" : "https://webrtc.googlecode.com/svn/trunk/third_party/google-gflags", "deps_file" : "DEPS", "managed" : True, "custom_deps" : { }, "safesync_url": "", }, { "name" : "src/third_party/google-gflags/src", "url" : "https://google-gflags.googlecode.com/svn/trunk/src", "deps_file" : "DEPS", "managed" : True, "custom_deps" : { }, "safesync_url": "", }, " > On 2013/07/01 18:14:47, frankf wrote: > > We're moving to using isolate files instead of hardcoding deps here. Are these > > tests run on other platforms? > Yes, but local to WebRTC. Done. https://codereview.chromium.org/18358005/diff/1/build/android/pylib/gtest/tes... build/android/pylib/gtest/test_runner.py:196: 'third_party/data', On 2013/07/01 18:32:38, frankf wrote: > I don't see these thirdparty directories in my chromium checkout? Also, how > large are these directories? third_party/resources (214.8 MB) are loaded from a remote (to Chrome and WebRTC) repository by this script: https://code.google.com/p/webrtc/source/browse/trunk/webrtc/tools/update.py (which is about to be renamed to update_resources.py). This is a gyp-action when building module_unittests which only happens if "include_tests==1". third_party/data (31.8 MB) is from here: https://code.google.com/p/webrtc/source/browse/#svn%2Ftrunk%2Fdata
https://codereview.chromium.org/18358005/diff/1/build/android/pylib/gtest/tes... File build/android/pylib/gtest/test_runner.py (right): https://codereview.chromium.org/18358005/diff/1/build/android/pylib/gtest/tes... build/android/pylib/gtest/test_runner.py:196: 'third_party/data', 200MB is a large amount of data to push to the device, and it may cause timeouts. Is it possible to prune this? On 2013/07/02 14:49:25, hellner1 wrote: > On 2013/07/01 18:32:38, frankf wrote: > > I don't see these thirdparty directories in my chromium checkout? Also, how > > large are these directories? > > third_party/resources (214.8 MB) are loaded from a remote (to Chrome and WebRTC) > repository by this script: > https://code.google.com/p/webrtc/source/browse/trunk/webrtc/tools/update.py > (which is about to be renamed to update_resources.py). This is a gyp-action when > building module_unittests which only happens if > "include_tests==1". > > third_party/data (31.8 MB) is from here: > https://code.google.com/p/webrtc/source/browse/#svn%252Ftrunk%252Fdata https://codereview.chromium.org/18358005/diff/13001/build/android/pylib/gtest... File build/android/pylib/gtest/gtest_config.py (right): https://codereview.chromium.org/18358005/diff/13001/build/android/pylib/gtest... build/android/pylib/gtest/gtest_config.py:31: Apk('modules_unittests'), As the comment above states, by adding this here, it will attempt to get run on all Android bots, which obviously is gonna fail since you require a special environment. However, you can pass arbitrary suite names to as such: build/android/run_tests.py -s modules_unittests So you can modify the buildbot scripts in build/android/buildbot to have a config for webrtc bot which calls the above command (I've cc'ed ilevy who owns the buildbot scripts)
On 2013/07/02 17:20:23, frankf wrote: > https://codereview.chromium.org/18358005/diff/1/build/android/pylib/gtest/tes... > File build/android/pylib/gtest/test_runner.py (right): > > https://codereview.chromium.org/18358005/diff/1/build/android/pylib/gtest/tes... > build/android/pylib/gtest/test_runner.py:196: 'third_party/data', > 200MB is a large amount of data to push to the device, and it may cause > timeouts. Is it possible to prune this? I'll have to get back to you regarding whether or not we can prune here. The issue is that we are using video sequences in our tests (e.g. to detect degradation in psnr). Additionally, I think this would take some time to implement. Since the WebRTC tests and files should not affect the Chromium tree in any way (include_tests==0), can we proceed and start investigating this separately? Once the files have been loaded to a device will it be smart about copying them next time? > On 2013/07/02 14:49:25, hellner1 wrote: > > On 2013/07/01 18:32:38, frankf wrote: > > > I don't see these thirdparty directories in my chromium checkout? Also, how > > > large are these directories? > > > > third_party/resources (214.8 MB) are loaded from a remote (to Chrome and > WebRTC) > > repository by this script: > > https://code.google.com/p/webrtc/source/browse/trunk/webrtc/tools/update.py > > (which is about to be renamed to update_resources.py). This is a gyp-action > when > > building module_unittests which only happens if > > "include_tests==1". > > > > third_party/data (31.8 MB) is from here: > > https://code.google.com/p/webrtc/source/browse/#svn%25252Ftrunk%25252Fdata > > https://codereview.chromium.org/18358005/diff/13001/build/android/pylib/gtest... > File build/android/pylib/gtest/gtest_config.py (right): > > https://codereview.chromium.org/18358005/diff/13001/build/android/pylib/gtest... > build/android/pylib/gtest/gtest_config.py:31: Apk('modules_unittests'), > As the comment above states, by adding this here, it will attempt to get run on > all Android bots, which obviously is gonna fail since you require a special > environment. > > However, you can pass arbitrary suite names to as such: > > build/android/run_tests.py -s modules_unittests I see that now. I updated (instructions and files) accordingly. PTAL. > So you can modify the buildbot scripts in > build/android/buildbot to have a config for webrtc bot which calls the above > command (I've cc'ed ilevy who owns the buildbot scripts) Thanks. I added kjellander as well as he has been setting up the WebRTC bots.
lgtm. Yes, we sync the deps and push only if it has changed.
Seems the cl referenced got reverted: http://src.chromium.org/viewvc/chrome/trunk/src/build/android/pylib/gtest/tes... Should I go back to the original cl?
On 2013/07/08 21:24:47, hellner1 wrote: > Seems the cl referenced got reverted: > http://src.chromium.org/viewvc/chrome/trunk/src/build/android/pylib/gtest/tes... > > Should I go back to the original cl? Yes, sorry for the inconvenience. https://codereview.chromium.org/18233018/ will change how we generate isolate files and should land by end of the week. If you need this before that, I'm OK with the original patch (minus the change to gtest_config.py) going in, and I'll change it to use the isolate file. Please land http://webrtc-codereview.appspot.com/1750004/ in preparation for this.
http://webrtc-codereview.appspot.com/1750004/ has landed. I attached myself as cc on the cl you are referring to so I should be notified when it lands. I'm fine with making the appropriate changes, once it does, if you want me to. The Chrome waterfall should be unaffected even if it happens to be broken. I'd like to land this now as it is the last step for kjellander to start working on the bots. There was no inconvenience, quite the opposite. Thanks for the speedy reviews. PTAL just to be sure :)
lgtm. Please run android try bots.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hellner@chromium.org/18358005/23001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hellner@chromium.org/18358005/27004
Message was sent while issue was closed.
Change committed as 210534 |
