Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2506653003:
Instrument CronetUrlRequestTest#testThrowOrCancelInOnCanceled flaky test (Closed)
|
Created:
4 years, 1 month ago by kapishnikov Modified:
4 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Charles Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInstrument CronetUrlRequestTest#testThrowOrCancelInOnCanceled flaky test
Also, add toString method to UrlRequestException.
BUG=664872
Committed: https://crrev.com/7acfe39937220b608b8dc8314c14f0e6604c8021
Cr-Commit-Position: refs/heads/master@{#432884}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed comments #Patch Set 3 : Changed equals to contains when comparing error messages. #
Total comments: 2
Patch Set 4 : Spelling fix #Messages
Total messages: 22 (5 generated)
kapishnikov@chromium.org changed reviewers: + mef@chromium.org
PTAL.
clm@google.com changed reviewers: + clm@google.com
https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/UrlRequestException.java (right): https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:166: public String toString() { Usually the preferred way to do this would be to add this information to the "message" argument of the superclass's constructor. It's not really idiomatic to override toString on an exception. https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:2016: String message = "Unexpected response state " + callback.mResponseStep; A more idiomatic way to show this would be to say if (state) { throw new AssertionError(callback.mError) } So the callback's error is the cause of the assertion that fails the test. This way you get message, and stack trace info.
Charles, thanks for the review. PTAL. https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/UrlRequestException.java (right): https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:166: public String toString() { On 2016/11/15 19:08:16, Charles wrote: > Usually the preferred way to do this would be to add this information to the > "message" argument of the superclass's constructor. It's not really idiomatic to > override toString on an exception. I cannot pass the augmented message to the supertype constructor but I can override the getMessage() method. See the change. https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:2016: String message = "Unexpected response state " + callback.mResponseStep; On 2016/11/15 19:08:16, Charles wrote: > A more idiomatic way to show this would be to say > > if (state) { > throw new AssertionError(callback.mError) > } > > So the callback's error is the cause of the assertion that fails the test. This > way you get message, and stack trace info. Done. The AssertionError with cause was only introduced in JDK 19. Our min SDK is set to 16. So, I used the plain Error instead.
https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/UrlRequestException.java (right): https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:166: public String toString() { On 2016/11/15 21:17:01, kapishnikov wrote: > On 2016/11/15 19:08:16, Charles wrote: > > Usually the preferred way to do this would be to add this information to the > > "message" argument of the superclass's constructor. It's not really idiomatic > to > > override toString on an exception. > > I cannot pass the augmented message to the supertype constructor but I can > override the getMessage() method. See the change. Why can't you pass it to the supertype constructor? https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:2016: String message = "Unexpected response state " + callback.mResponseStep; On 2016/11/15 21:17:01, kapishnikov wrote: > On 2016/11/15 19:08:16, Charles wrote: > > A more idiomatic way to show this would be to say > > > > if (state) { > > throw new AssertionError(callback.mError) > > } > > > > So the callback's error is the cause of the assertion that fails the test. > This > > way you get message, and stack trace info. > > Done. The AssertionError with cause was only introduced in JDK 19. Our min SDK > is set to 16. So, I used the plain Error instead. I think you may need to upload a new patchset
On 2016/11/15 21:38:22, Charles wrote: > https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/a... > File components/cronet/android/api/src/org/chromium/net/UrlRequestException.java > (right): > > https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/a... > components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:166: > public String toString() { > On 2016/11/15 21:17:01, kapishnikov wrote: > > On 2016/11/15 19:08:16, Charles wrote: > > > Usually the preferred way to do this would be to add this information to the > > > "message" argument of the superclass's constructor. It's not really > idiomatic > > to > > > override toString on an exception. > > > > I cannot pass the augmented message to the supertype constructor but I can > > override the getMessage() method. See the change. > > Why can't you pass it to the supertype constructor? > > https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... > File > components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java > (right): > > https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... > components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:2016: > String message = "Unexpected response state " + callback.mResponseStep; > On 2016/11/15 21:17:01, kapishnikov wrote: > > On 2016/11/15 19:08:16, Charles wrote: > > > A more idiomatic way to show this would be to say > > > > > > if (state) { > > > throw new AssertionError(callback.mError) > > > } > > > > > > So the callback's error is the cause of the assertion that fails the test. > > This > > > way you get message, and stack trace info. > > > > Done. The AssertionError with cause was only introduced in JDK 19. Our min SDK > > is set to 16. So, I used the plain Error instead. > > I think you may need to upload a new patchset There are only 2 patchsets currently. What patchset are you referring to?
https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:2016: String message = "Unexpected response state " + callback.mResponseStep; On 2016/11/15 21:38:22, Charles wrote: > On 2016/11/15 21:17:01, kapishnikov wrote: > > On 2016/11/15 19:08:16, Charles wrote: > > > A more idiomatic way to show this would be to say > > > > > > if (state) { > > > throw new AssertionError(callback.mError) > > > } > > > > > > So the callback's error is the cause of the assertion that fails the test. > > This > > > way you get message, and stack trace info. > > > > Done. The AssertionError with cause was only introduced in JDK 19. Our min SDK > > is set to 16. So, I used the plain Error instead. > > I think you may need to upload a new patchset Because the call to the supertype constructor should be the first statement in the constructor.
On 2016/11/15 21:49:05, kapishnikov wrote: > On 2016/11/15 21:38:22, Charles wrote: > > > https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/a... > > File > components/cronet/android/api/src/org/chromium/net/UrlRequestException.java > > (right): > > > > > https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/a... > > > components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:166: > > public String toString() { > > On 2016/11/15 21:17:01, kapishnikov wrote: > > > On 2016/11/15 19:08:16, Charles wrote: > > > > Usually the preferred way to do this would be to add this information to > the > > > > "message" argument of the superclass's constructor. It's not really > > idiomatic > > > to > > > > override toString on an exception. > > > > > > I cannot pass the augmented message to the supertype constructor but I can > > > override the getMessage() method. See the change. > > > > Why can't you pass it to the supertype constructor? > > > > > https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... > > File > > > components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java > > (right): > > > > > https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... > > > components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:2016: > > String message = "Unexpected response state " + callback.mResponseStep; > > On 2016/11/15 21:17:01, kapishnikov wrote: > > > On 2016/11/15 19:08:16, Charles wrote: > > > > A more idiomatic way to show this would be to say > > > > > > > > if (state) { > > > > throw new AssertionError(callback.mError) > > > > } > > > > > > > > So the callback's error is the cause of the assertion that fails the test. > > > This > > > > way you get message, and stack trace info. > > > > > > Done. The AssertionError with cause was only introduced in JDK 19. Our min > SDK > > > is set to 16. So, I used the plain Error instead. > > > > I think you may need to upload a new patchset > > There are only 2 patchsets currently. What patchset are you referring to? Got it!
https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java (right): https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:2016: String message = "Unexpected response state " + callback.mResponseStep; On 2016/11/15 21:49:17, kapishnikov wrote: > On 2016/11/15 21:38:22, Charles wrote: > > On 2016/11/15 21:17:01, kapishnikov wrote: > > > On 2016/11/15 19:08:16, Charles wrote: > > > > A more idiomatic way to show this would be to say > > > > > > > > if (state) { > > > > throw new AssertionError(callback.mError) > > > > } > > > > > > > > So the callback's error is the cause of the assertion that fails the test. > > > This > > > > way you get message, and stack trace info. > > > > > > Done. The AssertionError with cause was only introduced in JDK 19. Our min > SDK > > > is set to 16. So, I used the plain Error instead. > > > > I think you may need to upload a new patchset > > Because the call to the supertype constructor should be the first statement in > the constructor. That's true, but if you make this logic a private method you can do it - super(foomethod(param)); is valid java.
On 2016/11/15 22:29:33, Charles wrote: > https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... > File > components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java > (right): > > https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... > components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:2016: > String message = "Unexpected response state " + callback.mResponseStep; > On 2016/11/15 21:49:17, kapishnikov wrote: > > On 2016/11/15 21:38:22, Charles wrote: > > > On 2016/11/15 21:17:01, kapishnikov wrote: > > > > On 2016/11/15 19:08:16, Charles wrote: > > > > > A more idiomatic way to show this would be to say > > > > > > > > > > if (state) { > > > > > throw new AssertionError(callback.mError) > > > > > } > > > > > > > > > > So the callback's error is the cause of the assertion that fails the > test. > > > > This > > > > > way you get message, and stack trace info. > > > > > > > > Done. The AssertionError with cause was only introduced in JDK 19. Our min > > SDK > > > > is set to 16. So, I used the plain Error instead. > > > > > > I think you may need to upload a new patchset > > > > Because the call to the supertype constructor should be the first statement in > > the constructor. > > That's true, but if you make this logic a private method you can do it - > super(foomethod(param)); is valid java. I am getting: ../../components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:94: error: cannot reference this before supertype constructor has been called super(foomethod(), null); ^ in: public UrlRequestException(String message, int errorCode, int cronetInternalErrorCode) { super(foomethod(), null); mErrorCode = errorCode; mCronetInternalErrorCode = cronetInternalErrorCode; } private String foomethod() { return "foo"; }
On 2016/11/15 22:36:18, kapishnikov wrote: > On 2016/11/15 22:29:33, Charles wrote: > > > https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... > > File > > > components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java > > (right): > > > > > https://codereview.chromium.org/2506653003/diff/1/components/cronet/android/t... > > > components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:2016: > > String message = "Unexpected response state " + callback.mResponseStep; > > On 2016/11/15 21:49:17, kapishnikov wrote: > > > On 2016/11/15 21:38:22, Charles wrote: > > > > On 2016/11/15 21:17:01, kapishnikov wrote: > > > > > On 2016/11/15 19:08:16, Charles wrote: > > > > > > A more idiomatic way to show this would be to say > > > > > > > > > > > > if (state) { > > > > > > throw new AssertionError(callback.mError) > > > > > > } > > > > > > > > > > > > So the callback's error is the cause of the assertion that fails the > > test. > > > > > This > > > > > > way you get message, and stack trace info. > > > > > > > > > > Done. The AssertionError with cause was only introduced in JDK 19. Our > min > > > SDK > > > > > is set to 16. So, I used the plain Error instead. > > > > > > > > I think you may need to upload a new patchset > > > > > > Because the call to the supertype constructor should be the first statement > in > > > the constructor. > > > > That's true, but if you make this logic a private method you can do it - > > super(foomethod(param)); is valid java. > > I am getting: > > ../../components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:94: > error: cannot reference this before supertype constructor has been called > super(foomethod(), null); > ^ > in: > public UrlRequestException(String message, int errorCode, int > cronetInternalErrorCode) { > super(foomethod(), null); > mErrorCode = errorCode; > mCronetInternalErrorCode = cronetInternalErrorCode; > } > > private String foomethod() { > return "foo"; > } As Misha noted, it is possible to use a static method to avoid referencing 'this'. Is it a common pattern? I found many instances where exceptions override getMessage() method but couldn't find any where an augmented message is passed to the supertype. Also, there is getLocalizedMessage() that is supposed to be overridden by subclasses. It sounds inconsistent to override getLocalizedMessage() but not getMessage().
I have fixed the failing tests by changing how we compare the error messages with the expected values. The strict 'equals' comparison was changed to 'contains'. PTAL.
lgtm lgtm
lgtm https://codereview.chromium.org/2506653003/diff/40001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/UrlRequestException.java (right): https://codereview.chromium.org/2506653003/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:172: b.append(", Retriable=").append(immediatelyRetryable()); Retriable -> Retryable
https://codereview.chromium.org/2506653003/diff/40001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/UrlRequestException.java (right): https://codereview.chromium.org/2506653003/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/UrlRequestException.java:172: b.append(", Retriable=").append(immediatelyRetryable()); On 2016/11/16 22:35:49, mef wrote: > Retriable -> Retryable Done.
The CQ bit was checked by kapishnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org, clm@google.com Link to the patchset: https://codereview.chromium.org/2506653003/#ps60001 (title: "Spelling fix")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Instrument CronetUrlRequestTest#testThrowOrCancelInOnCanceled flaky test Also, add toString method to UrlRequestException. BUG=664872 ========== to ========== Instrument CronetUrlRequestTest#testThrowOrCancelInOnCanceled flaky test Also, add toString method to UrlRequestException. BUG=664872 Committed: https://crrev.com/7acfe39937220b608b8dc8314c14f0e6604c8021 Cr-Commit-Position: refs/heads/master@{#432884} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7acfe39937220b608b8dc8314c14f0e6604c8021 Cr-Commit-Position: refs/heads/master@{#432884} |
