Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2138873002:
Add tests for the Display operation of the mac notification bridge. (Closed)
|
Created:
4 years, 5 months ago by Miguel Garcia Modified:
4 years, 2 months ago CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, shrike Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd tests for the Display operation of the mac notification bridge.
Refactor the ability to create test notification objects so it can be
shared across tests.
BUG=571056
Committed: https://crrev.com/dbb2e2330b0455ce9256bc13e61c7ffc63c58ee7
Cr-Commit-Position: refs/heads/master@{#425403}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 15
Patch Set 3 : review #Patch Set 4 : revert files #
Total comments: 14
Patch Set 5 : review #Patch Set 6 : removed OCMOck deps #Patch Set 7 : Switch one test to ScopedObjCClassSwizzler #Patch Set 8 : scoped swizzle #
Total comments: 10
Patch Set 9 : review #Patch Set 10 : format #
Messages
Total messages: 43 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
miguelg@chromium.org changed reviewers: + peter@chromium.org, rsesek@chromium.org
Contains an important hack to avoid touching NSUserNotoficationCenter properties during tests. I have started an offline thread to resolve this.
https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:95: NotificationPlatformBridgeMac(notification_center, true /* set_delegate */); You can define a delegating constructor like: NotificationPlatformBridgeMac::NotificationPlatformBridgeMac( NSUserNotificationCenter* notification_center) : NotificationPlatformBridgeMac(notification_center, true /* set_delegate */) {} That said, the number of callers for NotificationPlatformBridgeMac is sufficiently low that it'd be even better to drop the second constructor altogether, and just always pass the boolean. https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:103: // Setting up the delegte upsets OCMock. delegte -> delegate Rather than saying that it "upsets OCMock", it would be more valuable to describe why that is an issue. https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:105: notification_center.delegate = delegate_.get(); Why did the syntax for the assignment change? https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (left): https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:47: TEST(NotificationPlatformBridgeMacTest, TestNotificationUnknownType) { Why did this test go away? https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:52: void TearDown() override { CocoaTest::TearDown(); } Please define this as: class NotificationPlatformBridgeMacTest : public CocoaTest {}; (You can drop the rest.) https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:120: "Title", "Context", "https://gmail.com", "", "", I'm don't think I'm in favor of this generalization.. (a) It exposes a very random subset of notification functionality. The Notification object already has a very.. capable constructor for most these settings, and setters for the rest. (b) The unnamed string arguments are very unclear at the call-site. (c) It creates yet another way to create notifications, which old code is unlikely to ever get updated to. (d) We know we're going to switch to Mojo eventually, in which case a builder of sorts for tests might be useful, but that's unlikely to look like this.
https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:120: "Title", "Context", "https://gmail.com", "", "", What do you suggest? I can't really use the constructor directly since it needs to compose all the optional args (which are not that optional). I can duplicate code as well if you prefer but other than that I am open to suggestions on what to do to pass notification objects to the tests. On 2016/07/11 17:25:48, Peter Beverloo wrote: > I'm don't think I'm in favor of this generalization.. > > (a) It exposes a very random subset of notification functionality. The > Notification object already has a very.. capable constructor for most these > settings, and setters for the rest. > > (b) The unnamed string arguments are very unclear at the call-site. > > (c) It creates yet another way to create notifications, which old code is > unlikely to ever get updated to. > > (d) We know we're going to switch to Mojo eventually, in which case a builder > of sorts for tests might be useful, but that's unlikely to look like this.
https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:95: NotificationPlatformBridgeMac(notification_center, true /* set_delegate */); You know I usually go with the flow with regards to following review comments but in this case I am going to insist on keep the hack private at least :) Now that we know why it is needed I think I will remove it the minute the dependent roll happens. So for now I switched to delegating constructor. On 2016/07/11 17:25:48, Peter Beverloo wrote: > You can define a delegating constructor like: > > NotificationPlatformBridgeMac::NotificationPlatformBridgeMac( > NSUserNotificationCenter* notification_center) > : NotificationPlatformBridgeMac(notification_center, > true /* set_delegate */) {} > > That said, the number of callers for NotificationPlatformBridgeMac is > sufficiently low that it'd be even better to drop the second constructor > altogether, and just always pass the boolean. https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:103: // Setting up the delegte upsets OCMock. On 2016/07/11 17:25:48, Peter Beverloo wrote: > delegte -> delegate > > Rather than saying that it "upsets OCMock", it would be more valuable to > describe why that is an issue. Now that we know why sure :) https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:105: notification_center.delegate = delegate_.get(); On 2016/07/11 17:25:48, Peter Beverloo wrote: > Why did the syntax for the assignment change? It was me trying some things to keep OCMock happy, will revert. https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (left): https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:47: TEST(NotificationPlatformBridgeMacTest, TestNotificationUnknownType) { On 2016/07/11 17:25:48, Peter Beverloo wrote: > Why did this test go away? I think it was a merge issue. Brought it back https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:52: void TearDown() override { CocoaTest::TearDown(); } On 2016/07/11 17:25:48, Peter Beverloo wrote: > Please define this as: > > class NotificationPlatformBridgeMacTest : public CocoaTest {}; > > (You can drop the rest.) Really? Don't we have to call CocoaTest::SetUp and so? https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:120: "Title", "Context", "https://gmail.com", "", "", Discussed offline, added its own CreateNotification method in this test instead of refactoring the one in notification conversions. On 2016/07/12 11:06:59, Miguel Garcia wrote: > What do you suggest? I can't really use the constructor directly since it needs > to compose all the optional args (which are not that optional). I can duplicate > code as well if you prefer but other than that I am open to suggestions on what > to do to pass notification objects to the tests. > > > On 2016/07/11 17:25:48, Peter Beverloo wrote: > > I'm don't think I'm in favor of this generalization.. > > > > (a) It exposes a very random subset of notification functionality. The > > Notification object already has a very.. capable constructor for most these > > settings, and setters for the rest. > > > > (b) The unnamed string arguments are very unclear at the call-site. > > > > (c) It creates yet another way to create notifications, which old code is > > unlikely to ever get updated to. > > > > (d) We know we're going to switch to Mojo eventually, in which case a > builder > > of sorts for tests might be useful, but that's unlikely to look like this. >
miguelg@chromium.org changed reviewers: + thakis@chromium.org
+Nico for the DEPS file.
https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac.h:50: FRIEND_TEST_ALL_PREFIXES(NotificationPlatformBridgeMacTest, Since this is just to gain access to the private ctor, what I like to do is to friend class NotificationConversionHelperTest. Then have a helper method on that friended class, so you can avoid the FRIEND_TEST macros (for when more tests are added). But since this is all a hack around the OCMock roll, this is optional. https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:156: id notificationCenter = naming: under_scores (maybe also just create the NotificationPlatformBridgeMac in the test fixture to avoid repeated boilerplate).
lgtm https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:52: void TearDown() override { CocoaTest::TearDown(); } On 2016/07/12 16:16:24, Miguel Garcia wrote: > On 2016/07/11 17:25:48, Peter Beverloo wrote: > > Please define this as: > > > > class NotificationPlatformBridgeMacTest : public CocoaTest {}; > > > > (You can drop the rest.) > > Really? Don't we have to call CocoaTest::SetUp and so? CocoaTest already defines them, which you inherit from. Since they're virtual functions, the vtable will make sure that the calls reach the right place. https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:55: const std::string& button2) { All of these arguments could be defined as "const char*" to avoid the allocations. base::UTF8ToUTF16() takes a StringPiece argument, which can implicitly be constructed from that. https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:72: gfx::Image icon = gfx::Image::CreateFrom1xBitmap(bitmap); fwiw, using an empty gfx::Image() as the icon is perfectly fine. You don't seem to be using the pixel anywhere? https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:77: message_center::NotifierId(message_center::NotifierId::APPLICATION, optional: APPLICATION -> WEB_PAGE? :)
https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/DEPS#ne... chrome/browser/DEPS:91: "+third_party/ocmock/OCMock/OCMock.h", why is this needed? we already have many files in c/b that use ocmock: https://cs.chromium.org/search/?q=ocmock.h+file:chrome&sq=package:chromium&ty... As far as I can tell, they don't allow this in subdirectory DEPS files. https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac.mm:96: true /* set_delegate */) {} please use /*set_delegate=*/true (the spelling you have is ambiguous for false parameters: Does `false /*do_it*/` mean that do_it is set to false and that it's not done, or does it mean that the flag is dont_do_it and since it's false we're about to do_it)?
https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:52: void TearDown() override { CocoaTest::TearDown(); } On 2016/07/12 17:44:19, Peter Beverloo wrote: > On 2016/07/12 16:16:24, Miguel Garcia wrote: > > On 2016/07/11 17:25:48, Peter Beverloo wrote: > > > Please define this as: > > > > > > class NotificationPlatformBridgeMacTest : public CocoaTest {}; > > > > > > (You can drop the rest.) > > > > Really? Don't we have to call CocoaTest::SetUp and so? > > CocoaTest already defines them, which you inherit from. Since they're virtual > functions, the vtable will make sure that the calls reach the right place. Duh, I guess I deserve the lecture :) I misread it as doing some static initialization of AppKit instead of just calling the superclass method. https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/DEPS#ne... chrome/browser/DEPS:91: "+third_party/ocmock/OCMock/OCMock.h", On 2016/07/12 17:54:38, Nico wrote: > why is this needed? we already have many files in c/b that use ocmock: > https://cs.chromium.org/search/?q=ocmock.h+file:chrome&sq=package:chromium&ty... > As far as I can tell, they don't allow this in subdirectory DEPS files. I looked a some of those results and all of them had a DEPS rule somewhere in their path to allow OCMock explicitly. https://cs.chromium.org/chromium/src/chrome/browser/renderer_host/DEPS https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/DEPS https://cs.chromium.org/chromium/src/ios/DEPS Worth noting that if I revert this file I get a presubmit error as well Running presubmit commit checks ... ** Presubmit ERRORS ** You added one or more #includes that violate checkdeps rules. chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm Illegal include: "third_party/ocmock/OCMock/OCMock.h" Because of no rule applying. \ chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm Illegal include: "third_party/ocmock/gtest_support.h" Because of no rule applying. Would you prefer if I added a new DEPS file in chrome/browser/notifications ? https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac.h:50: FRIEND_TEST_ALL_PREFIXES(NotificationPlatformBridgeMacTest, On 2016/07/12 17:33:36, Robert Sesek wrote: > Since this is just to gain access to the private ctor, what I like to do is to > friend class NotificationConversionHelperTest. Then have a helper method on that > friended class, so you can avoid the FRIEND_TEST macros (for when more tests are > added). > > But since this is all a hack around the OCMock roll, this is optional. Acknowledged. https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac.mm:96: true /* set_delegate */) {} On 2016/07/12 17:54:38, Nico wrote: > please use /*set_delegate=*/true (the spelling you have is ambiguous for false > parameters: Does `false /*do_it*/` mean that do_it is set to false and that it's > not done, or does it mean that the flag is dont_do_it and since it's false we're > about to do_it)? Done. https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:55: const std::string& button2) { On 2016/07/12 17:44:19, Peter Beverloo wrote: > All of these arguments could be defined as "const char*" to avoid the > allocations. base::UTF8ToUTF16() takes a StringPiece argument, which can > implicitly be constructed from that. I changed but for the record, I would trade some extra allocations for not having to do things like if(strlen(button1))... instead of if (!button1.empty())... https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:72: gfx::Image icon = gfx::Image::CreateFrom1xBitmap(bitmap); On 2016/07/12 17:44:19, Peter Beverloo wrote: > fwiw, using an empty gfx::Image() as the icon is perfectly fine. You don't seem > to be using the pixel anywhere? Done. https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:77: message_center::NotifierId(message_center::NotifierId::APPLICATION, On 2016/07/12 17:44:19, Peter Beverloo wrote: > optional: APPLICATION -> WEB_PAGE? :) Done. https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:156: id notificationCenter = On 2016/07/12 17:33:36, Robert Sesek wrote: > naming: under_scores > > (maybe also just create the NotificationPlatformBridgeMac in the test fixture to > avoid repeated boilerplate). Yeah this one was a great suggestion since allowed me to remove all the friend declarations in the .h file.
On 2016/07/13 14:05:16, Miguel Garcia wrote: > https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... > File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm > (right): > > https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:52: > void TearDown() override { CocoaTest::TearDown(); } > On 2016/07/12 17:44:19, Peter Beverloo wrote: > > On 2016/07/12 16:16:24, Miguel Garcia wrote: > > > On 2016/07/11 17:25:48, Peter Beverloo wrote: > > > > Please define this as: > > > > > > > > class NotificationPlatformBridgeMacTest : public CocoaTest {}; > > > > > > > > (You can drop the rest.) > > > > > > Really? Don't we have to call CocoaTest::SetUp and so? > > > > CocoaTest already defines them, which you inherit from. Since they're virtual > > functions, the vtable will make sure that the calls reach the right place. > > Duh, I guess I deserve the lecture :) I misread it as doing some static > initialization of AppKit instead of just calling the superclass method. > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/DEPS > File chrome/browser/DEPS (right): > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/DEPS#ne... > chrome/browser/DEPS:91: "+third_party/ocmock/OCMock/OCMock.h", > On 2016/07/12 17:54:38, Nico wrote: > > why is this needed? we already have many files in c/b that use ocmock: > > > https://cs.chromium.org/search/?q=ocmock.h+file:chrome&sq=package:chromium&ty... > > As far as I can tell, they don't allow this in subdirectory DEPS files. > > I looked a some of those results and all of them had a DEPS rule somewhere in > their path to allow OCMock explicitly. > https://cs.chromium.org/chromium/src/chrome/browser/renderer_host/DEPS > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/DEPS > https://cs.chromium.org/chromium/src/ios/DEPS > > Worth noting that if I revert this file I get a presubmit error as well > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > You added one or more #includes that violate checkdeps rules. > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm > Illegal include: "third_party/ocmock/OCMock/OCMock.h" > Because of no rule applying. \ > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm > Illegal include: "third_party/ocmock/gtest_support.h" > Because of no rule applying. > > > Would you prefer if I added a new DEPS file in chrome/browser/notifications ? > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > File chrome/browser/notifications/notification_platform_bridge_mac.h (right): > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > chrome/browser/notifications/notification_platform_bridge_mac.h:50: > FRIEND_TEST_ALL_PREFIXES(NotificationPlatformBridgeMacTest, > On 2016/07/12 17:33:36, Robert Sesek wrote: > > Since this is just to gain access to the private ctor, what I like to do is to > > friend class NotificationConversionHelperTest. Then have a helper method on > that > > friended class, so you can avoid the FRIEND_TEST macros (for when more tests > are > > added). > > > > But since this is all a hack around the OCMock roll, this is optional. > > Acknowledged. > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > chrome/browser/notifications/notification_platform_bridge_mac.mm:96: true /* > set_delegate */) {} > On 2016/07/12 17:54:38, Nico wrote: > > please use /*set_delegate=*/true (the spelling you have is ambiguous for false > > parameters: Does `false /*do_it*/` mean that do_it is set to false and that > it's > > not done, or does it mean that the flag is dont_do_it and since it's false > we're > > about to do_it)? > > Done. > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm > (right): > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:55: > const std::string& button2) { > On 2016/07/12 17:44:19, Peter Beverloo wrote: > > All of these arguments could be defined as "const char*" to avoid the > > allocations. base::UTF8ToUTF16() takes a StringPiece argument, which can > > implicitly be constructed from that. > > I changed but for the record, I would trade some extra allocations for not > having to do things like if(strlen(button1))... instead of if > (!button1.empty())... > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:72: > gfx::Image icon = gfx::Image::CreateFrom1xBitmap(bitmap); > On 2016/07/12 17:44:19, Peter Beverloo wrote: > > fwiw, using an empty gfx::Image() as the icon is perfectly fine. You don't > seem > > to be using the pixel anywhere? > > Done. > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:77: > message_center::NotifierId(message_center::NotifierId::APPLICATION, > On 2016/07/12 17:44:19, Peter Beverloo wrote: > > optional: APPLICATION -> WEB_PAGE? :) > > Done. > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:156: > id notificationCenter = > On 2016/07/12 17:33:36, Robert Sesek wrote: > > naming: under_scores > > > > (maybe also just create the NotificationPlatformBridgeMac in the test fixture > to > > avoid repeated boilerplate). > > Yeah this one was a great suggestion since allowed me to remove all the friend > declarations in the .h file. Note that the functionality you desire from OCMock 3 (such as mocking dynamic properties) can almost certainly be achieved by some combination of subclassing and/or swizzling. e.g. https://cs.chromium.org/chromium/src/base/mac/scoped_objc_class_swizzler.h?q=...
Description was changed from ========== Add tests for the Display operation of the mac notification bridge. Refactor the ability to create test notification objects so it can be shared across tests. BUG=571056 ========== to ========== Add tests for the Display operation of the mac notification bridge. Refactor the ability to create test notification objects so it can be shared across tests. BUG=571056 ==========
The CQ bit was checked by miguelg@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...
Ok so I removed the OCMock dependency altogether and replaced it with a version of the test that swizzles the deliverNotification method from a real NSUserNotificationCenter Some issues I found while doing this: First, NSUserNotificationCenter does not produce real objects. When calling any of the constructors you end up getting objects of type _NSConcreteUserNotificationCenter instead The traditional way of instantiating a NSUserNotificationCenter ([NSUserNotificationCenter defaultNotificationCenter]) returns null in tests. I had to use the private constructor. Perhaps because of the two issues above I have been unable to use scoped_objc_class_swizzler.h I have had to do the swizzling myself. The tests now are simple and look great but I would still prefer to replace them with OCMock once it's available. That's also why I'm hoping you'll be ok with this :) On 2016/09/15 18:03:57, erikchen wrote: > On 2016/07/13 14:05:16, Miguel Garcia wrote: > > > https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... > > File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm > > (right): > > > > > https://codereview.chromium.org/2138873002/diff/60001/chrome/browser/notifica... > > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:52: > > void TearDown() override { CocoaTest::TearDown(); } > > On 2016/07/12 17:44:19, Peter Beverloo wrote: > > > On 2016/07/12 16:16:24, Miguel Garcia wrote: > > > > On 2016/07/11 17:25:48, Peter Beverloo wrote: > > > > > Please define this as: > > > > > > > > > > class NotificationPlatformBridgeMacTest : public CocoaTest {}; > > > > > > > > > > (You can drop the rest.) > > > > > > > > Really? Don't we have to call CocoaTest::SetUp and so? > > > > > > CocoaTest already defines them, which you inherit from. Since they're > virtual > > > functions, the vtable will make sure that the calls reach the right place. > > > > Duh, I guess I deserve the lecture :) I misread it as doing some static > > initialization of AppKit instead of just calling the superclass method. > > > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/DEPS > > File chrome/browser/DEPS (right): > > > > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/DEPS#ne... > > chrome/browser/DEPS:91: "+third_party/ocmock/OCMock/OCMock.h", > > On 2016/07/12 17:54:38, Nico wrote: > > > why is this needed? we already have many files in c/b that use ocmock: > > > > > > https://cs.chromium.org/search/?q=ocmock.h+file:chrome&sq=package:chromium&ty... > > > As far as I can tell, they don't allow this in subdirectory DEPS files. > > > > I looked a some of those results and all of them had a DEPS rule somewhere in > > their path to allow OCMock explicitly. > > https://cs.chromium.org/chromium/src/chrome/browser/renderer_host/DEPS > > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/DEPS > > https://cs.chromium.org/chromium/src/ios/DEPS > > > > Worth noting that if I revert this file I get a presubmit error as well > > > > Running presubmit commit checks ... > > > > ** Presubmit ERRORS ** > > You added one or more #includes that violate checkdeps rules. > > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm > > Illegal include: "third_party/ocmock/OCMock/OCMock.h" > > Because of no rule applying. \ > > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm > > Illegal include: "third_party/ocmock/gtest_support.h" > > Because of no rule applying. > > > > > > Would you prefer if I added a new DEPS file in chrome/browser/notifications ? > > > > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > > File chrome/browser/notifications/notification_platform_bridge_mac.h (right): > > > > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > > chrome/browser/notifications/notification_platform_bridge_mac.h:50: > > FRIEND_TEST_ALL_PREFIXES(NotificationPlatformBridgeMacTest, > > On 2016/07/12 17:33:36, Robert Sesek wrote: > > > Since this is just to gain access to the private ctor, what I like to do is > to > > > friend class NotificationConversionHelperTest. Then have a helper method on > > that > > > friended class, so you can avoid the FRIEND_TEST macros (for when more tests > > are > > > added). > > > > > > But since this is all a hack around the OCMock roll, this is optional. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > > File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): > > > > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > > chrome/browser/notifications/notification_platform_bridge_mac.mm:96: true /* > > set_delegate */) {} > > On 2016/07/12 17:54:38, Nico wrote: > > > please use /*set_delegate=*/true (the spelling you have is ambiguous for > false > > > parameters: Does `false /*do_it*/` mean that do_it is set to false and that > > it's > > > not done, or does it mean that the flag is dont_do_it and since it's false > > we're > > > about to do_it)? > > > > Done. > > > > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > > File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm > > (right): > > > > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:55: > > const std::string& button2) { > > On 2016/07/12 17:44:19, Peter Beverloo wrote: > > > All of these arguments could be defined as "const char*" to avoid the > > > allocations. base::UTF8ToUTF16() takes a StringPiece argument, which can > > > implicitly be constructed from that. > > > > I changed but for the record, I would trade some extra allocations for not > > having to do things like if(strlen(button1))... instead of if > > (!button1.empty())... > > > > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:72: > > gfx::Image icon = gfx::Image::CreateFrom1xBitmap(bitmap); > > On 2016/07/12 17:44:19, Peter Beverloo wrote: > > > fwiw, using an empty gfx::Image() as the icon is perfectly fine. You don't > > seem > > > to be using the pixel anywhere? > > > > Done. > > > > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:77: > > message_center::NotifierId(message_center::NotifierId::APPLICATION, > > On 2016/07/12 17:44:19, Peter Beverloo wrote: > > > optional: APPLICATION -> WEB_PAGE? :) > > > > Done. > > > > > https://codereview.chromium.org/2138873002/diff/100001/chrome/browser/notific... > > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:156: > > id notificationCenter = > > On 2016/07/12 17:33:36, Robert Sesek wrote: > > > naming: under_scores > > > > > > (maybe also just create the NotificationPlatformBridgeMac in the test > fixture > > to > > > avoid repeated boilerplate). > > > > Yeah this one was a great suggestion since allowed me to remove all the friend > > declarations in the .h file. > > Note that the functionality you desire from OCMock 3 (such as mocking dynamic > properties) can almost certainly be achieved by some combination of subclassing > and/or swizzling. e.g. > https://cs.chromium.org/chromium/src/base/mac/scoped_objc_class_swizzler.h?q=...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/30 15:02:43, Miguel Garcia wrote: > Ok so I removed the OCMock dependency altogether and replaced it with a version > of the test that swizzles the deliverNotification method from a real > NSUserNotificationCenter > > Some issues I found while doing this: > > First, NSUserNotificationCenter does not produce real objects. When calling > any of the constructors you end up getting objects of type > _NSConcreteUserNotificationCenter instead > The traditional way of instantiating a NSUserNotificationCenter > ([NSUserNotificationCenter defaultNotificationCenter]) returns null in tests. I > had to use the private constructor. > > Perhaps because of the two issues above I have been unable to use > scoped_objc_class_swizzler.h I have had to do the swizzling myself. Why can't you use scoped_objc_class_swizzler.h for the private initializer?
On 2016/10/05 20:28:40, Robert Sesek wrote: > On 2016/09/30 15:02:43, Miguel Garcia wrote: > > Ok so I removed the OCMock dependency altogether and replaced it with a > version > > of the test that swizzles the deliverNotification method from a real > > NSUserNotificationCenter > > > > Some issues I found while doing this: > > > > First, NSUserNotificationCenter does not produce real objects. When calling > > any of the constructors you end up getting objects of type > > _NSConcreteUserNotificationCenter instead > > The traditional way of instantiating a NSUserNotificationCenter > > ([NSUserNotificationCenter defaultNotificationCenter]) returns null in tests. > I > > had to use the private constructor. > > > > Perhaps because of the two issues above I have been unable to use > > scoped_objc_class_swizzler.h I have had to do the swizzling myself. > > Why can't you use scoped_objc_class_swizzler.h for the private initializer? I uploaded a version where I switch to scoped_objc_class_swizzler in one of the two tests to illustrate what I have tried. It just does not seem to be swizzling it. https://codereview.chromium.org/2138873002/diff2/140001:160001/chrome/browser... shows the diff
Can you explain the problem you're seeing? I patched this CL in and was able to see the swizzled methods getting called.
On 2016/10/11 20:29:45, Robert Sesek wrote: > Can you explain the problem you're seeing? I patched this CL in and was able to > see the swizzled methods getting called. Ok I figured out why it was not working, check the last diff if you are curious (but don't judge me for the rookie mistake :) ). 100% working using scoped_objc_class_swizzler with a category for the NSUserNotificationCenter that contains the methods with the different expectations. PTAL
Glad you figured out the problem! https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:4: #import <objc/runtime.h> nit: blank line before, and keep AppKit.h https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:17: #include "third_party/ocmock/gtest_support.h" Remove for now. https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:58: if (strlen(button1)) { Why not just use nullptrs instead of strlen? https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:67: NotificationDelegate* delegate(new MockNotificationDelegate("id1")); Who owns this? Should this be a scoped_refptr<>? https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:167: ASSERT_TRUE([[notification title] isEqualToString:@"Title"]); Use EXPECT_NSEQ if you want a little bit better failure output.
https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:17: #include "third_party/ocmock/gtest_support.h" On 2016/10/13 16:47:32, Robert Sesek wrote: > Remove for now. Done. https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:58: if (strlen(button1)) { On 2016/10/13 16:47:32, Robert Sesek wrote: > Why not just use nullptrs instead of strlen? Changed to nullptrs, I kind of prefer empty strings for no buttons but don't feel strongly about it. https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:67: NotificationDelegate* delegate(new MockNotificationDelegate("id1")); On 2016/10/13 16:47:32, Robert Sesek wrote: > Who owns this? Should this be a scoped_refptr<>? The notification object owns it though a scoped_refptr. I can inline it to make it clearer https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:167: ASSERT_TRUE([[notification title] isEqualToString:@"Title"]); On 2016/10/13 16:47:32, Robert Sesek wrote: > Use EXPECT_NSEQ if you want a little bit better failure output. Done
LGTM https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:67: NotificationDelegate* delegate(new MockNotificationDelegate("id1")); On 2016/10/14 10:58:13, Miguel Garcia wrote: > On 2016/10/13 16:47:32, Robert Sesek wrote: > > Who owns this? Should this be a scoped_refptr<>? > > The notification object owns it though a scoped_refptr. I can inline it to make > it clearer I think it should just be wrapped in a scoped_refptr<> when being created, to make this explicit.
On 2016/10/14 14:21:26, Robert Sesek wrote: > LGTM > > https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... > File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm > (right): > > https://codereview.chromium.org/2138873002/diff/180001/chrome/browser/notific... > chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:67: > NotificationDelegate* delegate(new MockNotificationDelegate("id1")); > On 2016/10/14 10:58:13, Miguel Garcia wrote: > > On 2016/10/13 16:47:32, Robert Sesek wrote: > > > Who owns this? Should this be a scoped_refptr<>? > > > > The notification object owns it though a scoped_refptr. I can inline it to > make > > it clearer > > I think it should just be wrapped in a scoped_refptr<> when being created, to > make this explicit. But the notification constructor expects a raw pointer so it feels a bit boiler plate than simply inline it? That seems what all others usages do https://cs.chromium.org/search/?q=%22new+MockNotificationDelegate%22&sq=packa... I went ahead and inlined it but if you feel strongly I'll change it back.
The CQ bit was checked by miguelg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2138873002/#ps220001 (title: "format")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
miguelg@chromium.org changed reviewers: + jochen@chromium.org - thakis@chromium.org
+jochen for the chrome/browser/DEPS review
thakis@chromium.org changed reviewers: + thakis@chromium.org
chrome/browser/DEPS lgtm
The CQ bit was checked by thakis@chromium.org
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.
Description was changed from ========== Add tests for the Display operation of the mac notification bridge. Refactor the ability to create test notification objects so it can be shared across tests. BUG=571056 ========== to ========== Add tests for the Display operation of the mac notification bridge. Refactor the ability to create test notification objects so it can be shared across tests. BUG=571056 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add tests for the Display operation of the mac notification bridge. Refactor the ability to create test notification objects so it can be shared across tests. BUG=571056 ========== to ========== Add tests for the Display operation of the mac notification bridge. Refactor the ability to create test notification objects so it can be shared across tests. BUG=571056 Committed: https://crrev.com/dbb2e2330b0455ce9256bc13e61c7ffc63c58ee7 Cr-Commit-Position: refs/heads/master@{#425403} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/dbb2e2330b0455ce9256bc13e61c7ffc63c58ee7 Cr-Commit-Position: refs/heads/master@{#425403} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
