Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2382963002:
Views: Add two disabled tests for invoking the CC unmask prompt dialog. (Closed)
|
Created:
4 years, 2 months ago by Patti Lor Modified:
4 years, 2 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, estade+watch_chromium.org, vabr+watchlistautofill_chromium.org, jdonnelly+autofillwatch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionViews: Add two disabled tests for invoking the CC unmask prompt dialog.
The card unmask prompt is quite difficult to invoke (a signed-in official build
is needed with saved credit cards stored), so add two disabled tests which only
invoke the dialog UI, one with an expired and one with a valid credit card. This
is needed for easier testing and UI review. Note that these tests need to be
disabled because of a call to base::RunLoop().Run() which will make them always
time out if enabled.
Using disabled browser tests instead of another approach means that the mock
models/data used in the pre-existing tests for these dialogs can be re-used,
which will be a smaller maintenance burden. Run them with the browser_tests
target: ./browser_tests --gtest_filter=
"CardUnmaskPromptViewBrowserTest.DISABLED_Invoke*"
--gtest_also_run_disabled_tests --ui-test-action-max-timeout=10000000
--test-launcher-timeout=10000000
Use --secondary-ui-md for new Harmony styling.
BUG=649908
Committed: https://crrev.com/24ac066a9656e83b071cca34cbfe15c3f29a1539
Cr-Commit-Position: refs/heads/master@{#426663}
Patch Set 1 #Patch Set 2 : Initial approach for invoking UI tests. #
Total comments: 10
Patch Set 3 : Review comments, add a second test to show the dialog without month/year controls. #Patch Set 4 : Rebase #
Total comments: 14
Patch Set 5 : Review comments. #
Total comments: 22
Patch Set 6 : Review comments. #Patch Set 7 : Fix gn fail. #
Total comments: 2
Patch Set 8 : Move header file to be included in all platforms. #Patch Set 9 : Give user_interactive_test_case* its own if statement. #
Total comments: 10
Patch Set 10 : Review comments. #Patch Set 11 : Rebase? #
Total comments: 4
Patch Set 12 : Switch to CreditCardExpiry to enum class : uint8_t. #
Total comments: 1
Messages
Total messages: 83 (64 generated)
The CQ bit was checked by patricialor@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by patricialor@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...
patricialor@chromium.org changed reviewers: + tapted@chromium.org
Hi Trent, PTAL. This doesn't really work yet (timeouts don't work) and haven't yet come up with a way to kill the test nicely when the browser is closed, but am kinda doubtful about this approach - any feedback?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... File chrome/test/base/invoke_ui_browser_test.cc (right): https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... chrome/test/base/invoke_ui_browser_test.cc:36: base::RunLoop().Run(); I'd put this in the test case itself. So hm - maybe we don't need InvokeUIBrowserTest at all. We can just do something like // Foregrounds the test process and spins a run loop to run a test case as interactive UI. void RunTestIteractively(); use_interactive_test_case.cc #if !defined(OS_MACOSX) void RunTestIteractively() { base::RunLoop.Run(); } #endif use_interactive_test_case.mm void RunTestIteractively() { [NSApp setActivationPolicy:NSApplicationActivationPolicyRegular]; /* activate ignoring other apps */ base::RunLoop.Run(); } https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... File chrome/test/base/invoke_ui_browser_test.h (right): https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... chrome/test/base/invoke_ui_browser_test.h:12: class InvokeUIBrowserTest { I'd probably have this inherit from InProcessBrowserTest (we can also use template inheritance if there are lots of tests that have a class already inheriting from InProcessBrowserTest). https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... chrome/test/base/invoke_ui_browser_test.h:18: void SetCommandLineFlags(base::CommandLine* command_line); This this can override SetUpCommandLine and check for an existing flag like "--show-ui", but we can ignore timeouts for now. https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/move_t... File chrome/test/base/move_test_process_to_front_mac.mm (right): https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/move_t... chrome/test/base/move_test_process_to_front_mac.mm:10: SetFrontProcess(&psn); some of these apis might be deprecated...
The CQ bit was checked by patricialor@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by patricialor@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...
Thanks for the review Trent :) PTAL. https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... File chrome/test/base/invoke_ui_browser_test.cc (right): https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... chrome/test/base/invoke_ui_browser_test.cc:36: base::RunLoop().Run(); On 2016/10/13 00:53:34, tapted wrote: > I'd put this in the test case itself. So hm - maybe we don't need > InvokeUIBrowserTest at all. We can just do something like > > // Foregrounds the test process and spins a run loop to run a test case as > interactive UI. > void RunTestIteractively(); > > use_interactive_test_case.cc > > #if !defined(OS_MACOSX) > void RunTestIteractively() { > base::RunLoop.Run(); > } > #endif > > use_interactive_test_case.mm > > void RunTestIteractively() { > [NSApp setActivationPolicy:NSApplicationActivationPolicyRegular]; > /* activate ignoring other apps */ > base::RunLoop.Run(); > } Done. https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... File chrome/test/base/invoke_ui_browser_test.h (right): https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... chrome/test/base/invoke_ui_browser_test.h:12: class InvokeUIBrowserTest { On 2016/10/13 00:53:34, tapted wrote: > I'd probably have this inherit from InProcessBrowserTest (we can also use > template inheritance if there are lots of tests that have a class already > inheriting from InProcessBrowserTest). Deleted this as per your other comments. https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... chrome/test/base/invoke_ui_browser_test.h:18: void SetCommandLineFlags(base::CommandLine* command_line); On 2016/10/13 00:53:34, tapted wrote: > This this can override SetUpCommandLine and check for an existing flag like > "--show-ui", but we can ignore timeouts for now. Did you have something in mind for fixing the timeouts later on? Or is the plan to just run the test with the timeout flags manually? I'm also not really sure what you mean by the "--show-ui" thing? https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/move_t... File chrome/test/base/move_test_process_to_front_mac.mm (right): https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/move_t... chrome/test/base/move_test_process_to_front_mac.mm:10: SetFrontProcess(&psn); On 2016/10/13 00:53:34, tapted wrote: > some of these apis might be deprecated... You're right, the documentation seems to have disappeared from the Apple developer site but SetFrontProcess() was deprecated in 10.9 according to /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/System/Library/Frameworks/ApplicationServices.framework/Versions/A/Frameworks/HIServices.framework/Versions/A/Headers/Processes.h. Replaced with NSApplication methods as it recommends in the same file. TransformProcessType is not deprecated and I think ProcessSerialNumber and kCurrentProcess are fine too, so have left everything else unchanged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... File chrome/test/base/invoke_ui_browser_test.h (right): https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... chrome/test/base/invoke_ui_browser_test.h:18: void SetCommandLineFlags(base::CommandLine* command_line); On 2016/10/13 05:39:21, Patti Lor wrote: > On 2016/10/13 00:53:34, tapted wrote: > > This this can override SetUpCommandLine and check for an existing flag like > > "--show-ui", but we can ignore timeouts for now. > > Did you have something in mind for fixing the timeouts later on? Or is the plan > to just run the test with the timeout flags manually? > > I'm also not really sure what you mean by the "--show-ui" thing? I think we can make a python script to collect all the entrypoints for summoning test dialogs and adding any extra flags we need. The goal of this CL is to leverage the mock models/data used in tests so that we don't maintain those separately (let's say something about that in the CL description). If we later find that there's "stuff" that needs to be done via the InProcessBrowserTest interfaces common to multiple test dialogs, we could check for that in a shared base class and switch it on e.g. by looking for a new flag we add like --show-ui, but we might not need it. https://codereview.chromium.org/2382963002/diff/60001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/2382963002/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:103: CreditCard cc; nit: CreditCard cc = expired ? test::GetMaskedServerCard() : test::GetMaskedServerCardAmex(); https://codereview.chromium.org/2382963002/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:130: IN_PROC_BROWSER_TEST_F(CardUnmaskPromptViewBrowserTest, nit: these need a comment https://codereview.chromium.org/2382963002/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:145: // TODO(bondd): bring up on Mac. hrmh - we need to do something about this. I don't think bondd is around any longer and there's no crbug link for the TODO. Probably this will get lost/forgotten about, but at the very least we should try to bring up this test when switching to mac_views_browser - can you investigate whether there's an investigation on the initial code review via git blame? Maybe this is already working with MacViewsWebUIDialogs enabled. If there's no appropriate existing bug, we might need a "Phase 4" bug (like phase3 in http://crbug.com/603386). https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/move_t... File chrome/test/base/move_test_process_to_front_mac.h (right): https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/move_t... chrome/test/base/move_test_process_to_front_mac.h:8: void SetProcessToFront(); nit: needs documentation, but see next comment - I don't think we need this https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/move_t... File chrome/test/base/move_test_process_to_front_mac.mm (right): https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/move_t... chrome/test/base/move_test_process_to_front_mac.mm:9: TransformProcessType(&psn, kProcessTransformToForegroundApplication); I'm pretty sure this duplicates what [NSApp setActivationPolicy:NSApplicationActivationPolicyRegular]; does. But then since it's just 1-2 lines, I don't think we need SetProcessToFront. Let's not change interactive_test_utils_mac.mm for now. We can remove the SetProcessToFront() from RunTestInteractively(), and just do [NSApp setActivationPolicy:NSApplicationActivationPolicyRegular]; [NSApp activateIgnoringOtherApps:YES]; https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/use_in... File chrome/test/base/use_interactive_test_case.h (right): https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/use_in... chrome/test/base/use_interactive_test_case.h:5: #ifndef CHROME_TEST_BASE_USE_INTERACTIVE_TEST_CASE_H_ I think a nicer place for this is ui/base/test -- that's where most of the special-case-mac stuff is for ui tests currently. https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/use_in... chrome/test/base/use_interactive_test_case.h:11: void RunTestInteractively(); Let's put this in namespace test
The CQ bit was checked by patricialor@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Views: Add a fake 'test' just for invoking the CC unmask prompt. WIP. The card unmask prompt is super difficult to invoke (an official build is needed), so add a DISABLED_Invoke test for easier testing and UI review. BUG=649908 ========== to ========== Views: Add two disabled tests for invoking the CC unmask prompt dialog. The card unmask prompt is quite difficult to invoke (a signed-in official build is needed with saved credit cards stored), so add two disabled tests which only invoke the dialog UI, one with an expired and one with a valid credit card. This is needed for easier testing and UI review. Note that these tests need to be disabled because of a call to base::RunLoop().Run() which will make them always time out if enabled. Using disabled browser tests instead of another approach means that the mock models/data used in the pre-existing tests for these dialogs can be re-used, which will be a smaller maintenance burden. BUG=649908 ==========
Thanks Trent, PTAL w/ updated CL description. https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... File chrome/test/base/invoke_ui_browser_test.h (right): https://codereview.chromium.org/2382963002/diff/20001/chrome/test/base/invoke... chrome/test/base/invoke_ui_browser_test.h:18: void SetCommandLineFlags(base::CommandLine* command_line); On 2016/10/16 23:48:01, tapted wrote: > On 2016/10/13 05:39:21, Patti Lor wrote: > > On 2016/10/13 00:53:34, tapted wrote: > > > This this can override SetUpCommandLine and check for an existing flag like > > > "--show-ui", but we can ignore timeouts for now. > > > > Did you have something in mind for fixing the timeouts later on? Or is the > plan > > to just run the test with the timeout flags manually? > > > > I'm also not really sure what you mean by the "--show-ui" thing? > > I think we can make a python script to collect all the entrypoints for summoning > test dialogs and adding any extra flags we need. The goal of this CL is to > leverage the mock models/data used in tests so that we don't maintain those > separately (let's say something about that in the CL description). > > If we later find that there's "stuff" that needs to be done via the > InProcessBrowserTest interfaces common to multiple test dialogs, we could check > for that in a shared base class and switch it on e.g. by looking for a new flag > we add like --show-ui, but we might not need it. Ah ok, I didn't realise you had a separate thing in mind to invoke all the UI tests. Thanks for the clarification! https://codereview.chromium.org/2382963002/diff/60001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/2382963002/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:103: CreditCard cc; On 2016/10/16 23:48:01, tapted wrote: > nit: > > CreditCard cc = expired > ? test::GetMaskedServerCard() > : test::GetMaskedServerCardAmex(); Done. https://codereview.chromium.org/2382963002/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:130: IN_PROC_BROWSER_TEST_F(CardUnmaskPromptViewBrowserTest, On 2016/10/16 23:48:01, tapted wrote: > nit: these need a comment Done. https://codereview.chromium.org/2382963002/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:145: // TODO(bondd): bring up on Mac. On 2016/10/16 23:48:01, tapted wrote: > hrmh - we need to do something about this. I don't think bondd is around any > longer and there's no crbug link for the TODO. Probably this will get > lost/forgotten about, but at the very least we should try to bring up this test > when switching to mac_views_browser - can you investigate whether there's an > investigation on the initial code review via git blame? > > Maybe this is already working with MacViewsWebUIDialogs enabled. > > If there's no appropriate existing bug, we might need a "Phase 4" bug (like > phase3 in http://crbug.com/603386). I don't think there's any ongoing work - the most related bug I could find is https://crbug.com/448572 which has been marked fixed already. The TODO was originally added by estade@ (not bondd@, see https://codereview.chromium.org/1020013003), so maybe they would be a good person to ask if we'd like to double check there isn't someone working on it. It seems to work already for MacViews though, just not Cocoa (there needs to be a Cocoa version of CardUnmaskPromptViewTesterViews). Are we interested in fixing for Cocoa as well? Or just refactor the test out so it's defined on MacViews? https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/move_t... File chrome/test/base/move_test_process_to_front_mac.h (right): https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/move_t... chrome/test/base/move_test_process_to_front_mac.h:8: void SetProcessToFront(); On 2016/10/16 23:48:01, tapted wrote: > nit: needs documentation, but see next comment - I don't think we need this Deleted as per your other comment. https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/move_t... File chrome/test/base/move_test_process_to_front_mac.mm (right): https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/move_t... chrome/test/base/move_test_process_to_front_mac.mm:9: TransformProcessType(&psn, kProcessTransformToForegroundApplication); On 2016/10/16 23:48:01, tapted wrote: > I'm pretty sure this duplicates what [NSApp > setActivationPolicy:NSApplicationActivationPolicyRegular]; does. But then since > it's just 1-2 lines, I don't think we need SetProcessToFront. Let's not change > interactive_test_utils_mac.mm for now. > > We can remove the SetProcessToFront() from RunTestInteractively(), and just do > > [NSApp setActivationPolicy:NSApplicationActivationPolicyRegular]; > [NSApp activateIgnoringOtherApps:YES]; > Done. https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/use_in... File chrome/test/base/use_interactive_test_case.h (right): https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/use_in... chrome/test/base/use_interactive_test_case.h:5: #ifndef CHROME_TEST_BASE_USE_INTERACTIVE_TEST_CASE_H_ On 2016/10/16 23:48:01, tapted wrote: > I think a nicer place for this is ui/base/test -- that's where most of the > special-case-mac stuff is for ui tests currently. Done. https://codereview.chromium.org/2382963002/diff/60001/chrome/test/base/use_in... chrome/test/base/use_interactive_test_case.h:11: void RunTestInteractively(); On 2016/10/16 23:48:01, tapted wrote: > Let's put this in namespace test Done.
https://codereview.chromium.org/2382963002/diff/80001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/2382963002/diff/80001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:6: #include "base/command_line.h" nit: unused https://codereview.chromium.org/2382963002/diff/80001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2382963002/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:323: "../../ui/base/test/use_interactive_test_case.h", This should go in //ui/base:test_support (i.e. ui/base/BUILD.gn, in the static_library("test_support") sources list https://codereview.chromium.org/2382963002/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:324: "../../ui/base/test/use_interactive_test_case_mac.mm", this probably needs to go in the !is_ios branch (and the .cc in the else -- I guess ios complicates things a bit, but this approach allows us to remove the #ifdef mac) https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... File ui/base/test/use_interactive_test_case.cc (right): https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case.cc:5: #include "ui/base/test/use_interactive_test_case.h" oh dang-it - my review comment had a typo :(. I meant "user_interactive_test_case.h" (i.e. to distinguish it from interactive_ui_test). https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case.cc:11: #if !defined(OS_MACOSX) per the BUILD.gn comment (and because of ios) I think we need to remove this guard and pick the implementation file in the .gn instead https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case.cc:16: } nit: blank line before + " // namespace test" https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... File ui/base/test/use_interactive_test_case.h (right): https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case.h:12: // invoke UI (designed for manual testing of hard-to-summon dialogs). This comment is good. I think we just need a sentence like "This function does not return." (We pass in a base::Closure* quit that could allow a test case to quit the run loop, but that requires extra plumbing). https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case.h:14: } nit: blank line before + " // namespace test" https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... File ui/base/test/use_interactive_test_case_mac.mm (right): https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case_mac.mm:7: #include <Cocoa/Cocoa.h> nit: import https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case_mac.mm:18: } nit: blank line before + " // namespace test"
The CQ bit was checked by patricialor@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL, thanks! https://codereview.chromium.org/2382963002/diff/80001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/2382963002/diff/80001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:6: #include "base/command_line.h" On 2016/10/17 23:44:50, tapted wrote: > nit: unused Done. https://codereview.chromium.org/2382963002/diff/80001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2382963002/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:323: "../../ui/base/test/use_interactive_test_case.h", On 2016/10/17 23:44:50, tapted wrote: > This should go in //ui/base:test_support (i.e. ui/base/BUILD.gn, in the > static_library("test_support") sources list Done. https://codereview.chromium.org/2382963002/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:324: "../../ui/base/test/use_interactive_test_case_mac.mm", On 2016/10/17 23:44:50, tapted wrote: > this probably needs to go in the !is_ios branch (and the .cc in the else -- I > guess ios complicates things a bit, but this approach allows us to remove the > #ifdef mac) Done (but the other way around, with the .mm in the else). https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... File ui/base/test/use_interactive_test_case.cc (right): https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case.cc:5: #include "ui/base/test/use_interactive_test_case.h" On 2016/10/17 23:44:50, tapted wrote: > oh dang-it - my review comment had a typo :(. I meant > "user_interactive_test_case.h" (i.e. to distinguish it from > interactive_ui_test). No worries, done :) https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case.cc:11: #if !defined(OS_MACOSX) On 2016/10/17 23:44:50, tapted wrote: > per the BUILD.gn comment (and because of ios) I think we need to remove this > guard and pick the implementation file in the .gn instead Done. https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case.cc:16: } On 2016/10/17 23:44:50, tapted wrote: > nit: blank line before + " // namespace test" Oops, sorry about these - git cl format deleted them I think. Done. https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... File ui/base/test/use_interactive_test_case.h (right): https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case.h:12: // invoke UI (designed for manual testing of hard-to-summon dialogs). On 2016/10/17 23:44:51, tapted wrote: > This comment is good. I think we just need a sentence like > > "This function does not return." > > > (We pass in a base::Closure* quit that could allow a test case to quit the run > loop, but that requires extra plumbing). Done. I think not doing the quit thing is fine too, since |exit_when_last_browser_closes_| is true by default. https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case.h:14: } On 2016/10/17 23:44:51, tapted wrote: > nit: blank line before + " // namespace test" Done. https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... File ui/base/test/use_interactive_test_case_mac.mm (right): https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case_mac.mm:7: #include <Cocoa/Cocoa.h> On 2016/10/17 23:44:51, tapted wrote: > nit: import Done. https://codereview.chromium.org/2382963002/diff/80001/ui/base/test/use_intera... ui/base/test/use_interactive_test_case_mac.mm:18: } On 2016/10/17 23:44:51, tapted wrote: > nit: blank line before + " // namespace test" Done.
lgtm https://codereview.chromium.org/2382963002/diff/120001/ui/base/BUILD.gn File ui/base/BUILD.gn (right): https://codereview.chromium.org/2382963002/diff/120001/ui/base/BUILD.gn#newco... ui/base/BUILD.gn:647: "test/user_interactive_test_case.h", this should go in the sources = list above, so it's included in conjunction with the .mm
The CQ bit was checked by patricialor@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Views: Add two disabled tests for invoking the CC unmask prompt dialog. The card unmask prompt is quite difficult to invoke (a signed-in official build is needed with saved credit cards stored), so add two disabled tests which only invoke the dialog UI, one with an expired and one with a valid credit card. This is needed for easier testing and UI review. Note that these tests need to be disabled because of a call to base::RunLoop().Run() which will make them always time out if enabled. Using disabled browser tests instead of another approach means that the mock models/data used in the pre-existing tests for these dialogs can be re-used, which will be a smaller maintenance burden. BUG=649908 ========== to ========== Views: Add two disabled tests for invoking the CC unmask prompt dialog. The card unmask prompt is quite difficult to invoke (a signed-in official build is needed with saved credit cards stored), so add two disabled tests which only invoke the dialog UI, one with an expired and one with a valid credit card. This is needed for easier testing and UI review. Note that these tests need to be disabled because of a call to base::RunLoop().Run() which will make them always time out if enabled. Using disabled browser tests instead of another approach means that the mock models/data used in the pre-existing tests for these dialogs can be re-used, which will be a smaller maintenance burden. Run them with the browser_tests target: ./browser_tests --gtest_filter= "CardUnmaskPromptViewBrowserTest.DISABLED_Invoke*" --gtest_also_run_disabled_tests --ui-test-action-max-timeout=10000000 --test-launcher-timeout=10000000 Use --secondary-ui-md for new Harmony styling. BUG=649908 ==========
The CQ bit was checked by patricialor@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
patricialor@chromium.org changed reviewers: + isherman@chromium.org
isherman@chromium.org: Please review changes in chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc tapted@: I changed ui/base/BUILD.gn after your LGTM, PTAL! Thanks both! https://codereview.chromium.org/2382963002/diff/80001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2382963002/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:324: "../../ui/base/test/use_interactive_test_case_mac.mm", On 2016/10/18 03:44:25, Patti Lor wrote: > On 2016/10/17 23:44:50, tapted wrote: > > this probably needs to go in the !is_ios branch (and the .cc in the else -- I > > guess ios complicates things a bit, but this approach allows us to remove the > > #ifdef mac) > > Done (but the other way around, with the .mm in the else). I went back and double checked everything still worked and turns out this isn't actually correct, so I gave use_interactive_test_case*.{cc, mm} their own is_mac if statement (see patchset 9). (Having the .cc in the else instead makes compile errors.) https://codereview.chromium.org/2382963002/diff/120001/ui/base/BUILD.gn File ui/base/BUILD.gn (right): https://codereview.chromium.org/2382963002/diff/120001/ui/base/BUILD.gn#newco... ui/base/BUILD.gn:647: "test/user_interactive_test_case.h", On 2016/10/18 06:01:12, tapted wrote: > this should go in the sources = list above, so it's included in conjunction with > the .mm Done - thanks!!
lgtm https://codereview.chromium.org/2382963002/diff/80001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2382963002/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:324: "../../ui/base/test/use_interactive_test_case_mac.mm", On 2016/10/18 23:41:48, Patti Lor wrote: > On 2016/10/18 03:44:25, Patti Lor wrote: > > On 2016/10/17 23:44:50, tapted wrote: > > > this probably needs to go in the !is_ios branch (and the .cc in the else -- > I > > > guess ios complicates things a bit, but this approach allows us to remove > the > > > #ifdef mac) > > > > Done (but the other way around, with the .mm in the else). > > I went back and double checked everything still worked and turns out this isn't > actually correct, so I gave use_interactive_test_case*.{cc, mm} their own is_mac > if statement (see patchset 9). (Having the .cc in the else instead makes compile > errors.) Ah yep - makes sense. We could put the .mm in the !is_ios branch (gn will filter it automatically on !mac), but the .cc still needs to be in !is_mac, so it doesn't buy us much. So what you have looks good. https://codereview.chromium.org/2382963002/diff/160001/ui/base/test/user_inte... File ui/base/test/user_interactive_test_case.cc (right): https://codereview.chromium.org/2382963002/diff/160001/ui/base/test/user_inte... ui/base/test/user_interactive_test_case.cc:15: } nit: // namespace test https://codereview.chromium.org/2382963002/diff/160001/ui/base/test/user_inte... File ui/base/test/user_interactive_test_case.h (right): https://codereview.chromium.org/2382963002/diff/160001/ui/base/test/user_inte... ui/base/test/user_interactive_test_case.h:16: } nit: // namespace test https://codereview.chromium.org/2382963002/diff/160001/ui/base/test/user_inte... File ui/base/test/user_interactive_test_case_mac.mm (right): https://codereview.chromium.org/2382963002/diff/160001/ui/base/test/user_inte... ui/base/test/user_interactive_test_case_mac.mm:19: } nit: // namespace test
The autofill test file LGTM % nits: https://codereview.chromium.org/2382963002/diff/160001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/2382963002/diff/160001/chrome/browser/ui/auto... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:99: void ShowUI(bool expired) { nit: Please define an enum with values EXPIRED and NOT_EXPIRED, rather than passing a bool here. The advantage is that call sites become much easier to read -- ShowUI(EXPIRED) is clearer than ShowUI(true). (Please feel free to adjust the naming for further clarity if you'd like to. https://codereview.chromium.org/2382963002/diff/160001/chrome/browser/ui/auto... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:102: CreditCard cc = nit: s/cc/card
The CQ bit was checked by patricialor@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by patricialor@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 unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by patricialor@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2382963002/#ps200001 (title: "Rebase?")
The CQ bit was unchecked by patricialor@chromium.org
Thanks for the reviews! https://codereview.chromium.org/2382963002/diff/160001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/2382963002/diff/160001/chrome/browser/ui/auto... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:99: void ShowUI(bool expired) { On 2016/10/18 23:59:58, Ilya Sherman wrote: > nit: Please define an enum with values EXPIRED and NOT_EXPIRED, rather than > passing a bool here. The advantage is that call sites become much easier to > read -- ShowUI(EXPIRED) is clearer than ShowUI(true). (Please feel free to > adjust the naming for further clarity if you'd like to. Done, thanks! https://codereview.chromium.org/2382963002/diff/160001/chrome/browser/ui/auto... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:102: CreditCard cc = On 2016/10/18 23:59:58, Ilya Sherman wrote: > nit: s/cc/card Done. https://codereview.chromium.org/2382963002/diff/160001/ui/base/test/user_inte... File ui/base/test/user_interactive_test_case.cc (right): https://codereview.chromium.org/2382963002/diff/160001/ui/base/test/user_inte... ui/base/test/user_interactive_test_case.cc:15: } On 2016/10/18 23:49:19, tapted wrote: > nit: // namespace test Done. https://codereview.chromium.org/2382963002/diff/160001/ui/base/test/user_inte... File ui/base/test/user_interactive_test_case.h (right): https://codereview.chromium.org/2382963002/diff/160001/ui/base/test/user_inte... ui/base/test/user_interactive_test_case.h:16: } On 2016/10/18 23:49:19, tapted wrote: > nit: // namespace test Done, thanks. This is probably why formatting would try to get rid of the whitespace on the line above D: https://codereview.chromium.org/2382963002/diff/160001/ui/base/test/user_inte... File ui/base/test/user_interactive_test_case_mac.mm (right): https://codereview.chromium.org/2382963002/diff/160001/ui/base/test/user_inte... ui/base/test/user_interactive_test_case_mac.mm:19: } On 2016/10/18 23:49:19, tapted wrote: > nit: // namespace test Done.
The CQ bit was checked by patricialor@chromium.org
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 isherman@chromium.org
https://codereview.chromium.org/2382963002/diff/200001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/2382963002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:29: enum CreditCardExpiry { EXPIRED, VALID }; nit: Please use enum class, since that seems to be how you're using it below anyway :) https://codereview.chromium.org/2382963002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:31: } // namespace nit: There's already an anonymous namespace below -- please move this into it.
The CQ bit was checked by patricialor@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi isherman@, PTAL - just wanted to double check if this looks good to you. Thanks! https://codereview.chromium.org/2382963002/diff/200001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/2382963002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:29: enum CreditCardExpiry { EXPIRED, VALID }; On 2016/10/20 01:13:35, Ilya Sherman wrote: > nit: Please use enum class, since that seems to be how you're using it below > anyway :) Oops, thanks for catching these in time! Fixed. https://codereview.chromium.org/2382963002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:31: } // namespace On 2016/10/20 01:13:35, Ilya Sherman wrote: > nit: There's already an anonymous namespace below -- please move this into it. Done.
Still LGTM, thanks. https://codereview.chromium.org/2382963002/diff/220001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/2382963002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:31: enum class CreditCardExpiry : uint8_t { EXPIRED, VALID }; Optional nit: Probably no need to specify uint8_t as the backing storage type.
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2382963002/#ps220001 (title: "Switch to CreditCardExpiry to enum class : uint8_t.")
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 #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Views: Add two disabled tests for invoking the CC unmask prompt dialog. The card unmask prompt is quite difficult to invoke (a signed-in official build is needed with saved credit cards stored), so add two disabled tests which only invoke the dialog UI, one with an expired and one with a valid credit card. This is needed for easier testing and UI review. Note that these tests need to be disabled because of a call to base::RunLoop().Run() which will make them always time out if enabled. Using disabled browser tests instead of another approach means that the mock models/data used in the pre-existing tests for these dialogs can be re-used, which will be a smaller maintenance burden. Run them with the browser_tests target: ./browser_tests --gtest_filter= "CardUnmaskPromptViewBrowserTest.DISABLED_Invoke*" --gtest_also_run_disabled_tests --ui-test-action-max-timeout=10000000 --test-launcher-timeout=10000000 Use --secondary-ui-md for new Harmony styling. BUG=649908 ========== to ========== Views: Add two disabled tests for invoking the CC unmask prompt dialog. The card unmask prompt is quite difficult to invoke (a signed-in official build is needed with saved credit cards stored), so add two disabled tests which only invoke the dialog UI, one with an expired and one with a valid credit card. This is needed for easier testing and UI review. Note that these tests need to be disabled because of a call to base::RunLoop().Run() which will make them always time out if enabled. Using disabled browser tests instead of another approach means that the mock models/data used in the pre-existing tests for these dialogs can be re-used, which will be a smaller maintenance burden. Run them with the browser_tests target: ./browser_tests --gtest_filter= "CardUnmaskPromptViewBrowserTest.DISABLED_Invoke*" --gtest_also_run_disabled_tests --ui-test-action-max-timeout=10000000 --test-launcher-timeout=10000000 Use --secondary-ui-md for new Harmony styling. BUG=649908 Committed: https://crrev.com/24ac066a9656e83b071cca34cbfe15c3f29a1539 Cr-Commit-Position: refs/heads/master@{#426663} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/24ac066a9656e83b071cca34cbfe15c3f29a1539 Cr-Commit-Position: refs/heads/master@{#426663} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
