Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2074093002:
Add a unittest for image_data_fetcher (Closed)
|
Created:
4 years, 6 months ago by markusheintz_ Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a unittest for the ImageDataFetcher class
BUG=609127
Committed: https://crrev.com/c2fbb0f083499da42b6c4793108f689995cc4a5b
Cr-Commit-Position: refs/heads/master@{#401236}
Patch Set 1 #Patch Set 2 : Update gyp files #Patch Set 3 : Fix unit_test deps #Patch Set 4 : Fix unit_test deps #Patch Set 5 : Adding comments and fix minor nits #Patch Set 6 : Sync to tot #Patch Set 7 : Really sync to tot #
Total comments: 29
Patch Set 8 : Address comments bauerb@ #
Total comments: 17
Patch Set 9 : Address comments treib@ #
Total comments: 12
Patch Set 10 : Use MOCK_METHODS #Patch Set 11 : Address comments bauerb@, treib@ #Patch Set 12 : Use only a single callback object in the FetchImageData_MultipleRequests #
Messages
Total messages: 36 (7 generated)
Fix unit_test deps
Fix unit_test deps
Adding comments and fix minor nits
Description was changed from ========== Add a unittest for image_data_fetcher BUG=609127 ========== to ========== Add a unittest for the ImageDataFetcher class BUG=609127 ==========
Sync to tot
Really sync to tot
markusheintz@chromium.org changed reviewers: + bauerb@chromium.org, treib@chromium.org
On 2016/06/20 12:53:36, markusheintz_ wrote: > Really sync to tot Hey Marc and Bernhard PTAL
https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:52: // The next id to use for a newly created URLFetcher. Each URLFetcher gets an Nit: I would capitalize "ID" (you're not talking about https://en.wikipedia.org/wiki/Id,_ego_and_super-ego#Id 😉). https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:53: // id when it id created. The |url_fetcher_id_| is incremeted by one for each "[...] when it is created". Also, "incremented". https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:21: std::string kURLResponseData("EncodedImageData"); These will create static initializers. What you want to do is store the literals as Plain Old Data, i.e. const char[]. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:35: fetcher_factory_.reset(new net::TestURLFetcherFactory()); You could make this a direct member of the fixture. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:37: num_callbacks_run_ = 0; You don't need this -- you get a fresh test fixture instance for every test. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:42: EXPECT_EQ(image_data, kURLResponseData); The expected value should go first (for nicer error messages on failure). https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:47: EXPECT_EQ(image_data, std::string("")); The empty string literal in the constructor is unnecessary. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:52: ++num_callbacks_run_; Aren't you going to verify that data that comes back? https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:63: // called during a testrun. This counter is reset duing the SetUp method. "during the SetUp method" (but really, the comment is moot; see my comment above). https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:71: image_fetcher::ImageDataFetcher image_data_fetcher( You could make this part of the fixture, as you create one in every test. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:81: ASSERT_NE(test_url_fetcher, nullptr); Expected value first. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:86: // Call the URLFetcher delegate to continue the test. And then what? Don't you want to verify that the fetcher actually finished?
Address comments bauerb@
https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:52: // The next id to use for a newly created URLFetcher. Each URLFetcher gets an On 2016/06/20 13:36:15, Bernhard Bauer wrote: > Nit: I would capitalize "ID" (you're not talking about > https://en.wikipedia.org/wiki/Id,_ego_and_super-ego#Id 😉). Done. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:53: // id when it id created. The |url_fetcher_id_| is incremeted by one for each On 2016/06/20 13:36:15, Bernhard Bauer wrote: > "[...] when it is created". Also, "incremented". Done. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:21: std::string kURLResponseData("EncodedImageData"); On 2016/06/20 13:36:16, Bernhard Bauer wrote: > These will create static initializers. What you want to do is store the literals > as Plain Old Data, i.e. const char[]. Done. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:35: fetcher_factory_.reset(new net::TestURLFetcherFactory()); On 2016/06/20 13:36:16, Bernhard Bauer wrote: > You could make this a direct member of the fixture. Done. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:37: num_callbacks_run_ = 0; On 2016/06/20 13:36:15, Bernhard Bauer wrote: > You don't need this -- you get a fresh test fixture instance for every test. Removed https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:42: EXPECT_EQ(image_data, kURLResponseData); On 2016/06/20 13:36:15, Bernhard Bauer wrote: > The expected value should go first (for nicer error messages on failure). Done. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:47: EXPECT_EQ(image_data, std::string("")); On 2016/06/20 13:36:16, Bernhard Bauer wrote: > The empty string literal in the constructor is unnecessary. Done. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:52: ++num_callbacks_run_; On 2016/06/20 13:36:16, Bernhard Bauer wrote: > Aren't you going to verify that data that comes back? That's what the other two tests are for. Here I only want to test that multiple calls to FetchImageData result in multiple calls to callbacks. There is no batching implemented like in other places. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:63: // called during a testrun. This counter is reset duing the SetUp method. On 2016/06/20 13:36:16, Bernhard Bauer wrote: > "during the SetUp method" (but really, the comment is moot; see my comment > above). removed https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:71: image_fetcher::ImageDataFetcher image_data_fetcher( On 2016/06/20 13:36:16, Bernhard Bauer wrote: > You could make this part of the fixture, as you create one in every test. Done. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:81: ASSERT_NE(test_url_fetcher, nullptr); On 2016/06/20 13:36:16, Bernhard Bauer wrote: > Expected value first. Done. Here and below https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:86: // Call the URLFetcher delegate to continue the test. On 2016/06/20 13:36:16, Bernhard Bauer wrote: > And then what? Don't you want to verify that the fetcher actually finished? You mean whether I want to test the TestURLFetcher? ;-) When the callbacks above are called, then the fetcher completed one or the other way. The comment in the TestURLFetcher code does not mention any further things that need to be run in order to make URLFetcher run to completion.
https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:57: int url_fetcher_id_; next_url_fetcher_id_ ? https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:71: image_fetcher::ImageDataFetcher image_data_fetcher( You could put this whole file into the image_fetcher namespace. https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:28: : message_loop_(), Not necessary (the default ctor will be called anyway) https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:36: void OnImageDataFetched(const std::string& image_data) { I think it'd be more idiomatic (and easier to read) to have a MOCK_METHOD1(OnImageDataFetched, void(const std::string&)) and then use EXPECT_CALL below. https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:68: image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback = optional: If you don't use this again, just do the Bind directly where you need it? https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:86: image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback = Same here. https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:107: image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback2 = You can use the same callback twice.
Address comments treib@
https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher.h:57: int url_fetcher_id_; On 2016/06/20 14:47:29, Marc Treib wrote: > next_url_fetcher_id_ ? Done. https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:71: image_fetcher::ImageDataFetcher image_data_fetcher( On 2016/06/20 14:47:29, Marc Treib wrote: > You could put this whole file into the image_fetcher namespace. Done. https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:28: : message_loop_(), On 2016/06/20 14:47:29, Marc Treib wrote: > Not necessary (the default ctor will be called anyway) I know but it also does no harm. On the upside I find it more readable when you have all the fields initialized here. In particular since this on is used to initialize the next. https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:36: void OnImageDataFetched(const std::string& image_data) { On 2016/06/20 14:47:29, Marc Treib wrote: > I think it'd be more idiomatic (and easier to read) to have a > MOCK_METHOD1(OnImageDataFetched, void(const std::string&)) > and then use EXPECT_CALL below. Sorry I'm a bit rusty when it comes to gtest. Wouldn't this require some virtual method I can mock? https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:68: image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback = On 2016/06/20 14:47:29, Marc Treib wrote: > optional: If you don't use this again, just do the Bind directly where you need > it? Done. https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:86: image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback = On 2016/06/20 14:47:29, Marc Treib wrote: > Same here. Done. https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:107: image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback2 = On 2016/06/20 14:47:29, Marc Treib wrote: > You can use the same callback twice. I want to simulate two individual calls. Therefore I wanted to have to separate objects. May I'm over thinking it.
LGTM, thanks for adding these tests! I'll leave it up to you whether to change them to use a mock method. https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:36: void OnImageDataFetched(const std::string& image_data) { On 2016/06/20 15:06:53, markusheintz_ wrote: > On 2016/06/20 14:47:29, Marc Treib wrote: > > I think it'd be more idiomatic (and easier to read) to have a > > MOCK_METHOD1(OnImageDataFetched, void(const std::string&)) > > and then use EXPECT_CALL below. > > Sorry I'm a bit rusty when it comes to gtest. Wouldn't this require some virtual > method I can mock? No, the mock method doesn't need to override anything. You can just define a method with name and signature, and then EXPECT on it. The upside is that then, the expectations would be in the test body, rather than in some "random" method up here. https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:107: image_fetcher::ImageDataFetcher::ImageDataFetcherCallback callback2 = On 2016/06/20 15:06:53, markusheintz_ wrote: > On 2016/06/20 14:47:29, Marc Treib wrote: > > You can use the same callback twice. > > I want to simulate two individual calls. Therefore I wanted to have to separate > objects. May I'm over thinking it. You'd still check the two calls with the EXPECT_EQ(num_callbacks_run_, 2). Anyway, not important. [With the mock method, this would become something like: EXPECT_CALL(*this, OnImageDataFetched(_)).Times(2); with no need for manual tracking :)] https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:115: // multiple URLFetcher beeing created. nit: being
https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/120001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:86: // Call the URLFetcher delegate to continue the test. On 2016/06/20 14:30:17, markusheintz_ wrote: > On 2016/06/20 13:36:16, Bernhard Bauer wrote: > > And then what? Don't you want to verify that the fetcher actually finished? > > You mean whether I want to test the TestURLFetcher? ;-) > > When the callbacks above are called, then the fetcher completed one or the other > way. > The comment in the TestURLFetcher code does not mention any further things that > need to be run in order to make URLFetcher run to completion. Yes, but how do you know the callbacks above are actually called? :) https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:28: : message_loop_(), On 2016/06/20 15:06:53, markusheintz_ wrote: > On 2016/06/20 14:47:29, Marc Treib wrote: > > Not necessary (the default ctor will be called anyway) > > I know but it also does no harm. That's not really an argument for adding unnecessary bytes to the file. > On the upside I find it more readable when you > have all the fields initialized here. In particular since this on is used to > initialize the next. The compiler will automatically generate a call to the empty constructor in the exact same spot if you don't specify an initializer list. You can know that the field is initialized, because the compiler will warn you if you would try to initialize fields out of order. https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:36: void OnImageDataFetched(const std::string& image_data) { These callbacks could be protected. https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:20: const GURL kImageURL("http://www.example.com/image1"); This will still create a static initializer. The only things allowed are basic data types (int, char, etc.) and pointers to and arrays thereof. https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:73: base::Unretained(this)a)); typo? https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:80: test_url_fetcher->SetResponseString(std::string(kURLResponseData)); std::string has an implicit constructor that takes a char*, so you don't need to call it explicitly. https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:114: // Multiple calls to the FetchImageData for the same URL will result in No "the" before FetchImageData https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:115: // multiple URLFetcher beeing created. URLFetchers
Use MOCK_METHODS
Address comments bauerb@, treib@
https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:28: : message_loop_(), On 2016/06/20 17:28:38, Bernhard Bauer wrote: > On 2016/06/20 15:06:53, markusheintz_ wrote: > > On 2016/06/20 14:47:29, Marc Treib wrote: > > > Not necessary (the default ctor will be called anyway) > > > > I know but it also does no harm. > > That's not really an argument for adding unnecessary bytes to the file. > > > On the upside I find it more readable when you > > have all the fields initialized here. In particular since this on is used to > > initialize the next. > > The compiler will automatically generate a call to the empty constructor in the > exact same spot if you don't specify an initializer list. You can know that the > field is initialized, because the compiler will warn you if you would try to > initialize fields out of order. Removed https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:36: void OnImageDataFetched(const std::string& image_data) { On 2016/06/20 17:28:38, Bernhard Bauer wrote: > These callbacks could be protected. Using MOCK_METHODS now https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:20: const GURL kImageURL("http://www.example.com/image1"); On 2016/06/20 17:28:38, Bernhard Bauer wrote: > This will still create a static initializer. The only things allowed are basic > data types (int, char, etc.) and pointers to and arrays thereof. sry forgot. Done https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:73: base::Unretained(this)a)); On 2016/06/20 17:28:38, Bernhard Bauer wrote: > typo? Done. https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:80: test_url_fetcher->SetResponseString(std::string(kURLResponseData)); On 2016/06/20 17:28:38, Bernhard Bauer wrote: > std::string has an implicit constructor that takes a char*, so you don't need to > call it explicitly. Done. https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:114: // Multiple calls to the FetchImageData for the same URL will result in On 2016/06/20 17:28:38, Bernhard Bauer wrote: > No "the" before FetchImageData Done. https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:115: // multiple URLFetcher beeing created. On 2016/06/20 15:21:42, Marc Treib wrote: > nit: being Done. https://codereview.chromium.org/2074093002/diff/160001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:115: // multiple URLFetcher beeing created. On 2016/06/20 17:28:38, Bernhard Bauer wrote: > URLFetchers Done.
lgtm https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... File components/image_fetcher/image_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... components/image_fetcher/image_data_fetcher_unittest.cc:36: void OnImageDataFetched(const std::string& image_data) { On 2016/06/21 15:56:08, markusheintz_ wrote: > On 2016/06/20 17:28:38, Bernhard Bauer wrote: > > These callbacks could be protected. > > Using MOCK_METHODS now They still could be protected :-D
On 2016/06/21 16:24:19, Bernhard Bauer wrote: > lgtm > > https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... > File components/image_fetcher/image_data_fetcher_unittest.cc (right): > > https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... > components/image_fetcher/image_data_fetcher_unittest.cc:36: void > OnImageDataFetched(const std::string& image_data) { > On 2016/06/21 15:56:08, markusheintz_ wrote: > > On 2016/06/20 17:28:38, Bernhard Bauer wrote: > > > These callbacks could be protected. > > > > Using MOCK_METHODS now > > They still could be protected :-D Sorry they can't since gmock demands that they are public.
Use only a single callback object in the FetchImageData_MultipleRequests
markusheintz@chromium.org changed reviewers: + blundell@chromium.org
On 2016/06/21 20:09:35, markusheintz_ wrote: > Use only a single callback object in the FetchImageData_MultipleRequests Adding blundell@ for approving the components/BUILD.gn changes. PTAL Thanks
On Tue, Jun 21, 2016, 21:00 <markusheintz@chromium.org> wrote: > On 2016/06/21 16:24:19, Bernhard Bauer wrote: > > lgtm > > > > > > https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... > > File components/image_fetcher/image_data_fetcher_unittest.cc (right): > > > > > > https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... > > components/image_fetcher/image_data_fetcher_unittest.cc:36: void > > OnImageDataFetched(const std::string& image_data) { > > On 2016/06/21 15:56:08, markusheintz_ wrote: > > > On 2016/06/20 17:28:38, Bernhard Bauer wrote: > > > > These callbacks could be protected. > > > > > > Using MOCK_METHODS now > > > > They still could be protected :-D > > Sorry they can't since gmock demands that they are public. > Hm, in the GMock documentation ( https://github.com/google/googlemock/blob/master/googlemock/docs/v1_6/CookBoo...) I only find this as rationale: "This allows ON_CALL and EXPECT_CALL to reference the mock function from outside of the mock class.". But we are in fact in a subclass when we're setting up the expectations, so protected should be sufficient. Not a big deal though :) > https://codereview.chromium.org/2074093002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/21 20:51:15, Bernhard Bauer wrote: > On Tue, Jun 21, 2016, 21:00 <mailto:markusheintz@chromium.org> wrote: > > > On 2016/06/21 16:24:19, Bernhard Bauer wrote: > > > lgtm > > > > > > > > > > > https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... > > > File components/image_fetcher/image_data_fetcher_unittest.cc (right): > > > > > > > > > > > https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... > > > components/image_fetcher/image_data_fetcher_unittest.cc:36: void > > > OnImageDataFetched(const std::string& image_data) { > > > On 2016/06/21 15:56:08, markusheintz_ wrote: > > > > On 2016/06/20 17:28:38, Bernhard Bauer wrote: > > > > > These callbacks could be protected. > > > > > > > > Using MOCK_METHODS now > > > > > > They still could be protected :-D > > > > Sorry they can't since gmock demands that they are public. > > > > Hm, in the GMock documentation ( > https://github.com/google/googlemock/blob/master/googlemock/docs/v1_6/CookBoo...) > I only find this as rationale: "This allows ON_CALL and EXPECT_CALL to > reference the mock function from outside of the mock class.". But we are in > fact in a subclass when we're setting up the expectations, so protected > should be sufficient. Not a big deal though :) The code does not compile if I make the methods protected. I don't have the link to the source at my hand, but I was told that when expectations are set, the frameworks (which is in a different package) calls the mock method. > > > > https://codereview.chromium.org/2074093002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2016/06/21 20:56:42, markusheintz_ wrote: > On 2016/06/21 20:51:15, Bernhard Bauer wrote: > > On Tue, Jun 21, 2016, 21:00 <mailto:markusheintz@chromium.org> wrote: > > > > > On 2016/06/21 16:24:19, Bernhard Bauer wrote: > > > > lgtm > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... > > > > File components/image_fetcher/image_data_fetcher_unittest.cc (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... > > > > components/image_fetcher/image_data_fetcher_unittest.cc:36: void > > > > OnImageDataFetched(const std::string& image_data) { > > > > On 2016/06/21 15:56:08, markusheintz_ wrote: > > > > > On 2016/06/20 17:28:38, Bernhard Bauer wrote: > > > > > > These callbacks could be protected. > > > > > > > > > > Using MOCK_METHODS now > > > > > > > > They still could be protected :-D > > > > > > Sorry they can't since gmock demands that they are public. > > > > > > > Hm, in the GMock documentation ( > > > https://github.com/google/googlemock/blob/master/googlemock/docs/v1_6/CookBoo...) > > I only find this as rationale: "This allows ON_CALL and EXPECT_CALL to > > reference the mock function from outside of the mock class.". But we are in > > fact in a subclass when we're setting up the expectations, so protected > > should be sufficient. Not a big deal though :) > > The code does not compile if I make the methods protected. I don't have the link > to the source at my hand, but I was told that when expectations are set, the > frameworks (which is in a different package) calls the mock method. I see. Thanks for checking!
lgtm
The CQ bit was checked by markusheintz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2074093002/#ps220001 (title: "Use only a single callback object in the FetchImageData_MultipleRequests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2074093002/220001
Message was sent while issue was closed.
Description was changed from ========== Add a unittest for the ImageDataFetcher class BUG=609127 ========== to ========== Add a unittest for the ImageDataFetcher class BUG=609127 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add a unittest for the ImageDataFetcher class BUG=609127 ========== to ========== Add a unittest for the ImageDataFetcher class BUG=609127 Committed: https://crrev.com/c2fbb0f083499da42b6c4793108f689995cc4a5b Cr-Commit-Position: refs/heads/master@{#401236} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/c2fbb0f083499da42b6c4793108f689995cc4a5b Cr-Commit-Position: refs/heads/master@{#401236}
Message was sent while issue was closed.
On 2016/06/21 21:37:22, Bernhard Bauer wrote: > On 2016/06/21 20:56:42, markusheintz_ wrote: > > On 2016/06/21 20:51:15, Bernhard Bauer wrote: > > > On Tue, Jun 21, 2016, 21:00 <mailto:markusheintz@chromium.org> wrote: > > > > > > > On 2016/06/21 16:24:19, Bernhard Bauer wrote: > > > > > lgtm > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... > > > > > File components/image_fetcher/image_data_fetcher_unittest.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2074093002/diff/140001/components/image_fetch... > > > > > components/image_fetcher/image_data_fetcher_unittest.cc:36: void > > > > > OnImageDataFetched(const std::string& image_data) { > > > > > On 2016/06/21 15:56:08, markusheintz_ wrote: > > > > > > On 2016/06/20 17:28:38, Bernhard Bauer wrote: > > > > > > > These callbacks could be protected. > > > > > > > > > > > > Using MOCK_METHODS now > > > > > > > > > > They still could be protected :-D > > > > > > > > Sorry they can't since gmock demands that they are public. > > > > > > > > > > Hm, in the GMock documentation ( > > > > > > https://github.com/google/googlemock/blob/master/googlemock/docs/v1_6/CookBoo...) > > > I only find this as rationale: "This allows ON_CALL and EXPECT_CALL to > > > reference the mock function from outside of the mock class.". But we are in > > > fact in a subclass when we're setting up the expectations, so protected > > > should be sufficient. Not a big deal though :) > > > > The code does not compile if I make the methods protected. I don't have the > link > > to the source at my hand, but I was told that when expectations are set, the > > frameworks (which is in a different package) calls the mock method. > > I see. Thanks for checking! For the sake of completeness here is the link to the doc I mentioned: https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
