Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1989813002:
Convert LooperExecutorTest in AppRTC Demo into JUnit test (Closed)
|
Created:
4 years, 7 months ago by sakal Modified:
4 years, 7 months ago Reviewers:
magjed_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionConvert LooperExecutorTest in AppRTC Demo into JUnit test
LooperExecutorTest now uses Robolectric instead of being instrumentation
test. This allows the test to be run faster and easier.
Committed: https://crrev.com/f8312cb45aa9cf1bfb8f9053e64ccd7c778eb3f0
Cr-Commit-Position: refs/heads/master@{#12825}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Changes according to magjed's comments #
Total comments: 5
Patch Set 3 : Add dot at the end of a sentence. #
Messages
Total messages: 20 (7 generated)
Description was changed from ========== Convert LooperExecutorTest in AppRTC Demo into JUnit test LooperExecutorTest now uses Robolectric instead of being instrumentation test. This allows the test to be run faster and easier. ========== to ========== Convert LooperExecutorTest in AppRTC Demo into JUnit test LooperExecutorTest now uses Robolectric instead of being instrumentation test. This allows the test to be run faster and easier. ==========
sakal@webrtc.org changed reviewers: + magjed@webrtc.org
Magnus, please take a look. https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidapp/sr... File webrtc/examples/androidapp/src/org/appspot/apprtc/util/LooperExecutor.java (right): https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidapp/sr... webrtc/examples/androidapp/src/org/appspot/apprtc/util/LooperExecutor.java:74: handler.getLooper().quit(); This change was required because when using Robolectric, runnables are run on the main thread. So Looper.myLooper() would return the main looper, which cannot be quit. This should work identically to the existing line on native Android and also works under Robolectric.
It's not optimal that you need to expose the internal Handler in order to test
it, and also that you need to shadow the Looper.
What's the purpose of LooperExecutor really? Why can't we use some existing
Executor like
ExecutorService executor =
Executors.newSingleThreadExecutor();
or
ScheduledExecutorService executor =
Executors.newSingleThreadScheduledExecutor();
?
In a separate CL, can you try replacing LooperExecutor with the interface
ExecutorService in TCPChannelClient, and replace checkIfCalledOnValidThread()
with a member function 'ThreadUtils.ThreadChecker threadChecker' and then use
threadChecker.checkIsOnValidThread() instead. If that works out, maybe we can
remove LooperExecutor completely.
On 2016/05/19 08:13:42, magjed_webrtc wrote: > It's not optimal that you need to expose the internal Handler in order to test > it, and also that you need to shadow the Looper. > > What's the purpose of LooperExecutor really? Why can't we use some existing > Executor like > ExecutorService executor = > Executors.newSingleThreadExecutor(); > or > ScheduledExecutorService executor = > Executors.newSingleThreadScheduledExecutor(); > ? > In a separate CL, can you try replacing LooperExecutor with the interface > ExecutorService in TCPChannelClient, and replace checkIfCalledOnValidThread() > with a member function 'ThreadUtils.ThreadChecker threadChecker' and then use > threadChecker.checkIsOnValidThread() instead. If that works out, maybe we can > remove LooperExecutor completely. As noted by Issue 1992213002, LooperExecutor coudln't be completely removed. Therefore, I still propose this change. Handler exposure is unfortunate but required in order to gain access to the thread's Looper object. It should be noted that the handler is only exposed package-wide. ShadowLooper is not my own class but instead a class by Robolectric designed to replace normal Looper under testing environment.
https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidapp/sr... File webrtc/examples/androidapp/src/org/appspot/apprtc/util/LooperExecutor.java (right): https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidapp/sr... webrtc/examples/androidapp/src/org/appspot/apprtc/util/LooperExecutor.java:134: * Access to handler for testing purposes nit: Add dot at end of sentence. https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidjunit/... File webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java (right): https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidjunit/... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:59: shadowLooper.idle(); What happens when shadowLooper.idle() is called? Are the pending Runnables executed at that point? It looks like calling idle() is the only operation that happens on shadowLooper. Can you make that a function instead and comment about the behaviour. https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidjunit/... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:64: public void testExecuteBeforeStart() { Comment that calls to execute() before requestStart() are ignored. https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidjunit/... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:72: verifyNoMoreInteractions(mockRunnable); Isn't enough with one verifyNoMoreInteractions(mockRunnable)? https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidjunit/... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:76: public void testExecuteAfterStop() { Comment that calls to execute() after requestStop() are ignored. https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidjunit/... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:89: public void testMultipleStarts() { Comment that extra requestStart() calls are ok and just ignored.
https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidapp/sr... File webrtc/examples/androidapp/src/org/appspot/apprtc/util/LooperExecutor.java (right): https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidapp/sr... webrtc/examples/androidapp/src/org/appspot/apprtc/util/LooperExecutor.java:134: * Access to handler for testing purposes On 2016/05/19 12:17:12, magjed_webrtc wrote: > nit: Add dot at end of sentence. Done. https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidjunit/... File webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java (right): https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidjunit/... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:59: shadowLooper.idle(); On 2016/05/19 12:17:13, magjed_webrtc wrote: > What happens when shadowLooper.idle() is called? Are the pending Runnables > executed at that point? It looks like calling idle() is the only operation that > happens on shadowLooper. Can you make that a function instead and comment about > the behaviour. That is correct. Done. https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidjunit/... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:64: public void testExecuteBeforeStart() { On 2016/05/19 12:17:13, magjed_webrtc wrote: > Comment that calls to execute() before requestStart() are ignored. Done. https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidjunit/... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:72: verifyNoMoreInteractions(mockRunnable); On 2016/05/19 12:17:13, magjed_webrtc wrote: > Isn't enough with one verifyNoMoreInteractions(mockRunnable)? Done. https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidjunit/... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:76: public void testExecuteAfterStop() { On 2016/05/19 12:17:13, magjed_webrtc wrote: > Comment that calls to execute() after requestStop() are ignored. Done. https://codereview.webrtc.org/1989813002/diff/1/webrtc/examples/androidjunit/... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:89: public void testMultipleStarts() { On 2016/05/19 12:17:13, magjed_webrtc wrote: > Comment that extra requestStart() calls are ok and just ignored. Done.
https://codereview.webrtc.org/1989813002/diff/20001/webrtc/examples/androidju... File webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java (right): https://codereview.webrtc.org/1989813002/diff/20001/webrtc/examples/androidju... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:71: verifyNoMoreInteractions(mockRunnable); Can you move this to the top of the function? https://codereview.webrtc.org/1989813002/diff/20001/webrtc/examples/androidju... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:85: verifyNoMoreInteractions(mockRunnable); Can you move this to the top of the function?
lgtm https://codereview.webrtc.org/1989813002/diff/20001/webrtc/examples/androidju... File webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java (right): https://codereview.webrtc.org/1989813002/diff/20001/webrtc/examples/androidju... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:71: verifyNoMoreInteractions(mockRunnable); On 2016/05/20 07:50:52, magjed_webrtc wrote: > Can you move this to the top of the function? Discussed offline. I misunderstood how verifyNoMoreInteractions works. It should it be at the end of the function.
https://codereview.webrtc.org/1989813002/diff/20001/webrtc/examples/androidju... File webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java (right): https://codereview.webrtc.org/1989813002/diff/20001/webrtc/examples/androidju... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:71: verifyNoMoreInteractions(mockRunnable); On 2016/05/20 07:50:52, magjed_webrtc wrote: > Can you move this to the top of the function? Function checks that there haven't been any unexpected interactions before it was called. So, it should be at the bottom of the function. https://codereview.webrtc.org/1989813002/diff/20001/webrtc/examples/androidju... webrtc/examples/androidjunit/src/org/appspot/apprtc/util/LooperExecutorTest.java:85: verifyNoMoreInteractions(mockRunnable); On 2016/05/20 07:50:52, magjed_webrtc wrote: > Can you move this to the top of the function? ditto.
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/1989813002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1989813002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
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/1989813002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1989813002/40001
Message was sent while issue was closed.
Description was changed from ========== Convert LooperExecutorTest in AppRTC Demo into JUnit test LooperExecutorTest now uses Robolectric instead of being instrumentation test. This allows the test to be run faster and easier. ========== to ========== Convert LooperExecutorTest in AppRTC Demo into JUnit test LooperExecutorTest now uses Robolectric instead of being instrumentation test. This allows the test to be run faster and easier. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Convert LooperExecutorTest in AppRTC Demo into JUnit test LooperExecutorTest now uses Robolectric instead of being instrumentation test. This allows the test to be run faster and easier. ========== to ========== Convert LooperExecutorTest in AppRTC Demo into JUnit test LooperExecutorTest now uses Robolectric instead of being instrumentation test. This allows the test to be run faster and easier. Committed: https://crrev.com/f8312cb45aa9cf1bfb8f9053e64ccd7c778eb3f0 Cr-Commit-Position: refs/heads/master@{#12825} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f8312cb45aa9cf1bfb8f9053e64ccd7c778eb3f0 Cr-Commit-Position: refs/heads/master@{#12825} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
