Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1154943008:
Update push messaging tests to use both infobars and bubbles (w/ autoresponse) (Closed)
|
Created:
5 years, 6 months ago by felt Modified:
5 years, 6 months ago CC:
chromium-reviews, mvanouwerkerk+watch_chromium.org, peter+watch_chromium.org, johnme+watch_chromium.org, markusheintz_ Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate push messaging tests to use both infobars and bubbles (w/ autoresponse)
- This CL updates the Permission Bubble Manager to autorespond for browser tests.
The test tells the Permission Bubble Manager which response is desired, and the
Permission Bubble Manager will execute that response immediately after Show().
- This CL applies the new responder to parameterized PushMessagingBrowsertests.
BUG=438758
Committed: https://crrev.com/5a86ada324212abe358ac7da01a66ce08b3f62b7
Cr-Commit-Position: refs/heads/master@{#333166}
Patch Set 1 : All push tests passing locally #Patch Set 2 : Rebased #Patch Set 3 : Helper methods should use EXPECT instead of ASSERT #Patch Set 4 : Completely remove infobars #
Total comments: 4
Patch Set 5 : Factor out the GetPermissionBubbleManager calls #
Total comments: 7
Patch Set 6 : Put the auto-responder directly into the PermissionBubbleManager #
Total comments: 20
Patch Set 7 : Comments for msw #
Total comments: 2
Patch Set 8 : Fix subscribe test #Patch Set 9 : Paramaterize everything f'real #
Total comments: 4
Patch Set 10 : Comments from msw #
Messages
Total messages: 28 (10 generated)
felt@chromium.org changed reviewers: + johnme@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #2 (id:2) has been deleted
Hi John, I rewrote https://codereview.chromium.org/1153193013/ to instead create a new responder. It is much cleaner like you suggested. Can you please review the push messaging browsertest changes? Only a few of your remarks from the prior CL are still relevant due to the large shift in direction. Here are those relevant comments, with my responses inline: > Since these tests only run on desktop (unfortunately, chrome_browsertests isn't supported on Android), perhaps we don't need the parametrization, and could always test with bubbles? Or do you think it's important to maintain test coverage for infobars on desktop? I personally don't think we need both, but I thought test owners would prefer the parameterized version. Since it seems you don't, I've removed the parameterization. > Last I checked, you couldn't use ASSERT_TRUE in helper methods, as the ASSERT isn't able to terminate execution from a helper method. Should be fine to use EXPECT_TRUE instead. Ditto above. Done.
On 2015/06/03 02:00:09, felt wrote: > Hi John, > > I rewrote https://codereview.chromium.org/1153193013/ to instead create a new > responder. It is much cleaner like you suggested. Can you please review the push > messaging browsertest changes? > > Only a few of your remarks from the prior CL are still relevant due to the large > shift in direction. Here are those relevant comments, with my responses inline: > > > Since these tests only run on desktop (unfortunately, chrome_browsertests > isn't supported on Android), perhaps we don't need the parametrization, and > could always test with bubbles? Or do you think it's important to maintain test > coverage for infobars on desktop? > > I personally don't think we need both, but I thought test owners would prefer > the parameterized version. Since it seems you don't, I've removed the > parameterization. Ah! I just realized I may have misunderstood this. Do you mean that these tests are not run on Android at all? (There are completely separate Android tests?) If that's the case, I will completely delete the infobar references. > > Last I checked, you couldn't use ASSERT_TRUE in helper methods, as the ASSERT > isn't able to terminate execution from a helper method. Should be fine to use > EXPECT_TRUE instead. Ditto above. > > Done.
felt@chromium.org changed reviewers: + markusheintz@chromium.org
Markus, PTAL at the website_settings/ changes? The new responder should simplify and de-flake all of the permission bubble browser tests. I've demonstrated how with the push notification tests. I plan to update all of the other API tests to use the responder.
Thanks, this is wonderfully legible now! lgtm with one bug. https://codereview.chromium.org/1154943008/diff/80001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1154943008/diff/80001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_browsertest.cc:81: command_line->AppendSwitch(switches::kEnablePermissionsBubbles); Nit: Isn't this the default now? https://codereview.chromium.org/1154943008/diff/80001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_browsertest.cc:300: RequestAndAcceptPermission(); Actually, I think the point of this test is that if you haven't already called requestNotificationPermission, then calling subscribePush is expected to show the bubble instead. So you probably just want the following here: EXPECT_TRUE(PermissionBubbleManager::Enabled()); PermissionBubbleManager* bubble_manager = PermissionBubbleManager::FromWebContents( GetBrowser()->tab_strip_model()->GetActiveWebContents()); scoped_ptr<PermissionBubbleResponder> bubble_accept_responder( new PermissionBubbleResponder( bubble_manager, PermissionBubbleResponder::ACCEPT_ALL)); And since there'll now 3 callsites that check and get the PermissionBubbleManager, it may also make sense to factor out the first two statements into a helper function?
https://codereview.chromium.org/1154943008/diff/80001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1154943008/diff/80001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_browsertest.cc:81: command_line->AppendSwitch(switches::kEnablePermissionsBubbles); On 2015/06/03 14:14:35, johnme wrote: > Nit: Isn't this the default now? I've been going back and forth with some test failures, which I am trying to fix this week so it can stay permanently on. I want to make sure the push tests always run with it on, moving forwards. https://codereview.chromium.org/1154943008/diff/80001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_browsertest.cc:300: RequestAndAcceptPermission(); On 2015/06/03 14:14:35, johnme wrote: > Actually, I think the point of this test is that if you haven't already called > requestNotificationPermission, then calling subscribePush is expected to show > the bubble instead. > > So you probably just want the following here: > > EXPECT_TRUE(PermissionBubbleManager::Enabled()); > PermissionBubbleManager* bubble_manager = > PermissionBubbleManager::FromWebContents( > GetBrowser()->tab_strip_model()->GetActiveWebContents()); > scoped_ptr<PermissionBubbleResponder> bubble_accept_responder( > new PermissionBubbleResponder( > bubble_manager, > PermissionBubbleResponder::ACCEPT_ALL)); Ah, I missed the point of this test. Fixed. > And since there'll now 3 callsites that check and get the > PermissionBubbleManager, it may also make sense to factor out the first two > statements into a helper function? Done, added GetPermissionBubbleManager() and moved the EXPECT enabled check to SetUpCommandLine.
felt@chromium.org changed reviewers: + msw@chromium.org
msw@, would you be willing to review the files in chrome/browser/ui/website_settings/? Markus is OOO. (I am the other OWNER, which leaves me with no one else to ask for review.) Thanks! felt
https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:81: command_line->AppendSwitch(switches::kEnablePermissionsBubbles); Should we continue testing the old code path until it's removed or at least off-by-default? https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:228: scoped_ptr<PermissionBubbleResponder> bubble_accept_responder( nit: why scoped_ptr? ditto below. https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right): https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.cc:358: NotifyBubbleAdded(view_); Instead of adding the observer and responder classes, etc., just add a PermissionBubbleManager::set_auto_response_type_for_test(AutoResponseType response), then this code can Accept/Deny/Dismiss inline according to auto_response_type_for_test_, and the tests can just do GetPermissionBubbleManager()->set_auto_response_type_for_test(ACCEPT/DENY/DISMISS/NONE) as needed.
https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:81: command_line->AppendSwitch(switches::kEnablePermissionsBubbles); On 2015/06/04 01:22:08, msw wrote: > Should we continue testing the old code path until it's removed or at least > off-by-default? IMO there isn't a lot of value in testing the old infobar path because the odds of it being used for 45 onwards is close to 0. My expectation is that the old code path will be gone completely next week, so it isn't really worth the added complexity of parameterization. I've set a calendar reminder for myself for two weeks from now so that if the old code path isn't gone by then, I'll come back and parameterize this test. https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right): https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.cc:358: NotifyBubbleAdded(view_); On 2015/06/04 01:22:08, msw wrote: > Instead of adding the observer and responder classes, etc., just add a > PermissionBubbleManager::set_auto_response_type_for_test(AutoResponseType > response), then this code can Accept/Deny/Dismiss inline according to > auto_response_type_for_test_, and the tests can just do > GetPermissionBubbleManager()->set_auto_response_type_for_test(ACCEPT/DENY/DISMISS/NONE) > as needed. There is one disadvantage of the set_auto_response_type_for_test approach: now every new browser test will need to be friend classed, which is a bit ungainly and encourages calling more private methods in tests. However, you are right that it is much simpler. I added the set_auto_response_type_for_test approach as a new patchset, please take a look.
https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:81: command_line->AppendSwitch(switches::kEnablePermissionsBubbles); On 2015/06/05 00:38:21, felt wrote: > On 2015/06/04 01:22:08, msw wrote: > > Should we continue testing the old code path until it's removed or at least > > off-by-default? > > IMO there isn't a lot of value in testing the old infobar path because the odds > of it being used for 45 onwards is close to 0. My expectation is that the old > code path will be gone completely next week, so it isn't really worth the added > complexity of parameterization. I've set a calendar reminder for myself for two > weeks from now so that if the old code path isn't gone by then, I'll come back > and parameterize this test. Hmm, I worry about dropping testing for an on-by-default codepath. Can we delay landing this until the switch occurs (sounds quite soon)? https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:87: void SetUp() override { Make SetUp and TearDown clear the auto-response (so tests don't leak state). https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:226: GetPermissionBubbleManager()->set_auto_response_for_testing_only_( optional nit: just inline this (if you make the setter public) https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right): https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.cc:425: auto_response_for_testing_only_ = response; nit: inline the body for a simple setter with a unix_hacker name. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.cc:431: return; nit: maybe NOTREACHED() here if you check outside the function. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/permission_bubble_manager.h (right): https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.h:11: #include "base/observer_list.h" Remove this https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.h:34: class Observer { Remove this. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.h:82: friend class PushMessagingBrowserTest; Avoid friending by making the test setter public. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.h:85: NOT_TESTING, nit: maybe rename |NONE| (tests might want to clear the auto response too) https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.h:141: void set_auto_response_for_testing_only_(AutoResponseType response); Name this |set_auto_response_for_test|. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.h:165: AutoResponseType auto_response_for_testing_only_; Name this |auto_response_for_test_|
https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1154943008/diff/100001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:81: command_line->AppendSwitch(switches::kEnablePermissionsBubbles); On 2015/06/05 01:51:59, msw wrote: > On 2015/06/05 00:38:21, felt wrote: > > On 2015/06/04 01:22:08, msw wrote: > > > Should we continue testing the old code path until it's removed or at least > > > off-by-default? > > > > IMO there isn't a lot of value in testing the old infobar path because the > odds > > of it being used for 45 onwards is close to 0. My expectation is that the old > > code path will be gone completely next week, so it isn't really worth the > added > > complexity of parameterization. I've set a calendar reminder for myself for > two > > weeks from now so that if the old code path isn't gone by then, I'll come back > > and parameterize this test. > > Hmm, I worry about dropping testing for an on-by-default codepath. > Can we delay landing this until the switch occurs (sounds quite soon)? OK, so I've gone back to my original parameterized version. I will drop the parameters in a week or two instead. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:87: void SetUp() override { On 2015/06/05 01:51:59, msw wrote: > Make SetUp and TearDown clear the auto-response (so tests don't leak state). That's unnecessary here. The PermissionBubbleManager is tied to each WebContents, and each browser test will get a clean WebContents. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:226: GetPermissionBubbleManager()->set_auto_response_for_testing_only_( On 2015/06/05 01:51:59, msw wrote: > optional nit: just inline this (if you make the setter public) Done. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right): https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.cc:425: auto_response_for_testing_only_ = response; On 2015/06/05 01:51:59, msw wrote: > nit: inline the body for a simple setter with a unix_hacker name. Done. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.cc:431: return; On 2015/06/05 01:51:59, msw wrote: > nit: maybe NOTREACHED() here if you check outside the function. Done. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/permission_bubble_manager.h (right): https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.h:11: #include "base/observer_list.h" On 2015/06/05 01:52:00, msw wrote: > Remove this Done. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.h:34: class Observer { On 2015/06/05 01:52:00, msw wrote: > Remove this. Done. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.h:82: friend class PushMessagingBrowserTest; On 2015/06/05 01:52:00, msw wrote: > Avoid friending by making the test setter public. This makes me a little anxious... but I guess my comment ought to serve as an adequate deterrent. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.h:85: NOT_TESTING, On 2015/06/05 01:52:00, msw wrote: > nit: maybe rename |NONE| (tests might want to clear the auto response too) Done. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.h:141: void set_auto_response_for_testing_only_(AutoResponseType response); On 2015/06/05 01:52:00, msw wrote: > Name this |set_auto_response_for_test|. Done. https://codereview.chromium.org/1154943008/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/permission_bubble_manager.h:165: AutoResponseType auto_response_for_testing_only_; On 2015/06/05 01:52:00, msw wrote: > Name this |auto_response_for_test_| Done.
https://codereview.chromium.org/1154943008/diff/140001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1154943008/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:319: GetPermissionBubbleManager()->set_auto_response_for_test( Since we've gone back to parameterizing the tests, I guess this should still use an InfoBarResponder if !PermissionBubbleManager::Enabled().
https://codereview.chromium.org/1154943008/diff/140001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1154943008/diff/140001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:319: GetPermissionBubbleManager()->set_auto_response_for_test( On 2015/06/05 10:07:06, johnme wrote: > Since we've gone back to parameterizing the tests, I guess this should still use > an InfoBarResponder if !PermissionBubbleManager::Enabled(). Yup fixed this, didn't see the test failures until the end.
lgtm with nits, thanks for addressing my comments. https://codereview.chromium.org/1154943008/diff/180001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1154943008/diff/180001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:245: EXPECT_EQ("permission status - granted", script_result); nit: consolidate |script_result| checks after the conditionals. (ditto below) https://codereview.chromium.org/1154943008/diff/180001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:323: EXPECT_EQ(GetEndpointForSubscriptionId("1-0"), nit: ditto
Oh, please update the CL title/desc too.
Updated title/desc as well. https://codereview.chromium.org/1154943008/diff/180001/chrome/browser/push_me... File chrome/browser/push_messaging/push_messaging_browsertest.cc (right): https://codereview.chromium.org/1154943008/diff/180001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:245: EXPECT_EQ("permission status - granted", script_result); On 2015/06/05 16:56:26, msw wrote: > nit: consolidate |script_result| checks after the conditionals. (ditto below) Done. https://codereview.chromium.org/1154943008/diff/180001/chrome/browser/push_me... chrome/browser/push_messaging/push_messaging_browsertest.cc:323: EXPECT_EQ(GetEndpointForSubscriptionId("1-0"), On 2015/06/05 16:56:26, msw wrote: > nit: ditto Done.
The CQ bit was checked by felt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org Link to the patchset: https://codereview.chromium.org/1154943008/#ps180001 (title: "Paramaterize everything f'real")
The CQ bit was unchecked by felt@chromium.org
The CQ bit was checked by felt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1154943008/#ps200001 (title: "Comments from msw")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1154943008/200001
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/5a86ada324212abe358ac7da01a66ce08b3f62b7 Cr-Commit-Position: refs/heads/master@{#333166} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
