Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1985663002:
JUnit test framework for AppRTC Android demo. (Closed)
|
Created:
4 years, 7 months ago by sakal Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionJUnit test framework for AppRTC Android demo.
This allows creating tests for AppRTC Android demo that will be run on
the host machine instead of a device. These tests can mock Android APIs
through Robolectric. Because the tests are run on the host machine,
they run much faster.
BUG=webrtc:5896
NOTRY=True
Committed: https://crrev.com/ee3732622c0df433f62048d6f8e89e70a4561817
Cr-Commit-Position: refs/heads/master@{#12769}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove extra space in README #
Total comments: 15
Patch Set 3 : Changes according to comments #
Total comments: 1
Patch Set 4 : Update README to use wrapper script for running the tests #Messages
Total messages: 33 (12 generated)
Description was changed from ========== JUnit test framework for AppRTC Android demo. This allows creating tests for AppRTC Android demo that will be run on the host machine instead of a device. These tests can mock Android APIs through Robolectric. Because the tests are run on the host machine, they run much faster. BUG= ========== to ========== JUnit test framework for AppRTC Android demo. This allows creating tests for AppRTC Android demo that will be run on the host machine instead of a device. These tests can mock Android APIs through Robolectric. Because the tests are run on the host machine, they run much faster. BUG= ==========
sakal@webrtc.org changed reviewers: + kjellander@webrtc.org, magjed@webrtc.org
Is it possible to get try bots run these tests?
Magjed and kjellander, can you take a look, please?
Do we need to update webrtc/BUILD.gn as well? https://codereview.webrtc.org/1985663002/diff/1/webrtc/examples/androidjunit/... File webrtc/examples/androidjunit/README (right): https://codereview.webrtc.org/1985663002/diff/1/webrtc/examples/androidjunit/... webrtc/examples/androidjunit/README:1: This directory contains example JUnit tests for Android AppRTCDemo. nit: remove extra space between 'contains' and 'example'.
On 2016/05/16 10:13:35, magjed_webrtc wrote: > Do we need to update webrtc/BUILD.gn as well? I don't know. I don't see anything that seems like it would need changing, and the project compiles just fine. Maybe kjellander could comment on this issue since he knows the build system a lot better?
https://codereview.webrtc.org/1985663002/diff/1/webrtc/examples/androidjunit/... File webrtc/examples/androidjunit/README (right): https://codereview.webrtc.org/1985663002/diff/1/webrtc/examples/androidjunit/... webrtc/examples/androidjunit/README:1: This directory contains example JUnit tests for Android AppRTCDemo. On 2016/05/16 10:13:35, magjed_webrtc wrote: > nit: remove extra space between 'contains' and 'example'. Done.
I had a look around and it seems like AppRTC Demo doesn't have any entries in GN-based build system and is only compilable using GYP-based system.
On 2016/05/16 10:01:58, sakal wrote: > Is it possible to get try bots run these tests? Yes, you can do 'git cl try' (preferably specifying -b to select a few Android bots, to prevent loading down all the bots at the try server since this change is Android-only). You can also check bots from the 'tryserver.webrtc' section by clicking the "Choose trybots" link here in the Web UI.
On 2016/05/16 13:24:20, sakal wrote: > I had a look around and it seems like AppRTC Demo doesn't have any entries in > GN-based build system and is only compilable using GYP-based system. Right, we don't have any test code in GN yet.
I think we should adopt the way Chromium configures their similar tests, so we can get the wrapper scripts generated. https://codereview.webrtc.org/1985663002/diff/20001/webrtc/examples/androidju... File webrtc/examples/androidjunit/README (right): https://codereview.webrtc.org/1985663002/diff/20001/webrtc/examples/androidju... webrtc/examples/androidjunit/README:1: This directory contains example JUnit tests for Android AppRTCDemo. This is currently not yet true, but I assume you plan to add tests in future CLs? https://codereview.webrtc.org/1985663002/diff/20001/webrtc/examples/androidju... webrtc/examples/androidjunit/README:8: ./out/Debug/bin/AppRTCDemoJUnitTest name.of.the.class.to.test I think it would be better if we align with Chromium's Robolectric JUnit tests and use the wrappers scripts that gets written into out/{Debug,Release}/bin if configured correctly in the GYP file (see comments in webrtc_examples.gypi). I believe it might be needed in order to run this nicely on the bots anyway, and those scripts seems to add a lot of extra functionality that may be useful. https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gyp File webrtc/webrtc_examples.gyp (right): https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gy... webrtc/webrtc_examples.gyp:469: 'AppRTCDemo_apk', I think AppRTCDemo is the correct target to depend on here. The _apk target is just adding packaging of the APK. What you're interested in here is just to have the classes that you're about to write tests for compiled, right? Changing this will speed up a clean build of AppRTCDemoJUnitTest a little. https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gy... webrtc/webrtc_examples.gyp:470: '<(DEPTH)/base/base.gyp:base', Is 'base' really needed? Some of the Chromium tests seems to depend on 'base_java', which is smaller. Some only depends less targets as well. https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gy... webrtc/webrtc_examples.gyp:475: 'main_class': 'org.junit.runner.JUnitCore', What are the main differences between using this and the class Chromium uses (org.chromium.testing.local.JunitTestMain) ? I think we might want to use Chromium's helper classes since we're already compiling it and all. https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gy... webrtc/webrtc_examples.gyp:479: 'test_type': 'junit', To use all of Chromium's test framework for Android, I think you need to add the 'wrapper_script_name' variable here. Code searching in Chromium shows a lot of such examples: https://code.google.com/p/chromium/codesearch#search/&q=JunitTestMain%20f:gyp... https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gy... webrtc/webrtc_examples.gyp:482: '../build/host_jar.gypi', The tests in Chromium uses includes build/android/test_runner.gypi as well here. I think that's needed to generate the test wrapper scripts in out/{Debug,Release}/bin, which may be required to run these tests easily on the bots.
I changed this to use the test_runner system. Only thing I wasn't able to change was dependency on AppRTCDemo_apk. https://codereview.webrtc.org/1985663002/diff/20001/webrtc/examples/androidju... File webrtc/examples/androidjunit/README (right): https://codereview.webrtc.org/1985663002/diff/20001/webrtc/examples/androidju... webrtc/examples/androidjunit/README:1: This directory contains example JUnit tests for Android AppRTCDemo. On 2016/05/16 14:18:09, kjellander (webrtc) wrote: > This is currently not yet true, but I assume you plan to add tests in future > CLs? Yes, that is correct. https://codereview.webrtc.org/1985663002/diff/20001/webrtc/examples/androidju... webrtc/examples/androidjunit/README:8: ./out/Debug/bin/AppRTCDemoJUnitTest name.of.the.class.to.test On 2016/05/16 14:18:09, kjellander (webrtc) wrote: > I think it would be better if we align with Chromium's Robolectric JUnit tests > and use the wrappers scripts that gets written into out/{Debug,Release}/bin if > configured correctly in the GYP file (see comments in webrtc_examples.gypi). > > I believe it might be needed in order to run this nicely on the bots anyway, and > those scripts seems to add a lot of extra functionality that may be useful. Done. https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gyp File webrtc/webrtc_examples.gyp (right): https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gy... webrtc/webrtc_examples.gyp:469: 'AppRTCDemo_apk', On 2016/05/16 14:18:10, kjellander (webrtc) wrote: > I think AppRTCDemo is the correct target to depend on here. The _apk target is > just adding packaging of the APK. What you're interested in here is just to have > the classes that you're about to write tests for compiled, right? Changing this > will speed up a clean build of AppRTCDemoJUnitTest a little. For some reason if I change this, tests won't compile. They don't seem to be able to find the classes. examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java:3: error: cannot find symbol https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gy... webrtc/webrtc_examples.gyp:470: '<(DEPTH)/base/base.gyp:base', On 2016/05/16 14:18:10, kjellander (webrtc) wrote: > Is 'base' really needed? Some of the Chromium tests seems to depend on > 'base_java', which is smaller. Some only depends less targets as well. Right, I'll change this. https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gy... webrtc/webrtc_examples.gyp:475: 'main_class': 'org.junit.runner.JUnitCore', On 2016/05/16 14:18:10, kjellander (webrtc) wrote: > What are the main differences between using this and the class Chromium uses > (org.chromium.testing.local.JunitTestMain) ? I think we might want to use > Chromium's helper classes since we're already compiling it and all. I believe "org.chromium.testing.local.JunitTestMain" uses versions of jar files that are actually used in the project while "org.junit.runner.JUnitCore" would download them from the internet repository. There might be some other differences as well. I changed this. https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gy... webrtc/webrtc_examples.gyp:479: 'test_type': 'junit', On 2016/05/16 14:18:10, kjellander (webrtc) wrote: > To use all of Chromium's test framework for Android, I think you need to add the > 'wrapper_script_name' variable here. > > Code searching in Chromium shows a lot of such examples: > https://code.google.com/p/chromium/codesearch#search/&q=JunitTestMain%20f:gyp... Done. https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gy... webrtc/webrtc_examples.gyp:482: '../build/host_jar.gypi', On 2016/05/16 14:18:10, kjellander (webrtc) wrote: > The tests in Chromium uses includes build/android/test_runner.gypi as well here. > I think that's needed to generate the test wrapper scripts in > out/{Debug,Release}/bin, which may be required to run these tests easily on the > bots. Done.
Patchset #3 (id:40001) has been deleted
Thanks for adopting this to the way the bots run the tests (using the wrapper scripts). It'll be slightly easier to remember too for developers: please update the README. Can you also please file a tracking bug at bugs.webrtc.org to explain a bit about the effort you're doing here, so you can reference that by adding BUG=webrtc:<bug-number> to this CL description. https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gyp File webrtc/webrtc_examples.gyp (right): https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gy... webrtc/webrtc_examples.gyp:469: 'AppRTCDemo_apk', On 2016/05/17 08:01:25, sakal wrote: > On 2016/05/16 14:18:10, kjellander (webrtc) wrote: > > I think AppRTCDemo is the correct target to depend on here. The _apk target is > > just adding packaging of the APK. What you're interested in here is just to > have > > the classes that you're about to write tests for compiled, right? Changing > this > > will speed up a clean build of AppRTCDemoJUnitTest a little. > > For some reason if I change this, tests won't compile. They don't seem to be > able to find the classes. > > examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java:3: error: > cannot find symbol Hmm, OK. Let's not bother spending time on that then. https://codereview.webrtc.org/1985663002/diff/60001/webrtc/examples/androidju... File webrtc/examples/androidjunit/README (right): https://codereview.webrtc.org/1985663002/diff/60001/webrtc/examples/androidju... webrtc/examples/androidjunit/README:8: webrtc/build/android/test_runner.py junit -s AppRTCDemoJUnitTest -vvv I think it's even easier to tell users to use the wrapper script here: out/Debug/bin/run_AppRTCDemoJUnitTest Then it's the same as for the native tests as well (they have similar wrapper scripts in there). I plan to send out a PSA explaining more about this as soon I've managed to get the bots to start using them (there's one blocking issue).
Description was changed from ========== JUnit test framework for AppRTC Android demo. This allows creating tests for AppRTC Android demo that will be run on the host machine instead of a device. These tests can mock Android APIs through Robolectric. Because the tests are run on the host machine, they run much faster. BUG= ========== to ========== JUnit test framework for AppRTC Android demo. This allows creating tests for AppRTC Android demo that will be run on the host machine instead of a device. These tests can mock Android APIs through Robolectric. Because the tests are run on the host machine, they run much faster. BUG=webrtc:5896 ==========
On 2016/05/17 08:22:48, kjellander (webrtc) wrote: > Thanks for adopting this to the way the bots run the tests (using the wrapper > scripts). It'll be slightly easier to remember too for developers: please update > the README. > > Can you also please file a tracking bug at http://bugs.webrtc.org to explain a bit > about the effort you're doing here, so you can reference that by adding > BUG=webrtc:<bug-number> to this CL description. > > https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gyp > File webrtc/webrtc_examples.gyp (right): > > https://codereview.webrtc.org/1985663002/diff/20001/webrtc/webrtc_examples.gy... > webrtc/webrtc_examples.gyp:469: 'AppRTCDemo_apk', > On 2016/05/17 08:01:25, sakal wrote: > > On 2016/05/16 14:18:10, kjellander (webrtc) wrote: > > > I think AppRTCDemo is the correct target to depend on here. The _apk target > is > > > just adding packaging of the APK. What you're interested in here is just to > > have > > > the classes that you're about to write tests for compiled, right? Changing > > this > > > will speed up a clean build of AppRTCDemoJUnitTest a little. > > > > For some reason if I change this, tests won't compile. They don't seem to be > > able to find the classes. > > > > examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java:3: > error: > > cannot find symbol > > Hmm, OK. Let's not bother spending time on that then. > > https://codereview.webrtc.org/1985663002/diff/60001/webrtc/examples/androidju... > File webrtc/examples/androidjunit/README (right): > > https://codereview.webrtc.org/1985663002/diff/60001/webrtc/examples/androidju... > webrtc/examples/androidjunit/README:8: webrtc/build/android/test_runner.py junit > -s AppRTCDemoJUnitTest -vvv > I think it's even easier to tell users to use the wrapper script here: > out/Debug/bin/run_AppRTCDemoJUnitTest > > Then it's the same as for the native tests as well (they have similar wrapper > scripts in there). I plan to send out a PSA explaining more about this as soon > I've managed to get the bots to start using them (there's one blocking issue). Done.
Description was changed from ========== JUnit test framework for AppRTC Android demo. This allows creating tests for AppRTC Android demo that will be run on the host machine instead of a device. These tests can mock Android APIs through Robolectric. Because the tests are run on the host machine, they run much faster. BUG=webrtc:5896 ========== to ========== JUnit test framework for AppRTC Android demo. This allows creating tests for AppRTC Android demo that will be run on the host machine instead of a device. These tests can mock Android APIs through Robolectric. Because the tests are run on the host machine, they run much faster. BUG=webrtc:5896 NOTRY=True ==========
This is great! I'm happy to see Robolectric tests being added to our project. I added NOTRY=True to save some of our CQ resources. Please commit this if all the trybots I triggered are green.
Forgot the magic word: lgtm
The CQ bit was checked by sakal@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985663002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985663002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5589)
lgtm. Thanks for doing this Sami!
The CQ bit was checked by sakal@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985663002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985663002/80001
The CQ bit was unchecked by sakal@webrtc.org
The CQ bit was checked by sakal@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985663002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985663002/80001
Message was sent while issue was closed.
Description was changed from ========== JUnit test framework for AppRTC Android demo. This allows creating tests for AppRTC Android demo that will be run on the host machine instead of a device. These tests can mock Android APIs through Robolectric. Because the tests are run on the host machine, they run much faster. BUG=webrtc:5896 NOTRY=True ========== to ========== JUnit test framework for AppRTC Android demo. This allows creating tests for AppRTC Android demo that will be run on the host machine instead of a device. These tests can mock Android APIs through Robolectric. Because the tests are run on the host machine, they run much faster. BUG=webrtc:5896 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== JUnit test framework for AppRTC Android demo. This allows creating tests for AppRTC Android demo that will be run on the host machine instead of a device. These tests can mock Android APIs through Robolectric. Because the tests are run on the host machine, they run much faster. BUG=webrtc:5896 NOTRY=True ========== to ========== JUnit test framework for AppRTC Android demo. This allows creating tests for AppRTC Android demo that will be run on the host machine instead of a device. These tests can mock Android APIs through Robolectric. Because the tests are run on the host machine, they run much faster. BUG=webrtc:5896 NOTRY=True Committed: https://crrev.com/ee3732622c0df433f62048d6f8e89e70a4561817 Cr-Commit-Position: refs/heads/master@{#12769} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ee3732622c0df433f62048d6f8e89e70a4561817 Cr-Commit-Position: refs/heads/master@{#12769} |
