Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1122403009:
Add tests for PushMessagingAppIdentifier (Closed)
|
Created:
5 years, 7 months ago by johnme Modified:
5 years, 7 months ago Reviewers:
Peter Beverloo CC:
chromium-reviews, mvanouwerkerk+watch_chromium.org, peter+watch_chromium.org, johnme+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@ident_refactor Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd tests for PushMessagingAppIdentifier
This adds test coverage for PersistToPrefs, DeleteFromPrefs, GetAll, the
two Get methods, RegisterProfilePrefs, as well as some additional
coverage for Generate (the only part that had testing before).
BUG=458592
Committed: https://crrev.com/2f8f7b1b9ebf668d428412e334d69f48af1f8afa
Cr-Commit-Position: refs/heads/master@{#329642}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addressed peter's comments #
Total comments: 6
Patch Set 3 : Address peter's final nits #Messages
Total messages: 14 (6 generated)
johnme@chromium.org changed reviewers: + peter@chromium.org
lgtm! Thank you for adding these! The major comment I have is that I'd like to see these split up in a few smaller, isolated test-cases. Unit tests are very cheap, the overhead is setting them up is low, and it has the benefit that we'd get a much better idea of which part of the functionality is failing when that occurs. In order to achieve this without too much overhead, I also suggest just setting up threads in the test fixture. (Maybe also consider doing this for the profile?) https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc (right): https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:50: void ExpectEq(const PushMessagingAppIdentifier& a, I'd slightly prefer to define an (anonymous) equality operator for PMAI: bool operator==(const PushMessagingAppIdentifier& a, const PushMessagingAppIdentifier& b) {} While this wouldn't tell us exactly which two members aren't equal to each other, that's OK (and is in scope of reasonable debugging of one who made the tests fail). The benefit of this is that we can just use EXPECT_EQ(a, b); Finally, please have anonymous namespaces at the top of the file. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:62: content::TestBrowserThread ui_thread(BrowserThread::UI, &message_loop); Since we've already got the PushMessagingAppIdentifierTest base-class in either case, I'd be OK with just having a |content::TestBrowserThreadBundle| member that does this for you. Overhead for the two unit tests that don't need it is minimal. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:72: // Test basic PersistToPrefs round trips. nit: This can be a separate test. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:88: // Test that PersistToPrefs overwrites (when same origin and Service Worker). nit: This can be a separate test. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:115: // Test that PersistToPrefs doesn't overwrite (when different origin or SW). nit: This can be a separate test. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:138: // Test DeleteFromPrefs. Deleted app identifier should be deleted. nit: This can be a separate test. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:152: // Test GetAll. Non-deleted app identifiers should all be listed. nit: This can be a separate test.
Thanks for the review! Addressed comments. > The major comment I have is that I'd like to see these split up in a few > smaller, isolated test-cases. Unit tests are very cheap, the overhead is > setting them up is low, and it has the benefit that we'd get a much better > idea of which part of the functionality is failing when that occurs. > > In order to achieve this without too much overhead, I also suggest just > setting up threads in the test fixture. (Maybe also consider doing this for > the profile?) Done. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... File chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc (right): https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:50: void ExpectEq(const PushMessagingAppIdentifier& a, On 2015/05/12 13:55:40, Peter Beverloo wrote: > I'd slightly prefer to define an (anonymous) equality operator for PMAI: > > bool operator==(const PushMessagingAppIdentifier& a, > const PushMessagingAppIdentifier& b) {} > > While this wouldn't tell us exactly which two members aren't equal to each > other, that's OK (and is in scope of reasonable debugging of one who made the > tests fail). > > The benefit of this is that we can just use EXPECT_EQ(a, b); Discussed offline, and we've agreed that it's ok to keep a method with multiple EXPECT_EQs so failures on trybots etc give enough information to debug the failure without having to repro locally. But I've renamed the method to ExpectAppIdentifiersEqual at your suggestion. > Finally, please have anonymous namespaces at the top of the file. Done. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:62: content::TestBrowserThread ui_thread(BrowserThread::UI, &message_loop); On 2015/05/12 13:55:40, Peter Beverloo wrote: > Since we've already got the PushMessagingAppIdentifierTest base-class in either > case, I'd be OK with just having a |content::TestBrowserThreadBundle| member > that does this for you. Overhead for the two unit tests that don't need it is > minimal. Done. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:72: // Test basic PersistToPrefs round trips. On 2015/05/12 13:55:40, Peter Beverloo wrote: > nit: This can be a separate test. Done. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:88: // Test that PersistToPrefs overwrites (when same origin and Service Worker). On 2015/05/12 13:55:39, Peter Beverloo wrote: > nit: This can be a separate test. Done. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:115: // Test that PersistToPrefs doesn't overwrite (when different origin or SW). On 2015/05/12 13:55:40, Peter Beverloo wrote: > nit: This can be a separate test. Done. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:138: // Test DeleteFromPrefs. Deleted app identifier should be deleted. On 2015/05/12 13:55:39, Peter Beverloo wrote: > nit: This can be a separate test. Done. https://codereview.chromium.org/1122403009/diff/1/chrome/browser/push_messagi... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:152: // Test GetAll. Non-deleted app identifiers should all be listed. On 2015/05/12 13:55:39, Peter Beverloo wrote: > nit: This can be a separate test. Done.
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/1122403009/#ps20001 (title: "Addressed peter's comments")
The CQ bit was unchecked by johnme@chromium.org
https://codereview.chromium.org/1122403009/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc (right): https://codereview.chromium.org/1122403009/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:14: const PushMessagingAppIdentifier& b) { nit: indent https://codereview.chromium.org/1122403009/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:36: void GenerateAppIdentifiers() { nit: why can't we do this in SetUp()? https://codereview.chromium.org/1122403009/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:49: PushMessagingAppIdentifier original_; nit: private w/ getters
Addressed peter's final nits - thanks :) https://codereview.chromium.org/1122403009/diff/20001/chrome/browser/push_mes... File chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc (right): https://codereview.chromium.org/1122403009/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:14: const PushMessagingAppIdentifier& b) { On 2015/05/13 13:49:29, Peter Beverloo wrote: > nit: indent Done. https://codereview.chromium.org/1122403009/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:36: void GenerateAppIdentifiers() { On 2015/05/13 13:49:29, Peter Beverloo wrote: > nit: why can't we do this in SetUp()? Done. https://codereview.chromium.org/1122403009/diff/20001/chrome/browser/push_mes... chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc:49: PushMessagingAppIdentifier original_; On 2015/05/13 13:49:29, Peter Beverloo wrote: > nit: private w/ getters Kept this, as you mentioned offline that the style guide allows it for gtests.
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/1122403009/#ps40001 (title: "Address peter's final nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1122403009/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2f8f7b1b9ebf668d428412e334d69f48af1f8afa Cr-Commit-Position: refs/heads/master@{#329642} |
