Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2869013004:
Test SuggestionsServiceImpl without friending anything (Closed)
|
Created:
3 years, 7 months ago by fhorschig Modified:
3 years, 7 months ago Reviewers:
Marc Treib CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTest SuggestionsServiceImpl without friending anything
This CL changes only the tests (and test-only getters) for the
SuggestionsServiceImpl class.
Changed:
- Responding to Fetches with TestUrlFetchers -> no alternative URL hacks
- Blackboxed -> neither tests nor test class are friends of the Impl
Intentionally not changed (although considerable):
(- anything in the .cc file)
- visibility of privately inherited OnStateChanged
- constructor (to inject a clock)
- EXPECT_EQ (to go for consistency)
BUG=709959
Review-Url: https://codereview.chromium.org/2869013004
Cr-Commit-Position: refs/heads/master@{#470560}
Committed: https://chromium.googlesource.com/chromium/src/+/590d7f9b223b8fde7722c7f0c395f8cd2e7e3bc9
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebase (and rerebase to gclient) #Patch Set 3 : Rebase (to HEAD) #
Total comments: 54
Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : Fix comments #
Messages
Total messages: 19 (12 generated)
Patchset #1 (id:1) has been deleted
fhorschig@chromium.org changed reviewers: + treib@chromium.org
Hi Marc, would you please take a look? https://codereview.chromium.org/2869013004/diff/20001/components/suggestions/... File components/suggestions/suggestions_service_impl_unittest.cc (left): https://codereview.chromium.org/2869013004/diff/20001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:688: EXPECT_EQ(blacklisted_url, retrieved_url.spec()); This case is covered by other tests like RetryBlacklistURLRequestAfterFailure which would not call the mock_blacklist_store_ otherwise.
Description was changed from ========== Test SuggestionsServiceImpl without friending anything This CL changes only the tests (and test-only getters) for the SuggestionsServiceImpl class. Changed: - Tests don't locally own service -> no dangling pointers. - Responding to Fetches with TestUrlFetchers -> no alternative URL hacks - Blackboxed -> neither tests nor test class are friends of the Impl - Inlined large test helpers (that were essentially parameterized tests) Intentionally not changed (although considerable): (- anything in the .cc file) - visibility of privately inherited OnStateChanged - constructor (to inject a clock) - EXPECT_EQ (to go for consistency) BUG=709959 ========== to ========== Test SuggestionsServiceImpl without friending anything This CL changes only the tests (and test-only getters) for the SuggestionsServiceImpl class. Changed: - Responding to Fetches with TestUrlFetchers -> no alternative URL hacks - Blackboxed -> neither tests nor test class are friends of the Impl - Inlined large test helpers (that were essentially parameterized tests) Intentionally not changed (although considerable): (- anything in the .cc file) - visibility of privately inherited OnStateChanged - constructor (to inject a clock) - EXPECT_EQ (to go for consistency) BUG=709959 ==========
Very nice overall! Lots of small comments below. Description nit: "Inlined large test helpers" was rebased away ;) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... File components/suggestions/suggestions_service_impl.h (right): https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl.h:81: base::TimeDelta blacklist_delay_for_testing() const { Out of curiosity: Do you feel that "for_testing" methods are better than friends? I usually tend to prefer friending the test base class, since then you don't have to clutter up the public interface (but I don't feel very strongly, so this is fine too). https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... File components/suggestions/suggestions_service_impl_unittest.cc (left): https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:473: // BlacklistStore's candidacy delay are zero). I'd leave the explanation here. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... File components/suggestions/suggestions_service_impl_unittest.cc (right): https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:163: testing::Mock::VerifyAndClearExpectations(sync_service()); I don't think this is necessary. Doesn't that happen at destruction anyway? https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:166: GURL CurrentlyQueriedUrl() { GetCurrentlyQueriedUrl? https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:185: void RespondToFetchWithProfile(const SuggestionsProfile& profile) { nit: RespondToFetchWithSuggestions? I find the "profile" name quite confusing; "profile" usually has a different meaning in Chrome. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:191: void NotifySuggestionsService() { Notify of what? TBH, I'd leave this inlined into all the tests that need it. You can make it a one-liner by inlining the cast. (Or by making the overridden method public, which people seem to prefer anyway. Not a fan of that, but oh well.) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:198: void IgnoreRepeatedMockCalls() { The point isn't to ignore the calls, but to return appropriate defaults. (Arguably these shouldn't be mocks at all, but fakes. But let's leave that for a possible followup.) So "Set up default responses" or something? (Or just inline it back into SetUp? If not, it should be private.) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:226: FakeProfileOAuth2TokenService* token_service() { return &token_service_; } nit: The order of the accessors seems a bit arbitrary. Match the order of the fields? https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:229: void CreateSuggestionsServiceWithMocks() { Any reason this is a separate method? https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:275: base::RunLoop().RunUntilIdle(); Is the RunLoop still needed? (Lots more instances of this below) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:277: EXPECT_TRUE(CurrentlyQueriedUrl().is_valid()); Valid implies not empty, so the above check is redundant. Also, this should maybe be an ASSERT? https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:281: SuggestionsProfile suggestions_profile; nit: Any reason for putting the "profile" back? (more instances below) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:297: // Let any network request run (there shouldn't be one). If the RunLoop is still needed, this comment needs to be updated. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:411: net::ERR_INVALID_RESPONSE)); What exactly does this test? https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:434: // Calling RunUntilIdle on the MessageLoop only works when the task is not nit: It's a RunLoop, not a MessageLoop. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:497: EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_)) I think the stuff below might be worth a comment (along the lines of the "Note" that was there before) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:521: ASSERT_TRUE(CurrentlyQueriedUrl().is_valid()); nit: Why? There isn't a similar assert above. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:563: // Ensure scheduling the request doesn't happen before undo. Pre-existing problem, but this comment seems out of place - there's no undo involved? https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:566: base::MockCallback<SuggestionsService::ResponseCallback> callback; nit: Keep the newline above https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:592: base::MockCallback<SuggestionsService::ResponseCallback> callback; Also here: newline above https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:611: EXPECT_FALSE(suggestions_service_->UndoBlacklistURL(blacklisted_url)); These two lines were replaced by the two below I think? https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:647: TEST_F(SuggestionsServiceTest, TemporaryIncreasesBlacklistDelayOnFailure) { s/Temporary/Temporarily/ https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:703: EXPECT_LT(kTestSetExpiry, suggestions_profile.suggestions(0).expiry_ts()); nit: Check suggestions(0) first?
Description was changed from ========== Test SuggestionsServiceImpl without friending anything This CL changes only the tests (and test-only getters) for the SuggestionsServiceImpl class. Changed: - Responding to Fetches with TestUrlFetchers -> no alternative URL hacks - Blackboxed -> neither tests nor test class are friends of the Impl - Inlined large test helpers (that were essentially parameterized tests) Intentionally not changed (although considerable): (- anything in the .cc file) - visibility of privately inherited OnStateChanged - constructor (to inject a clock) - EXPECT_EQ (to go for consistency) BUG=709959 ========== to ========== Test SuggestionsServiceImpl without friending anything This CL changes only the tests (and test-only getters) for the SuggestionsServiceImpl class. Changed: - Responding to Fetches with TestUrlFetchers -> no alternative URL hacks - Blackboxed -> neither tests nor test class are friends of the Impl Intentionally not changed (although considerable): (- anything in the .cc file) - visibility of privately inherited OnStateChanged - constructor (to inject a clock) - EXPECT_EQ (to go for consistency) BUG=709959 ==========
https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... File components/suggestions/suggestions_service_impl.h (right): https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl.h:81: base::TimeDelta blacklist_delay_for_testing() const { On 2017/05/10 09:53:44, Marc Treib wrote: > Out of curiosity: Do you feel that "for_testing" methods are better than > friends? I usually tend to prefer friending the test base class, since then you > don't have to clutter up the public interface (but I don't feel very strongly, > so this is fine too). In this case, I am (obviously) absolutely for for_testing as the friending was abused to test small parts of the implementation and not overall behaviors. Also, the public interface that really is used is SuggestionsService, so in order to abuse the public for_testing methods, you would have to put in quite some effort. (2nd point in our guidelines: go/zine-testing#heading=h.ch8i4lz9s0f2) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... File components/suggestions/suggestions_service_impl_unittest.cc (left): https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:473: // BlacklistStore's candidacy delay are zero). On 2017/05/10 09:53:45, Marc Treib wrote: > I'd leave the explanation here. Reverted. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... File components/suggestions/suggestions_service_impl_unittest.cc (right): https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:163: testing::Mock::VerifyAndClearExpectations(sync_service()); On 2017/05/10 09:53:44, Marc Treib wrote: > I don't think this is necessary. Doesn't that happen at destruction anyway? Gone. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:166: GURL CurrentlyQueriedUrl() { On 2017/05/10 09:53:45, Marc Treib wrote: > GetCurrentlyQueriedUrl? Done. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:185: void RespondToFetchWithProfile(const SuggestionsProfile& profile) { On 2017/05/10 09:53:44, Marc Treib wrote: > nit: RespondToFetchWithSuggestions? I find the "profile" name quite confusing; > "profile" usually has a different meaning in Chrome. Done. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:191: void NotifySuggestionsService() { On 2017/05/10 09:53:45, Marc Treib wrote: > Notify of what? > TBH, I'd leave this inlined into all the tests that need it. You can make it a > one-liner by inlining the cast. (Or by making the overridden method public, > which people seem to prefer anyway. Not a fan of that, but oh well.) Inlined with cast. (I am a little bit split here. Making the already publicly accessible method public seems like a good move) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:198: void IgnoreRepeatedMockCalls() { On 2017/05/10 09:53:45, Marc Treib wrote: > The point isn't to ignore the calls, but to return appropriate defaults. > (Arguably these shouldn't be mocks at all, but fakes. But let's leave that for a > possible followup.) > So "Set up default responses" or something? (Or just inline it back into SetUp? > If not, it should be private.) Inlined. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:226: FakeProfileOAuth2TokenService* token_service() { return &token_service_; } On 2017/05/10 09:53:45, Marc Treib wrote: > nit: The order of the accessors seems a bit arbitrary. Match the order of the > fields? I don't consider alphabetic ordering very arbitrary. But order of fields make more sense. Changed. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:229: void CreateSuggestionsServiceWithMocks() { On 2017/05/10 09:53:44, Marc Treib wrote: > Any reason this is a separate method? Only Historic reasons. Inlined into setup. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:275: base::RunLoop().RunUntilIdle(); On 2017/05/10 09:53:45, Marc Treib wrote: > Is the RunLoop still needed? (Lots more instances of this below) Sadly yes. The request is preceeded by a task from the token fetcher. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:277: EXPECT_TRUE(CurrentlyQueriedUrl().is_valid()); On 2017/05/10 09:53:45, Marc Treib wrote: > Valid implies not empty, so the above check is redundant. > Also, this should maybe be an ASSERT? Absolutely. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:281: SuggestionsProfile suggestions_profile; On 2017/05/10 09:53:44, Marc Treib wrote: > nit: Any reason for putting the "profile" back? (more instances below) Rerenamed all instances. (Technically, it _is_ a profile that contains a list of suggestions) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:297: // Let any network request run (there shouldn't be one). On 2017/05/10 09:53:44, Marc Treib wrote: > If the RunLoop is still needed, this comment needs to be updated. Done. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:411: net::ERR_INVALID_RESPONSE)); On 2017/05/10 09:53:45, Marc Treib wrote: > What exactly does this test? The original idea was probably to just have the code path running and look if it crashes. The only expectation is |GetTimeUntilReadyForUpload| which seems to be a hacky proxy to check whether |ScheduleBlacklistUpload| has been called. (I did not want to write a replacement in this CL. It's already too large) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:434: // Calling RunUntilIdle on the MessageLoop only works when the task is not On 2017/05/10 09:53:45, Marc Treib wrote: > nit: It's a RunLoop, not a MessageLoop. Thanks! I mindlessly copied that comment. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:497: EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_)) On 2017/05/10 09:53:45, Marc Treib wrote: > I think the stuff below might be worth a comment (along the lines of the "Note" > that was there before) Done and split the expectations to see the the correlation to success/failure. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:521: ASSERT_TRUE(CurrentlyQueriedUrl().is_valid()); On 2017/05/10 09:53:44, Marc Treib wrote: > nit: Why? There isn't a similar assert above. But there should be. Fixed. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:563: // Ensure scheduling the request doesn't happen before undo. On 2017/05/10 09:53:45, Marc Treib wrote: > Pre-existing problem, but this comment seems out of place - there's no undo > involved? Gone. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:566: base::MockCallback<SuggestionsService::ResponseCallback> callback; On 2017/05/10 09:53:44, Marc Treib wrote: > nit: Keep the newline above Done. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:592: base::MockCallback<SuggestionsService::ResponseCallback> callback; On 2017/05/10 09:53:45, Marc Treib wrote: > Also here: newline above Also done. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:611: EXPECT_FALSE(suggestions_service_->UndoBlacklistURL(blacklisted_url)); On 2017/05/10 09:53:44, Marc Treib wrote: > These two lines were replaced by the two below I think? Gone. (Yes, they even broke compilation) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:647: TEST_F(SuggestionsServiceTest, TemporaryIncreasesBlacklistDelayOnFailure) { On 2017/05/10 09:53:45, Marc Treib wrote: > s/Temporary/Temporarily/ Done. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:703: EXPECT_LT(kTestSetExpiry, suggestions_profile.suggestions(0).expiry_ts()); On 2017/05/10 09:53:45, Marc Treib wrote: > nit: Check suggestions(0) first? Done.
Thanks! LGTM https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... File components/suggestions/suggestions_service_impl_unittest.cc (right): https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:191: void NotifySuggestionsService() { On 2017/05/10 11:54:45, fhorschig wrote: > On 2017/05/10 09:53:45, Marc Treib wrote: > > Notify of what? > > TBH, I'd leave this inlined into all the tests that need it. You can make it a > > one-liner by inlining the cast. (Or by making the overridden method public, > > which people seem to prefer anyway. Not a fan of that, but oh well.) > > Inlined with cast. > (I am a little bit split here. Making the already publicly accessible method > public > seems like a good move) We had that discussion before. I prefer keeping it private, but I'm in the minority :) Anyway, either's fine. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:226: FakeProfileOAuth2TokenService* token_service() { return &token_service_; } On 2017/05/10 11:54:45, fhorschig wrote: > On 2017/05/10 09:53:45, Marc Treib wrote: > > nit: The order of the accessors seems a bit arbitrary. Match the order of the > > fields? > > I don't consider alphabetic ordering very arbitrary. > But order of fields make more sense. Changed. Hah, I didn't notice it was alphabetical :) Still, the new order is better IMO. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:281: SuggestionsProfile suggestions_profile; On 2017/05/10 11:54:45, fhorschig wrote: > On 2017/05/10 09:53:44, Marc Treib wrote: > > nit: Any reason for putting the "profile" back? (more instances below) > > Rerenamed all instances. > (Technically, it _is_ a profile that contains a list of suggestions) Some places called this "suggestions_profile", some just "suggestions". I decided to make it "suggestions" everywhere for consistency, since I'm not a fan of the "Profile" name anyway. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:411: net::ERR_INVALID_RESPONSE)); On 2017/05/10 11:54:45, fhorschig wrote: > On 2017/05/10 09:53:45, Marc Treib wrote: > > What exactly does this test? > > The original idea was probably to just have the code path running and look if > it crashes. > The only expectation is |GetTimeUntilReadyForUpload| which seems to be a hacky > proxy to check whether |ScheduleBlacklistUpload| has been called. > > (I did not want to write a replacement in this CL. It's already too large) Acknowledged. https://codereview.chromium.org/2869013004/diff/80001/components/suggestions/... File components/suggestions/suggestions_service_impl_unittest.cc (right): https://codereview.chromium.org/2869013004/diff/80001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:484: // Have the first call fail and the second one succeed. Fail expectations: nit: "Fail expectations" is a little misleading, since nothing here is actually related to the failing (right?) https://codereview.chromium.org/2869013004/diff/80001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:637: // Undo expectations. ?
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! (Generally for explaining your thoughts behind every major comment) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... File components/suggestions/suggestions_service_impl_unittest.cc (right): https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:191: void NotifySuggestionsService() { On 2017/05/10 12:16:24, Marc Treib wrote: > On 2017/05/10 11:54:45, fhorschig wrote: > > On 2017/05/10 09:53:45, Marc Treib wrote: > > > Notify of what? > > > TBH, I'd leave this inlined into all the tests that need it. You can make it > a > > > one-liner by inlining the cast. (Or by making the overridden method public, > > > which people seem to prefer anyway. Not a fan of that, but oh well.) > > > > Inlined with cast. > > (I am a little bit split here. Making the already publicly accessible method > > public > > seems like a good move) > > We had that discussion before. I prefer keeping it private, but I'm in the > minority :) > Anyway, either's fine. It's not a big deal. (My reasoning was: we don't approve private inheritance, so why would you make single methods private ... it's the same outcome with different use of keywords.) https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:226: FakeProfileOAuth2TokenService* token_service() { return &token_service_; } On 2017/05/10 12:16:24, Marc Treib wrote: > On 2017/05/10 11:54:45, fhorschig wrote: > > On 2017/05/10 09:53:45, Marc Treib wrote: > > > nit: The order of the accessors seems a bit arbitrary. Match the order of > the > > > fields? > > > > I don't consider alphabetic ordering very arbitrary. > > But order of fields make more sense. Changed. > > Hah, I didn't notice it was alphabetical :) > Still, the new order is better IMO. Agreed. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:281: SuggestionsProfile suggestions_profile; On 2017/05/10 12:16:24, Marc Treib wrote: > On 2017/05/10 11:54:45, fhorschig wrote: > > On 2017/05/10 09:53:44, Marc Treib wrote: > > > nit: Any reason for putting the "profile" back? (more instances below) > > > > Rerenamed all instances. > > (Technically, it _is_ a profile that contains a list of suggestions) > > Some places called this "suggestions_profile", some just "suggestions". I > decided to make it "suggestions" everywhere for consistency, since I'm not a fan > of the "Profile" name anyway. Acknowledged. https://codereview.chromium.org/2869013004/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:411: net::ERR_INVALID_RESPONSE)); On 2017/05/10 12:16:24, Marc Treib wrote: > On 2017/05/10 11:54:45, fhorschig wrote: > > On 2017/05/10 09:53:45, Marc Treib wrote: > > > What exactly does this test? > > > > The original idea was probably to just have the code path running and look if > > it crashes. > > The only expectation is |GetTimeUntilReadyForUpload| which seems to be a hacky > > proxy to check whether |ScheduleBlacklistUpload| has been called. > > > > (I did not want to write a replacement in this CL. It's already too large) > > Acknowledged. Acknowledged. https://codereview.chromium.org/2869013004/diff/80001/components/suggestions/... File components/suggestions/suggestions_service_impl_unittest.cc (right): https://codereview.chromium.org/2869013004/diff/80001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:484: // Have the first call fail and the second one succeed. Fail expectations: On 2017/05/10 12:16:24, Marc Treib wrote: > nit: "Fail expectations" is a little misleading, since nothing here is actually > related to the failing (right?) Yes, removed & rephrased. https://codereview.chromium.org/2869013004/diff/80001/components/suggestions/... components/suggestions/suggestions_service_impl_unittest.cc:637: // Undo expectations. On 2017/05/10 12:16:24, Marc Treib wrote: > ? Gone.
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 fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2869013004/#ps100001 (title: "Fix comments")
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": 100001, "attempt_start_ts": 1494425464174550,
"parent_rev": "610e540fa3179c3a5ed8eb91b7d48e935c67d86d", "commit_rev":
"590d7f9b223b8fde7722c7f0c395f8cd2e7e3bc9"}
Message was sent while issue was closed.
Description was changed from ========== Test SuggestionsServiceImpl without friending anything This CL changes only the tests (and test-only getters) for the SuggestionsServiceImpl class. Changed: - Responding to Fetches with TestUrlFetchers -> no alternative URL hacks - Blackboxed -> neither tests nor test class are friends of the Impl Intentionally not changed (although considerable): (- anything in the .cc file) - visibility of privately inherited OnStateChanged - constructor (to inject a clock) - EXPECT_EQ (to go for consistency) BUG=709959 ========== to ========== Test SuggestionsServiceImpl without friending anything This CL changes only the tests (and test-only getters) for the SuggestionsServiceImpl class. Changed: - Responding to Fetches with TestUrlFetchers -> no alternative URL hacks - Blackboxed -> neither tests nor test class are friends of the Impl Intentionally not changed (although considerable): (- anything in the .cc file) - visibility of privately inherited OnStateChanged - constructor (to inject a clock) - EXPECT_EQ (to go for consistency) BUG=709959 Review-Url: https://codereview.chromium.org/2869013004 Cr-Commit-Position: refs/heads/master@{#470560} Committed: https://chromium.googlesource.com/chromium/src/+/590d7f9b223b8fde7722c7f0c395... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/590d7f9b223b8fde7722c7f0c395... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
