|
|
Created:
4 years, 7 months ago by please use gerrit instead Modified:
4 years, 7 months ago Reviewers:
Marijn Kruisselbrink CC:
blink-reviews, chromium-reviews, haraken Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPaymentResponse tests
- Verify that PaymentResponse.details contains the parsed stringified
JSON object with instrument details.
- Verify that PaymentResponse.complete(boolean) will forward the correct
boolean boolean value to the implementation of the PaymentCompleter.
BUG=587995
Committed: https://crrev.com/5f33419202c68e3272053961ef89562e80352f5e
Cr-Commit-Position: refs/heads/master@{#391047}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address comments #Patch Set 3 : Remove dependency on an other patch #Messages
Total messages: 16 (7 generated)
rouslan@chromium.org changed reviewers: + mek@chromium.org
mek@, ptal. I had a TODO to verify more details in PaymentResponseTest.cpp and finally got around it recently.
looks mostly good. Please add a more descriptive CL description/commit message though. https://codereview.chromium.org/1941633002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp (left): https://codereview.chromium.org/1941633002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:18: namespace { Any particular reason you got rid of the anonymous namespace? I would at least keep MockPaymentCompleter in an anonymous namespace (for the tests themselves there isn't much reason to keep the anonymous namespace, since those names should be unique anyway, but I'd keep helpers such as MockPaymentCompleter properly isolated). https://codereview.chromium.org/1941633002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp (right): https://codereview.chromium.org/1941633002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:67: ASSERT_FALSE(getExceptionState().hadException()); This assert seems a bit unnecessary? At this point nothing has accessed m_exceptionState yet, so all this is testing is the constructor of NonThrowableExceptionState?
Description was changed from ========== PaymentResponse tests BUG=587995 ========== to ========== PaymentResponse tests - Verify that PaymentResponse.details contains the parsed stringified JSON object with instrument details. - Verify that PaymentResponse.complete(boolean) will forward the correct boolean boolean value to the implementation of the PaymentCompleter. BUG=587995 ==========
https://codereview.chromium.org/1941633002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp (left): https://codereview.chromium.org/1941633002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:18: namespace { On 2016/05/02 18:04:58, Marijn Kruisselbrink wrote: > Any particular reason you got rid of the anonymous namespace? > > I would at least keep MockPaymentCompleter in an anonymous namespace (for the > tests themselves there isn't much reason to keep the anonymous namespace, since > those names should be unique anyway, but I'd keep helpers such as > MockPaymentCompleter properly isolated). `git cl format` kept trying to indent anonymous namespace code +4. Since you feel that the anonymous namespace provides positive value in tests, I am bringing it back. https://codereview.chromium.org/1941633002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp (right): https://codereview.chromium.org/1941633002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:67: ASSERT_FALSE(getExceptionState().hadException()); On 2016/05/02 18:04:58, Marijn Kruisselbrink wrote: > This assert seems a bit unnecessary? At this point nothing has accessed > m_exceptionState yet, so all this is testing is the constructor of > NonThrowableExceptionState? Removed. Not sure why it was here.
ptal patch 2.
lgtm https://codereview.chromium.org/1941633002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp (left): https://codereview.chromium.org/1941633002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:18: namespace { On 2016/05/02 at 18:17:24, Rouslan wrote: > On 2016/05/02 18:04:58, Marijn Kruisselbrink wrote: > > Any particular reason you got rid of the anonymous namespace? > > > > I would at least keep MockPaymentCompleter in an anonymous namespace (for the > > tests themselves there isn't much reason to keep the anonymous namespace, since > > those names should be unique anyway, but I'd keep helpers such as > > MockPaymentCompleter properly isolated). > > `git cl format` kept trying to indent anonymous namespace code +4. Since you feel that the anonymous namespace provides positive value in tests, I am bringing it back. Ah, yeah, git cl format on blink code never seems to work quite well... Hopefully it won't be long until the more unified coding style. (and I don't really care if the actual tests are in an anonymous namespace, but helpers suck as MockPaymentCompleter, that could conceivably exist in multiple test files definitely should be in an anonymous namespace to prevent hard to detect bugs).
The CQ bit was checked by rouslan@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1938853002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mek@chromium.org Link to the patchset: https://codereview.chromium.org/1941633002/#ps40001 (title: "Remove dependency on an other patch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1941633002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1941633002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== PaymentResponse tests - Verify that PaymentResponse.details contains the parsed stringified JSON object with instrument details. - Verify that PaymentResponse.complete(boolean) will forward the correct boolean boolean value to the implementation of the PaymentCompleter. BUG=587995 ========== to ========== PaymentResponse tests - Verify that PaymentResponse.details contains the parsed stringified JSON object with instrument details. - Verify that PaymentResponse.complete(boolean) will forward the correct boolean boolean value to the implementation of the PaymentCompleter. BUG=587995 Committed: https://crrev.com/5f33419202c68e3272053961ef89562e80352f5e Cr-Commit-Position: refs/heads/master@{#391047} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5f33419202c68e3272053961ef89562e80352f5e Cr-Commit-Position: refs/heads/master@{#391047} |