Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2569703004:
Add "interactive" UI integration tests for PermissionRequestManager. (Closed)
DescriptionAdd "interactive" UI integration tests for PermissionRequestManager.
Existing PermissionRequestManager tests use a
MockPermissionPromptFactory which replaces the code that generates
actual UI with stubs. To get coverage of the UI creation code, and to
allow the set of permission prompts to be easily "invoked" for UI
review, extend PermissionRequestManagerBrowserTest with one that uses
"real" UI rather than a mock.
Then, add "Dialog" BrowserTests for a range of permission types that can
be requested. These are run on the bots or (optionally) interactively
with a command such as `browser_tests
--gtest_filter=BrowserDialogTest.Invoke --interactive
--dialog=PermissionDialogTest.InvokeDialog_multiple`
BUG=605667, 663935
Review-Url: https://codereview.chromium.org/2569703004
Cr-Commit-Position: refs/heads/master@{#457296}
Committed: https://chromium.googlesource.com/chromium/src/+/38424d53080c397f8cc13c6581fe937070922ac0
Patch Set 1 #Patch Set 2 : Neater #Patch Set 3 : Add media requests #Patch Set 4 : rebase #Patch Set 5 : CrOS test #
Total comments: 6
Patch Set 6 : rebase? #Patch Set 7 : Respond to comments + adapt to new MediaRequest API #
Messages
Total messages: 33 (25 generated)
The CQ bit was checked by tapted@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Opt RegisterProtocolHandler into TEST_BROWSER_DIALOG BUG= ========== to ========== Add Permission Requests to TEST_BROWSER_DIALOG BUG= ==========
The CQ bit was checked by tapted@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 tapted@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 ========== Add Permission Requests to TEST_BROWSER_DIALOG BUG= ========== to ========== Add "interactive" UI integration tests for PermissionRequestManager. Existing PermissionRequestManager tests use a MockPermissionPromptFactory which replaces the code that generates actual UI with stubs. To get coverage of the UI creation code, and to allow the set of permission prompts to be easily "invoked" for UI review, extend PermissionRequestManagerBrowserTest with one that uses "real" UI rather than a mock. Then, add "Dialog" BrowserTests for a range of permission types that can be requested. These are run on the bots or (optionally) interactively with a command such as `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=PermissionDialogTest.InvokeDialog_multiple` BUG=605667 ==========
Description was changed from ========== Add "interactive" UI integration tests for PermissionRequestManager. Existing PermissionRequestManager tests use a MockPermissionPromptFactory which replaces the code that generates actual UI with stubs. To get coverage of the UI creation code, and to allow the set of permission prompts to be easily "invoked" for UI review, extend PermissionRequestManagerBrowserTest with one that uses "real" UI rather than a mock. Then, add "Dialog" BrowserTests for a range of permission types that can be requested. These are run on the bots or (optionally) interactively with a command such as `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=PermissionDialogTest.InvokeDialog_multiple` BUG=605667 ========== to ========== Add "interactive" UI integration tests for PermissionRequestManager. Existing PermissionRequestManager tests use a MockPermissionPromptFactory which replaces the code that generates actual UI with stubs. To get coverage of the UI creation code, and to allow the set of permission prompts to be easily "invoked" for UI review, extend PermissionRequestManagerBrowserTest with one that uses "real" UI rather than a mock. Then, add "Dialog" BrowserTests for a range of permission types that can be requested. These are run on the bots or (optionally) interactively with a command such as `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=PermissionDialogTest.InvokeDialog_multiple` BUG=605667 ==========
tapted@chromium.org changed reviewers: + raymes@chromium.org
Hi Raymes, please take a look. I dwelled on this for a bit thinking "this feels like a lot of setup/utility code" - happy to move this around / restructure / flesh out the TestAPI if you think it might be useful elsewhere. Otherwise, I've just kept it encapsulated in the browsertest.cc file - it can always be moved out later when something needs it. The main thing I'm considering is to make MediaStreamDevicesControllerTest::SetUp() use MediaStreamDevicesControllerTestApi to share a bit more code (and move the test api out into its own .h/.cc, and maybe remove MediaStreamDevicesControllerTest's direct friendship), but I don't know if these test files should be coupled together this way, or if that's better as a follow-up anyway. See what you think - Thanks!
1 request. I think keeping this decoupled from MediaStreamDevicesController is good for now. I'm doing some major refactoring of that class so the tests might change. Hopefully the PermissionRequest for media becomes similar to the other ones and we can then remove the special casing here :) Thanks Trent! https://codereview.chromium.org/2569703004/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager_browsertest.cc (right): https://codereview.chromium.org/2569703004/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager_browsertest.cc:103: InProcessBrowserTest::SetUpOnMainThread(); // Skip super. nit: could you explain why we skip the super setup? https://codereview.chromium.org/2569703004/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager_browsertest.cc:236: case CONTENT_SETTINGS_TYPE_MIDI: // Disabled (no icon). This one actually doesn't prompt either. https://codereview.chromium.org/2569703004/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager_browsertest.cc:264: // No default case, to pick up new enum values. I'd actually suggest adding default: here because it's going to become burdensome for people adding new types that don't have prompts. It may be better to switch on PermissionRequestType. That more closely aligns with what can show up in a prompt.
The CQ bit was checked by tapted@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...
PTAL - extended the MediaStreamDevicesControllerTestApi to cater for the API changes https://codereview.chromium.org/2569703004/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_request_manager_browsertest.cc (right): https://codereview.chromium.org/2569703004/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager_browsertest.cc:103: InProcessBrowserTest::SetUpOnMainThread(); // Skip super. On 2017/03/12 23:33:53, raymes wrote: > nit: could you explain why we skip the super setup? Done. added comment // Skip super: It will install a mock permission UI factory, but for this // test we want to show "real" UI. https://codereview.chromium.org/2569703004/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager_browsertest.cc:236: case CONTENT_SETTINGS_TYPE_MIDI: // Disabled (no icon). On 2017/03/12 23:33:53, raymes wrote: > This one actually doesn't prompt either. Done. https://codereview.chromium.org/2569703004/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_request_manager_browsertest.cc:264: // No default case, to pick up new enum values. On 2017/03/12 23:33:53, raymes wrote: > I'd actually suggest adding default: here because it's going to become > burdensome for people adding new types that don't have prompts. > > It may be better to switch on PermissionRequestType. That more closely aligns > with what can show up in a prompt. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dominickn@chromium.org changed reviewers: + dominickn@chromium.org
Request: can you add crbug.com/663935 to this CL? I think this definitely covers "test permissions UI on desktop" :)
Description was changed from ========== Add "interactive" UI integration tests for PermissionRequestManager. Existing PermissionRequestManager tests use a MockPermissionPromptFactory which replaces the code that generates actual UI with stubs. To get coverage of the UI creation code, and to allow the set of permission prompts to be easily "invoked" for UI review, extend PermissionRequestManagerBrowserTest with one that uses "real" UI rather than a mock. Then, add "Dialog" BrowserTests for a range of permission types that can be requested. These are run on the bots or (optionally) interactively with a command such as `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=PermissionDialogTest.InvokeDialog_multiple` BUG=605667 ========== to ========== Add "interactive" UI integration tests for PermissionRequestManager. Existing PermissionRequestManager tests use a MockPermissionPromptFactory which replaces the code that generates actual UI with stubs. To get coverage of the UI creation code, and to allow the set of permission prompts to be easily "invoked" for UI review, extend PermissionRequestManagerBrowserTest with one that uses "real" UI rather than a mock. Then, add "Dialog" BrowserTests for a range of permission types that can be requested. These are run on the bots or (optionally) interactively with a command such as `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=PermissionDialogTest.InvokeDialog_multiple` BUG=605667, 663935 ==========
On 2017/03/14 05:46:58, dominickn wrote: > Request: can you add crbug.com/663935 to this CL? I think this definitely covers > "test permissions UI on desktop" :) Done.
lgtm, thanks Trent! Sorry for the delay
The CQ bit was checked by tapted@chromium.org
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": 120001, "attempt_start_ts": 1489623013124350,
"parent_rev": "8fdeee30d2eaf99de96e21785b564884f91000cb", "commit_rev":
"38424d53080c397f8cc13c6581fe937070922ac0"}
Message was sent while issue was closed.
Description was changed from ========== Add "interactive" UI integration tests for PermissionRequestManager. Existing PermissionRequestManager tests use a MockPermissionPromptFactory which replaces the code that generates actual UI with stubs. To get coverage of the UI creation code, and to allow the set of permission prompts to be easily "invoked" for UI review, extend PermissionRequestManagerBrowserTest with one that uses "real" UI rather than a mock. Then, add "Dialog" BrowserTests for a range of permission types that can be requested. These are run on the bots or (optionally) interactively with a command such as `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=PermissionDialogTest.InvokeDialog_multiple` BUG=605667, 663935 ========== to ========== Add "interactive" UI integration tests for PermissionRequestManager. Existing PermissionRequestManager tests use a MockPermissionPromptFactory which replaces the code that generates actual UI with stubs. To get coverage of the UI creation code, and to allow the set of permission prompts to be easily "invoked" for UI review, extend PermissionRequestManagerBrowserTest with one that uses "real" UI rather than a mock. Then, add "Dialog" BrowserTests for a range of permission types that can be requested. These are run on the bots or (optionally) interactively with a command such as `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=PermissionDialogTest.InvokeDialog_multiple` BUG=605667, 663935 Review-Url: https://codereview.chromium.org/2569703004 Cr-Commit-Position: refs/heads/master@{#457296} Committed: https://chromium.googlesource.com/chromium/src/+/38424d53080c397f8cc13c6581fe... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/38424d53080c397f8cc13c6581fe... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
