Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1361213002:
Adding 20-second timeout to Java and Objective-C tests. (Closed)
|
Created:
5 years, 3 months ago by Taylor Brandstetter Modified:
5 years, 2 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), Andrew MacDonald Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding 20-second timeout to Java and Objective-C tests.
This is the same sort of thing we do in C++ end-to-end PeerConnection
tests.
Committed: https://crrev.com/4fa648be681ee20d43f79f6c9ec9570631ebcf5a
Cr-Commit-Position: refs/heads/master@{#10098}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Use NSTimeInterval instead of int, and constant instead of define. #
Total comments: 4
Patch Set 3 : Minor changes to objc waitForExpectations #Patch Set 4 : Removing redundant stack trace for timeout. #
Messages
Total messages: 22 (7 generated)
deadbeef@webrtc.org changed reviewers: + jiayl@webrtc.org
tkchin@webrtc.org changed reviewers: + tkchin@webrtc.org
Also please run against all relevant trybots (iOS+OSX) https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... File talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.h (right): https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.h:58: - (BOOL)waitForAllExpectationsToBeSatisfiedWithTimeout:(int)timeoutSeconds; Don't use int type in ObjC unless you're interop-ing with C++ bits that need it. NSInteger/NSUInteger https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... File talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m (right): https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:146: NSDate *endTime = [NSDate dateWithTimeIntervalSinceNow:timeoutSeconds]; Since you are using NSDate API, use NSTimeInterval type for the timeout. https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:148: if ([endTime compare:[NSDate date]] != NSOrderedDescending) { I think it's easier/clearer to use a comparison with timeIntervalSinceNow rather than calling compare. https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:149: return false; return NO; https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:154: return true; return YES; https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... File talk/app/webrtc/objctests/RTCPeerConnectionTest.mm (right): https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionTest.mm:49: #define TIMEOUT_SECONDS 20 Avoid defines, prefer constants. const NSTimeInterval kRTCPeerConnectionTestTimeout = 20; https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionTest.mm:240: EXPECT_TRUE([offeringExpectations Wouldn't this be a critical failure? Do we want ASSERT_TRUE? Since likely if this fails everything else will too. No point waiting.
deadbeef@webrtc.org changed reviewers: + glaznev@webrtc.org
Thanks Zeke; as you can tell, I don't write a lot of Objective-C code. :) Alex, could you review the Java side? https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... File talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.h (right): https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.h:58: - (BOOL)waitForAllExpectationsToBeSatisfiedWithTimeout:(int)timeoutSeconds; On 2015/09/25 15:40:32, tkchin_webrtc wrote: > Don't use int type in ObjC unless you're interop-ing with C++ bits that need it. > NSInteger/NSUInteger Done. https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... File talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m (right): https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:146: NSDate *endTime = [NSDate dateWithTimeIntervalSinceNow:timeoutSeconds]; On 2015/09/25 15:40:32, tkchin_webrtc wrote: > Since you are using NSDate API, use NSTimeInterval type for the timeout. Done. https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:148: if ([endTime compare:[NSDate date]] != NSOrderedDescending) { On 2015/09/25 15:40:32, tkchin_webrtc wrote: > I think it's easier/clearer to use a comparison with timeIntervalSinceNow rather > than calling compare. Done. https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:149: return false; On 2015/09/25 15:40:32, tkchin_webrtc wrote: > return NO; Done. https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:154: return true; On 2015/09/25 15:40:32, tkchin_webrtc wrote: > return YES; Done. https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... File talk/app/webrtc/objctests/RTCPeerConnectionTest.mm (right): https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionTest.mm:49: #define TIMEOUT_SECONDS 20 On 2015/09/25 15:40:32, tkchin_webrtc wrote: > Avoid defines, prefer constants. > const NSTimeInterval kRTCPeerConnectionTestTimeout = 20; Ah. I wasn't sure how to avoid the constant having external linkage (since I can't do the unnamed namespace trick in Objective-C). But having a long name is a sensible workaround. https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionTest.mm:240: EXPECT_TRUE([offeringExpectations On 2015/09/25 15:40:32, tkchin_webrtc wrote: > Wouldn't this be a critical failure? Do we want ASSERT_TRUE? Since likely if > this fails everything else will too. No point waiting. Well, not necessarily. The last time this broke for me, it was because it was expecting 2 ICE candidates and got 1, but everything else worked fine. Also, the equivalent C++ end-to-end tests don't use ASSERT in this kind of situation.
https://codereview.webrtc.org/1361213002/diff/20001/talk/app/webrtc/java/test... File talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java (right): https://codereview.webrtc.org/1361213002/diff/20001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:374: + (new Throwable()).getStackTrace()[1] + "\n for: " Why "[1]" here? May be use Throwable.printStackTrace() instead or print all elements of the array.
https://codereview.webrtc.org/1361213002/diff/20001/talk/app/webrtc/java/test... File talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java (right): https://codereview.webrtc.org/1361213002/diff/20001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:374: + (new Throwable()).getStackTrace()[1] + "\n for: " On 2015/09/25 18:49:25, AlexG wrote: > Why "[1]" here? May be use Throwable.printStackTrace() instead or print all > elements of the array. I assume because waitForAllExpectationsToBeSatisfied will always be called from doTest, and we really just want to know which line of doTest we're on. The rest of the stack should just be unit test infrastructure.
lgtm https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... File talk/app/webrtc/objctests/RTCPeerConnectionTest.mm (right): https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionTest.mm:49: #define TIMEOUT_SECONDS 20 On 2015/09/25 18:01:57, Taylor Brandstetter wrote: > On 2015/09/25 15:40:32, tkchin_webrtc wrote: > > Avoid defines, prefer constants. > > const NSTimeInterval kRTCPeerConnectionTestTimeout = 20; > > Ah. I wasn't sure how to avoid the constant having external linkage (since I > can't do the unnamed namespace trick in Objective-C). But having a long name is > a sensible workaround. Yeh, this is a standard thing in ObjC - just add the class as a prefix to static constants. https://codereview.webrtc.org/1361213002/diff/1/talk/app/webrtc/objctests/RTC... talk/app/webrtc/objctests/RTCPeerConnectionTest.mm:240: EXPECT_TRUE([offeringExpectations On 2015/09/25 18:01:57, Taylor Brandstetter wrote: > On 2015/09/25 15:40:32, tkchin_webrtc wrote: > > Wouldn't this be a critical failure? Do we want ASSERT_TRUE? Since likely if > > this fails everything else will too. No point waiting. > > Well, not necessarily. The last time this broke for me, it was because it was > expecting 2 ICE candidates and got 1, but everything else worked fine. Also, the > equivalent C++ end-to-end tests don't use ASSERT in this kind of situation. Gotcha. https://codereview.webrtc.org/1361213002/diff/20001/talk/app/webrtc/objctests... File talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m (right): https://codereview.webrtc.org/1361213002/diff/20001/talk/app/webrtc/objctests... talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:142: - (BOOL)waitForAllExpectationsToBeSatisfiedWithTimeout:(NSTimeInterval)timeout { might want to NSParameterAssert(timeout >= 0); or timeout > -DBL_EPSILON https://codereview.webrtc.org/1361213002/diff/20001/talk/app/webrtc/objctests... talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:146: NSDate *endTime = [NSDate dateWithTimeIntervalSinceNow:timeout]; nit: dot syntax for properties. Also, consider using startTime (but up to you). NSDate *startTime = [NSDate date]; ... if (startTime.timeIntervalSinceNow < -timeout) { return NO; } ...
On 2015/09/25 19:44:03, Taylor Brandstetter wrote: > https://codereview.webrtc.org/1361213002/diff/20001/talk/app/webrtc/java/test... > File talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java > (right): > > https://codereview.webrtc.org/1361213002/diff/20001/talk/app/webrtc/java/test... > talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:374: + > (new Throwable()).getStackTrace()[1] + "\n for: " > On 2015/09/25 18:49:25, AlexG wrote: > > Why "[1]" here? May be use Throwable.printStackTrace() instead or print all > > elements of the array. > > I assume because waitForAllExpectationsToBeSatisfied will always be called from > doTest, and we really just want to know which line of doTest we're on. The rest > of the stack should just be unit test infrastructure. But why not to make general solution which does not make any assumption on call sequence? Just use printStackTrace - it will not hurt to have a full stack trace of failed test.
On 2015/09/25 19:51:51, AlexG wrote: > On 2015/09/25 19:44:03, Taylor Brandstetter wrote: > > > https://codereview.webrtc.org/1361213002/diff/20001/talk/app/webrtc/java/test... > > File talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java > > (right): > > > > > https://codereview.webrtc.org/1361213002/diff/20001/talk/app/webrtc/java/test... > > talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:374: + > > (new Throwable()).getStackTrace()[1] + "\n for: " > > On 2015/09/25 18:49:25, AlexG wrote: > > > Why "[1]" here? May be use Throwable.printStackTrace() instead or print all > > > elements of the array. > > > > I assume because waitForAllExpectationsToBeSatisfied will always be called > from > > doTest, and we really just want to know which line of doTest we're on. The > rest > > of the stack should just be unit test infrastructure. > > But why not to make general solution which does not make any assumption on call > sequence? > Just use printStackTrace - it will not hurt to have a full stack trace of failed > test. Actually, I just realized that assertTrue already prints a full stack trace. So I just removed the "timed out" stack trace since it's redundant.
lgtm
lgtm
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1361213002/#ps60001 (title: "Removing redundant stack trace for timeout.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1361213002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1361213002/60001
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 deadbeef@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1361213002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1361213002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4fa648be681ee20d43f79f6c9ec9570631ebcf5a Cr-Commit-Position: refs/heads/master@{#10098} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
