Unit-testing AnswerCardSearchProvider.

Unit-testing AnswerCardSearchProvider.

Removed OpenURLFromTab from the provider, as it’s not related to its
mission to produce search results.

Bug=712331
Review-Url: https://codereview.chromium.org/2947283002
Cr-Commit-Position: refs/heads/master@{#491631}
Committed: https://chromium.googlesource.com/chromium/src/+/a138ec706839c1f162b0dd4e7b916bce57344ef8

[vadimt, 2017-06-21 23:33:13]

The CQ bit was checked by vadimt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 23:33:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vadimt, 2017-06-21 23:35:46]

vadimt@chromium.org changed reviewers:
+ jam@chromium.org, xiyuan@chromium.org

[vadimt, 2017-06-21 23:35:47]

jam@chromium.org: Please review changes in 
content/

xiyuan@chromium.org: Please review changes in 
chrome/

[commit-bot: I haz the power, 2017-06-21 23:42:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 23:42:37]

Dry run: 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...)

[vadimt, 2017-06-21 23:49:32]

Hope the try run failure won't block the review, as it seems to be related to a
bogus warning by presubmit. I'm digging in this...

[xiyuan, 2017-06-22 16:43:22]

https://codereview.chromium.org/2947283002/diff/1/chrome/browser/ui/app_list/...
File
chrome/browser/ui/app_list/search/answer_card/answer_card_search_provider_unittest.cc
(right):

https://codereview.chromium.org/2947283002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search/answer_card/answer_card_search_provider_unittest.cc:38:
views::View* const kView = reinterpret_cast<views::View*>(123456);
Can we create a real views::View owned by AnswerCardSearchProviderTest instead ?

https://codereview.chromium.org/2947283002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search/answer_card/answer_card_search_provider_unittest.cc:97:
EXPECT_EQ(expected_result_count, results().size());
We might need a 
   testing::Mock::VerifyAndClearExpectations(contents());
here because EXPECT_CALL() is verified on mock object destruction by default and
TestHeadersParsing() is called in sequence before mock contents is destroyed.

https://codereview.chromium.org/2947283002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search/answer_card/answer_card_search_provider_unittest.cc:135:
std::unique_ptr<AnswerCardContents> contents(contents_);
Would presubmit script complains about using std::unique_ptr constructor like
this? If so, maybe we should add a GetContentsForTest accessor to
AnswerCardSearchProvider instead.

[vadimt, 2017-06-22 19:48:03]

Also fixed presubmit problems that turned to be real.

https://codereview.chromium.org/2947283002/diff/1/chrome/browser/ui/app_list/...
File
chrome/browser/ui/app_list/search/answer_card/answer_card_search_provider_unittest.cc
(right):

https://codereview.chromium.org/2947283002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search/answer_card/answer_card_search_provider_unittest.cc:38:
views::View* const kView = reinterpret_cast<views::View*>(123456);
On 2017/06/22 16:43:21, xiyuan wrote:
> Can we create a real views::View owned by AnswerCardSearchProviderTest instead
?

Done.

https://codereview.chromium.org/2947283002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search/answer_card/answer_card_search_provider_unittest.cc:97:
EXPECT_EQ(expected_result_count, results().size());
On 2017/06/22 16:43:21, xiyuan wrote:
> We might need a 
>    testing::Mock::VerifyAndClearExpectations(contents());
> here because EXPECT_CALL() is verified on mock object destruction by default
and
> TestHeadersParsing() is called in sequence before mock contents is destroyed.

Done.

https://codereview.chromium.org/2947283002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search/answer_card/answer_card_search_provider_unittest.cc:135:
std::unique_ptr<AnswerCardContents> contents(contents_);
No, presubmit is fine.

[xiyuan, 2017-06-22 19:50:36]

lgtm

[jam, 2017-06-23 01:00:37]

jam@chromium.org changed reviewers:
+ clamy@chromium.org, nasko@chromium.org

[jam, 2017-06-23 01:00:38]

Camille/Nasko: what are your thoughts? Originally I thought we can avoid this by
using the test method on NH to create one. But Vadim pointed out it needs a RFH.

The options are:
-this cl
-create a dummy RFH to pass to NH
-make NH handle a null FTN
-?

I prefer we avoid 3, because it complicates production code. What do you prefer?

[clamy, 2017-06-23 13:46:14]

On 2017/06/23 01:00:38, jam wrote:
> Camille/Nasko: what are your thoughts? Originally I thought we can avoid this
by
> using the test method on NH to create one. But Vadim pointed out it needs a
RFH.
> 
> The options are:
> -this cl
> -create a dummy RFH to pass to NH
> -make NH handle a null FTN
> -?
> 
> I prefer we avoid 3, because it complicates production code. What do you
prefer?

Have you looked at whether we could use a NavigationSimulator to simulate the
navigation? This should create the required RFH, NavHandles etc.. and you can
specify a certain number of parameters. We might not have headers, but I would
be fine with adding them if needed.

[vadimt, 2017-06-23 17:59:59]

The CQ bit was checked by vadimt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 18:00:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 19:27:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 19:27:18]

Dry run: This issue passed the CQ dry run.

[vadimt, 2017-07-25 01:27:33]

On 2017/06/23 13:46:14, clamy wrote:
> On 2017/06/23 01:00:38, jam wrote:
> > Camille/Nasko: what are your thoughts? Originally I thought we can avoid
this
> by
> > using the test method on NH to create one. But Vadim pointed out it needs a
> RFH.
> > 
> > The options are:
> > -this cl
> > -create a dummy RFH to pass to NH
> > -make NH handle a null FTN
> > -?
> > 
> > I prefer we avoid 3, because it complicates production code. What do you
> prefer?
> 
> Have you looked at whether we could use a NavigationSimulator to simulate the
> navigation? This should create the required RFH, NavHandles etc.. and you can
> specify a certain number of parameters. We might not have headers, but I would
> be fine with adding them if needed.

I see that NavigationSimulator doesn't create RFH, but takes it as a parameter,
like:
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe...
Are there methods that I'm missing that actually create NavigationSimulator
without requiring an RFH instance?

[clamy, 2017-07-25 11:49:21]

On 2017/07/25 01:27:33, vadimt wrote:
> On 2017/06/23 13:46:14, clamy wrote:
> > On 2017/06/23 01:00:38, jam wrote:
> > > Camille/Nasko: what are your thoughts? Originally I thought we can avoid
> this
> > by
> > > using the test method on NH to create one. But Vadim pointed out it needs
a
> > RFH.
> > > 
> > > The options are:
> > > -this cl
> > > -create a dummy RFH to pass to NH
> > > -make NH handle a null FTN
> > > -?
> > > 
> > > I prefer we avoid 3, because it complicates production code. What do you
> > prefer?
> > 
> > Have you looked at whether we could use a NavigationSimulator to simulate
the
> > navigation? This should create the required RFH, NavHandles etc.. and you
can
> > specify a certain number of parameters. We might not have headers, but I
would
> > be fine with adding them if needed.
> 
> I see that NavigationSimulator doesn't create RFH, but takes it as a
parameter,
> like:
>
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe...
> Are there methods that I'm missing that actually create NavigationSimulator
> without requiring an RFH instance?

We will soon have a version that will create a NavigationSimulator with just a
WebContents but that might not solve your problem - since if you have a
WebContents you have a RFH. NavigationSimulator was meant to be used in
RenderViewHostTestHarness based-tests, where getting the RFH is not an issue.

[jam, 2017-07-25 17:49:30]

On 2017/07/25 11:49:21, clamy wrote:
> On 2017/07/25 01:27:33, vadimt wrote:
> > On 2017/06/23 13:46:14, clamy wrote:
> > > On 2017/06/23 01:00:38, jam wrote:
> > > > Camille/Nasko: what are your thoughts? Originally I thought we can avoid
> > this
> > > by
> > > > using the test method on NH to create one. But Vadim pointed out it
needs
> a
> > > RFH.
> > > > 
> > > > The options are:
> > > > -this cl
> > > > -create a dummy RFH to pass to NH
> > > > -make NH handle a null FTN
> > > > -?
> > > > 
> > > > I prefer we avoid 3, because it complicates production code. What do you
> > > prefer?
> > > 
> > > Have you looked at whether we could use a NavigationSimulator to simulate
> the
> > > navigation? This should create the required RFH, NavHandles etc.. and you
> can
> > > specify a certain number of parameters. We might not have headers, but I
> would
> > > be fine with adding them if needed.
> > 
> > I see that NavigationSimulator doesn't create RFH, but takes it as a
> parameter,
> > like:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe...
> > Are there methods that I'm missing that actually create NavigationSimulator
> > without requiring an RFH instance?
> 
> We will soon have a version that will create a NavigationSimulator with just a
> WebContents but that might not solve your problem - since if you have a
> WebContents you have a RFH. NavigationSimulator was meant to be used in
> RenderViewHostTestHarness based-tests, where getting the RFH is not an issue.

Camille: to make sure I'm understanding, does this mean that this patch is fine
as is and Vadim can't use NavigationSimulator?
Vadim: if Camille's answer is that there's no other way and we need the mocks,
please move them to content/public/test since chrome can only include files from
content/public.

[clamy, 2017-07-27 12:25:25]

Is it not possible for the test to derive from RenderViewHostTestHarness? This
way the mock would not be needed.

https://codereview.chromium.org/2947283002/diff/50001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/answer_card/DEPS (right):

https://codereview.chromium.org/2947283002/diff/50001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/answer_card/DEPS:2: "+content/test",
This is wrong. You should not include content/test outside of content. Only
content/public/* can be used outside of content/.

[vadimt, 2017-07-28 01:55:20]

No, I can't inherit from RenderViewHostTestHarness because I'd have to go up the
hierarchy to ExtensionServiceTestBase, and reinherit at that level, at which
point (this is based on experience!) I'll have a strong pushback from owners
because they don't want general classes like this to include things that are
only needed for a particular test suite.

https://codereview.chromium.org/2947283002/diff/50001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/answer_card/DEPS (right):

https://codereview.chromium.org/2947283002/diff/50001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/answer_card/DEPS:2: "+content/test",
On 2017/07/27 12:25:25, clamy wrote:
> This is wrong. You should not include content/test outside of content. Only
> content/public/* can be used outside of content/.

Done.

[clamy, 2017-08-02 12:05:16]

On 2017/07/28 01:55:20, vadimt wrote:
> No, I can't inherit from RenderViewHostTestHarness because I'd have to go up
the
> hierarchy to ExtensionServiceTestBase, and reinherit at that level, at which
> point (this is based on experience!) I'll have a strong pushback from owners
> because they don't want general classes like this to include things that are
> only needed for a particular test suite.
> 
>
https://codereview.chromium.org/2947283002/diff/50001/chrome/browser/ui/app_l...
> File chrome/browser/ui/app_list/search/answer_card/DEPS (right):
> 
>
https://codereview.chromium.org/2947283002/diff/50001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/search/answer_card/DEPS:2: "+content/test",
> On 2017/07/27 12:25:25, clamy wrote:
> > This is wrong. You should not include content/test outside of content. Only
> > content/public/* can be used outside of content/.
> 
> Done.

Sorry for the delay, I had to make up my mind on the CL. Overall, I don't think
we want to add a MockNavigationHandle to the content/public/ testing API. The
reason is that we've spent a lot of engineering time dealing with unit tests
that simulate navigations in the wrong way, so we want to encourage the usage of
testing APIs that do things in the right way and are easily maintainable
(NavigationSimulator). Adding a mock version of NavigationHandle goes against
that.

In terms of writing tests, this means that tests that rely on navigation (which
NavigationHandle is a part of) should extend RenderViewHostTestHarness as much
as possible - afterall having navigation without having frames makes little
sense. When that is not possible, having test/mock versions of the function
using the NavigationHandle can be considered. In terms of doing a full mock of
the NavigationHandle, if you want to go that way please put the mock either in
chrome/ test utils or in your code, since the issue here is that the answer card
code is forwarding a bunch of WebContents::Observer calls while dropping the
WebContents itself.

[vadimt, 2017-08-03 02:06:35]

OK, thanks for your time, acting in the spirit of your advice, I've moved
parsing headers from the tested code. Now there is no need to mimic navigation.

[vadimt, 2017-08-03 02:06:53]

The CQ bit was checked by vadimt@chromium.org

[vadimt, 2017-08-03 02:06:53]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2947283002/#ps90001
(title: "CR comments")

[commit-bot: I haz the power, 2017-08-03 02:06:57]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-03 03:31:10]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1501726013382220,
"parent_rev": "016663d8bb463ada632b37e5db8ff9fd3819c0d3", "commit_rev":
"fd277488429a0d9bcff0a387cb7baf81462c105f"}

[commit-bot: I haz the power, 2017-08-03 03:31:26]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1501726013382220,
"parent_rev": "020014791f31622c9d659c9091745eeb86c53422", "commit_rev":
"a138ec706839c1f162b0dd4e7b916bce57344ef8"}

[commit-bot: I haz the power, 2017-08-03 03:31:41]

Description was changed from

==========
Unit-testing AnswerCardSearchProvider.

Removed OpenURLFromTab from the provider, as it’s not related to its
mission to produce search results.

Bug=712331
==========

to

==========
Unit-testing AnswerCardSearchProvider.

Removed OpenURLFromTab from the provider, as it’s not related to its
mission to produce search results.

Bug=712331

Review-Url: https://codereview.chromium.org/2947283002
Cr-Commit-Position: refs/heads/master@{#491631}
Committed:
https://chromium.googlesource.com/chromium/src/+/a138ec706839c1f162b0dd4e7b91...
==========

[commit-bot: I haz the power, 2017-08-03 03:31:42]

Committed patchset #6 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/a138ec706839c1f162b0dd4e7b91...