Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2631133003:
[Payments] Introduce basic interactive browsertests for Payment Request (Closed)
Description[Payments] Introduce basic interactive browsertests for Payment Request
According to code documentation [1], the tests need to be in
"interactive_ui_tests" because they will need focus, and thus can't be
sharded like regular browsertests.
I also went with the approach of putting several test classes in the
same file for a couple of reasons: we may not have many tests at first
(thus little clutter), and it's easier to add a test when one can see
what other tests are doing nearby. Certainly later on as these things
are less true, we can split into multiple files as needed.
Also want to mention that the tests are simplistic now; a follow-up CL
will focus on View elements and click them.
[1]
https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.h?rcl=0&l=37
BUG=679734
TEST=./out/Default/interactive_ui_tests --gtest_filter=PaymentRequest*
Review-Url: https://codereview.chromium.org/2631133003
Cr-Commit-Position: refs/heads/master@{#444825}
Committed: https://chromium.googlesource.com/chromium/src/+/a397e24fc14ad76c5e7d462eb7b934da031629b3
Patch Set 1 : Initial #
Total comments: 6
Patch Set 2 : fix for mac compile #
Total comments: 6
Patch Set 3 : addressed comments #Patch Set 4 : Observer pattern #
Total comments: 4
Patch Set 5 : ObserverForTest #Messages
Total messages: 47 (31 generated)
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:40001) has been deleted
Description was changed from ========== [Payments] Introduce basic browsertests for Payment Request BUG=679734 TEST=PaymentRequest*BrowserTest ========== to ========== [Payments] Introduce basic interactive browsertests for Payment Request According to code documentation [1], the tests need to be in "interactive_ui_tests" because they will need focus, and thus can't be sharded like regular browsertests. I also went with the approach of putting several test classes in the same file for a couple of reasons: we may not have many at first (little clutter), and it's easier to add a test when others are nearby and not split in different files. Certainly later on we can split into multiple files as needed. Also want to mention that the tests are simplistic now; a follow-up CL will focus on View elements and click them. [1] https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... BUG=679734 TEST=./out/Default/interactive_ui_tests --gtest_filter=PaymentRequest* ==========
mathp@chromium.org changed reviewers: + rouslan@chromium.org, sky@chromium.org
Description was changed from ========== [Payments] Introduce basic interactive browsertests for Payment Request According to code documentation [1], the tests need to be in "interactive_ui_tests" because they will need focus, and thus can't be sharded like regular browsertests. I also went with the approach of putting several test classes in the same file for a couple of reasons: we may not have many at first (little clutter), and it's easier to add a test when others are nearby and not split in different files. Certainly later on we can split into multiple files as needed. Also want to mention that the tests are simplistic now; a follow-up CL will focus on View elements and click them. [1] https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... BUG=679734 TEST=./out/Default/interactive_ui_tests --gtest_filter=PaymentRequest* ========== to ========== [Payments] Introduce basic interactive browsertests for Payment Request According to code documentation [1], the tests need to be in "interactive_ui_tests" because they will need focus, and thus can't be sharded like regular browsertests. I also went with the approach of putting several test classes in the same file for a couple of reasons: we may not have many tests at first (thus little clutter), and it's easier to add a test when others are nearby and not split in different files. Certainly later on we can split into multiple files as needed. Also want to mention that the tests are simplistic now; a follow-up CL will focus on View elements and click them. [1] https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... BUG=679734 TEST=./out/Default/interactive_ui_tests --gtest_filter=PaymentRequest* ==========
Hi, Rouslan please have a look Scott for approval Thanks
Description was changed from ========== [Payments] Introduce basic interactive browsertests for Payment Request According to code documentation [1], the tests need to be in "interactive_ui_tests" because they will need focus, and thus can't be sharded like regular browsertests. I also went with the approach of putting several test classes in the same file for a couple of reasons: we may not have many tests at first (thus little clutter), and it's easier to add a test when others are nearby and not split in different files. Certainly later on we can split into multiple files as needed. Also want to mention that the tests are simplistic now; a follow-up CL will focus on View elements and click them. [1] https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... BUG=679734 TEST=./out/Default/interactive_ui_tests --gtest_filter=PaymentRequest* ========== to ========== [Payments] Introduce basic interactive browsertests for Payment Request According to code documentation [1], the tests need to be in "interactive_ui_tests" because they will need focus, and thus can't be sharded like regular browsertests. I also went with the approach of putting several test classes in the same file for a couple of reasons: we may not have many tests at first (thus little clutter), and it's easier to add a test when one can see what other tests are doing nearby. Certainly later on as these things are less true, we can split into multiple files as needed. Also want to mention that the tests are simplistic now; a follow-up CL will focus on View elements and click them. [1] https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... BUG=679734 TEST=./out/Default/interactive_ui_tests --gtest_filter=PaymentRequest* ==========
Description was changed from ========== [Payments] Introduce basic interactive browsertests for Payment Request According to code documentation [1], the tests need to be in "interactive_ui_tests" because they will need focus, and thus can't be sharded like regular browsertests. I also went with the approach of putting several test classes in the same file for a couple of reasons: we may not have many tests at first (thus little clutter), and it's easier to add a test when one can see what other tests are doing nearby. Certainly later on as these things are less true, we can split into multiple files as needed. Also want to mention that the tests are simplistic now; a follow-up CL will focus on View elements and click them. [1] https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... BUG=679734 TEST=./out/Default/interactive_ui_tests --gtest_filter=PaymentRequest* ========== to ========== [Payments] Introduce basic interactive browsertests for Payment Request According to code documentation [1], the tests need to be in "interactive_ui_tests" because they will need focus, and thus can't be sharded like regular browsertests. I also went with the approach of putting several test classes in the same file for a couple of reasons: we may not have many tests at first (thus little clutter), and it's easier to add a test when one can see what other tests are doing nearby. Certainly later on as these things are less true, we can split into multiple files as needed. Also want to mention that the tests are simplistic now; a follow-up CL will focus on View elements and click them. [1] https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... BUG=679734 TEST=./out/Default/interactive_ui_tests --gtest_filter=PaymentRequest* ==========
lgtm % nits https://codereview.chromium.org/2631133003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc (right): https://codereview.chromium.org/2631133003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc:19: }; Destructor? https://codereview.chromium.org/2631133003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc:32: "/payment_request_no_shipping_test.html") {} Destructor?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
Thanks https://codereview.chromium.org/2631133003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc (right): https://codereview.chromium.org/2631133003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc:19: }; On 2017/01/17 18:57:20, rouslan wrote: > Destructor? Is there a particular reason I should have one here? I would think the base class destructor is fine? https://codereview.chromium.org/2631133003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc:32: "/payment_request_no_shipping_test.html") {} On 2017/01/17 18:57:20, rouslan wrote: > Destructor? see above
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2631133003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc (right): https://codereview.chromium.org/2631133003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc:19: }; On 2017/01/17 19:23:06, Mathieu Perreault wrote: > On 2017/01/17 18:57:20, rouslan wrote: > > Destructor? > > Is there a particular reason I should have one here? I would think the base > class destructor is fine? If you don't specify a destructor with "override" keyword, then the parent destructor does not get called, I think.
Thanks https://codereview.chromium.org/2631133003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc (right): https://codereview.chromium.org/2631133003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_interactive_uitest.cc:19: }; On 2017/01/17 20:41:24, rouslan wrote: > On 2017/01/17 19:23:06, Mathieu Perreault wrote: > > On 2017/01/17 18:57:20, rouslan wrote: > > > Destructor? > > > > Is there a particular reason I should have one here? I would think the base > > class destructor is fine? > > If you don't specify a destructor with "override" keyword, then the parent > destructor does not get called, I think. I put a log statement inside ~PaymentRequestInteractiveTestBase() and it is called for the two tests in this file.
LGTM https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc (right): https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:35: new net::EmbeddedTestServer(net::EmbeddedTestServer::TYPE_HTTPS)); MakeUnique. https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:46: static const std::string kClickBuyButtonJs = I wouldn't bother with a static here. https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:70: for (const auto& p : manager->payment_requests_) { no {}
On 2017/01/17 23:05:52, sky wrote: > LGTM > > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc > (right): > > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:35: > new net::EmbeddedTestServer(net::EmbeddedTestServer::TYPE_HTTPS)); > MakeUnique. > > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:46: > static const std::string kClickBuyButtonJs = > I wouldn't bother with a static here. > > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:70: > for (const auto& p : manager->payment_requests_) { > no {} Actually, one question, I don't see you using anything from https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... . Am I just missing it?
On 2017/01/17 23:07:57, sky wrote: > On 2017/01/17 23:05:52, sky wrote: > > LGTM > > > > > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... > > File > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc > > (right): > > > > > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... > > > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:35: > > new net::EmbeddedTestServer(net::EmbeddedTestServer::TYPE_HTTPS)); > > MakeUnique. > > > > > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... > > > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:46: > > static const std::string kClickBuyButtonJs = > > I wouldn't bother with a static here. > > > > > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... > > > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:70: > > for (const auto& p : manager->payment_requests_) { > > no {} > > Actually, one question, I don't see you using anything from > https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... > . Am I just missing it?
On 2017/01/18 14:48:23, Mathieu Perreault wrote: > On 2017/01/17 23:07:57, sky wrote: > > On 2017/01/17 23:05:52, sky wrote: > > > LGTM > > > > > > > > > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... > > > File > > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... > > > > > > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:35: > > > new net::EmbeddedTestServer(net::EmbeddedTestServer::TYPE_HTTPS)); > > > MakeUnique. > > > > > > > > > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... > > > > > > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:46: > > > static const std::string kClickBuyButtonJs = > > > I wouldn't bother with a static here. > > > > > > > > > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... > > > > > > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:70: > > > for (const auto& p : manager->payment_requests_) { > > > no {} > > > > Actually, one question, I don't see you using anything from > > > https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... > > . Am I just missing it? Actually, you are correct for this patch but in follow-up patches I intend on using void MoveMouseToCenterAndPress(views::View* view, ui_controls::MouseButton button, int state, const base::Closure& task); which is only available for interactive ui tests. If I'm wrong and end up not using any of those functions, I'll bring them back to be regular browsertests.
https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc (right): https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:35: new net::EmbeddedTestServer(net::EmbeddedTestServer::TYPE_HTTPS)); On 2017/01/17 23:05:52, sky wrote: > MakeUnique. Done. https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:46: static const std::string kClickBuyButtonJs = On 2017/01/17 23:05:52, sky wrote: > I wouldn't bother with a static here. Done. https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:70: for (const auto& p : manager->payment_requests_) { On 2017/01/17 23:05:52, sky wrote: > no {} Done.
Ok, makes sense. Thanks for clarification. On Wed, Jan 18, 2017 at 6:50 AM, <mathp@chromium.org> wrote: > On 2017/01/18 14:48:23, Mathieu Perreault wrote: >> On 2017/01/17 23:07:57, sky wrote: >> > On 2017/01/17 23:05:52, sky wrote: >> > > LGTM >> > > >> > > >> > >> > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... >> > > File >> > >> > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc >> > > (right): >> > > >> > > >> > >> > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... >> > > >> > >> > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:35: >> > > new net::EmbeddedTestServer(net::EmbeddedTestServer::TYPE_HTTPS)); >> > > MakeUnique. >> > > >> > > >> > >> > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... >> > > >> > >> > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:46: >> > > static const std::string kClickBuyButtonJs = >> > > I wouldn't bother with a static here. >> > > >> > > >> > >> > https://codereview.chromium.org/2631133003/diff/80001/chrome/browser/ui/views... >> > > >> > >> > chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:70: >> > > for (const auto& p : manager->payment_requests_) { >> > > no {} >> > >> > Actually, one question, I don't see you using anything from >> > >> > https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... >> > . Am I just missing it? > > Actually, you are correct for this patch but in follow-up patches I intend > on > using > > void MoveMouseToCenterAndPress(views::View* view, > ui_controls::MouseButton button, > int state, > const base::Closure& task); > > which is only available for interactive ui tests. If I'm wrong and end up > not > using any of those functions, I'll bring them back to be regular > browsertests. > > https://codereview.chromium.org/2631133003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:120001) has been deleted
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The initial approach which you reviewed was flaky. It seems like the timing of Mojo events varies. Therefore I made the test be an observer of PaymentRequestDialog events, and am now waiting on them to occur before moving forward. My observer is flexible to the timing of the Mojo-related events. Would like another look from Rouslan at least, thanks and sorry.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % minor nits https://codereview.chromium.org/2631133003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_dialog.h (right): https://codereview.chromium.org/2631133003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_dialog.h:31: class Observer { nit: ObserverForTest https://codereview.chromium.org/2631133003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_dialog.h:37: // be notified of dialog events as they happen (but may be NULL). Mention that the observer should outlive this object.
Thanks, submitting. https://codereview.chromium.org/2631133003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_dialog.h (right): https://codereview.chromium.org/2631133003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_dialog.h:31: class Observer { On 2017/01/19 18:20:12, rouslan wrote: > nit: ObserverForTest Done. https://codereview.chromium.org/2631133003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_dialog.h:37: // be notified of dialog events as they happen (but may be NULL). On 2017/01/19 18:20:12, rouslan wrote: > Mention that the observer should outlive this object. Done.
The CQ bit was checked by mathp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2631133003/#ps160001 (title: "ObserverForTest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484851847584700,
"parent_rev": "a10ba600a7ad192660ded5dbe0ef94a559724452", "commit_rev":
"a397e24fc14ad76c5e7d462eb7b934da031629b3"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Introduce basic interactive browsertests for Payment Request According to code documentation [1], the tests need to be in "interactive_ui_tests" because they will need focus, and thus can't be sharded like regular browsertests. I also went with the approach of putting several test classes in the same file for a couple of reasons: we may not have many tests at first (thus little clutter), and it's easier to add a test when one can see what other tests are doing nearby. Certainly later on as these things are less true, we can split into multiple files as needed. Also want to mention that the tests are simplistic now; a follow-up CL will focus on View elements and click them. [1] https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... BUG=679734 TEST=./out/Default/interactive_ui_tests --gtest_filter=PaymentRequest* ========== to ========== [Payments] Introduce basic interactive browsertests for Payment Request According to code documentation [1], the tests need to be in "interactive_ui_tests" because they will need focus, and thus can't be sharded like regular browsertests. I also went with the approach of putting several test classes in the same file for a couple of reasons: we may not have many tests at first (thus little clutter), and it's easier to add a test when one can see what other tests are doing nearby. Certainly later on as these things are less true, we can split into multiple files as needed. Also want to mention that the tests are simplistic now; a follow-up CL will focus on View elements and click them. [1] https://cs.chromium.org/chromium/src/chrome/test/base/interactive_test_utils.... BUG=679734 TEST=./out/Default/interactive_ui_tests --gtest_filter=PaymentRequest* Review-Url: https://codereview.chromium.org/2631133003 Cr-Commit-Position: refs/heads/master@{#444825} Committed: https://chromium.googlesource.com/chromium/src/+/a397e24fc14ad76c5e7d462eb7b9... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://chromium.googlesource.com/chromium/src/+/a397e24fc14ad76c5e7d462eb7b9... |
