[Instant] Instant Extended tests for search term extraction.

[Instant] Instant Extended tests for search term extraction.

Some Instant Extended tests around search term extraction, specifically the validity of text around "Enter commit" and "Focus lost commit"

BUG=None


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180856

[Mathieu, 2013-02-01 16:52:12]

Hi Sreeram, 

I decided to send this separately. Please have a look!

[gideonwald1, 2013-02-01 17:51:46]

Thank you for doing this, Mathieu!


On Fri, Feb 1, 2013 at 8:52 AM, <mathp@chromium.org> wrote:

> Reviewers: sreeram,
>
> Message:
> Hi Sreeram,
>
> I decided to send this separately. Please have a look!
>
> Description:
> [Instant] Instant Extended tests for search term extraction.
>
> Some Instant Extended tests around search term extraction.
>
> BUG=None
>
>
> Please review this at
https://codereview.chromium.**org/12114050/<https://codereview.chromium.org/1...
>
> SVN Base:
http://git.chromium.org/**chromium/src.git@master<http://git.chromium.org/chr...
>
> Affected files:
>   M chrome/browser/instant/**instant_extended_browsertest.**cc
>   M chrome/browser/instant/**instant_test_utils.h
>   M chrome/browser/instant/**instant_test_utils.cc
>
>
> Index: chrome/browser/instant/**instant_extended_browsertest.**cc
> diff --git a/chrome/browser/instant/**instant_extended_browsertest.**cc
> b/chrome/browser/instant/**instant_extended_browsertest.**cc
> index 08f8b4d094579c90ea12e3e047faf8**820e4c4b55..**
> 25c33577a53ff5a44c8520a82c824f**cd3681ae28 100644
> --- a/chrome/browser/instant/**instant_extended_browsertest.**cc
> +++ b/chrome/browser/instant/**instant_extended_browsertest.**cc
> @@ -2,9 +2,12 @@
>  // Use of this source code is governed by a BSD-style license that can be
>  // found in the LICENSE file.
>
> +#include "chrome/browser/instant/**instant_commit_type.h"
>  #include "chrome/browser/instant/**instant_loader.h"
>  #include "chrome/browser/instant/**instant_test_utils.h"
>  #include "chrome/browser/ui/search/**search.h"
> +#include "chrome/common/instant_types.**h"
> +#include "chrome/common/search_types.h"
>  #include "chrome/test/base/interactive_**test_utils.h"
>  #include "chrome/test/base/ui_test_**utils.h"
>
> @@ -12,8 +15,9 @@ class InstantExtendedTest : public InstantTestBase {
>   protected:
>    virtual void SetUpInProcessBrowserTestFixtu**re() OVERRIDE {
>      chrome::search::**EnableInstantExtendedAPIForTes**ting();
> -    ASSERT_TRUE(test_server()->**Start());
> -    instant_url_ = test_server()->GetURL("files/**
> instant_extended.html");
> +    ASSERT_TRUE(https_test_server_**->Start());
> +    instant_url_ = https_test_server_->
> +        GetURL("files/instant_**extended.html?espv=1");
>    }
>  };
>
> @@ -80,3 +84,54 @@ IN_PROC_BROWSER_TEST_F(**InstantExtendedTest,
> InputShowsOverlay) {
>    EXPECT_TRUE(instant()->model()**->mode().is_search_**suggestions());
>    EXPECT_EQ(preview_tab, instant()->GetPreviewContents(**));
>  }
> +
> +// Test that omnibox text is correctly set when overlay is committed with
> Enter.
> +IN_PROC_BROWSER_TEST_F(**InstantExtendedTest,
> OmniboxTextUponEnterCommit) {
> +  ASSERT_NO_FATAL_FAILURE(**SetupInstant());
> +  FocusOmniboxAndWaitForInstantS**upport();
> +
> +  // Set the text, and wait for suggestions to show up.
> +  SetOmniboxTextAndWaitForInstan**tToShow("santa");
> +  EXPECT_EQ(ASCIIToUTF16("santa"**), omnibox()->GetText());
> +
> +  // Set autocomplete text (grey text).
> +  EXPECT_TRUE(ExecuteScript(
> +      "window.chrome.searchBox.**setAutocompleteText('santa claus',
> 2)"));
> +
> +  // Make the InstantController think we are in Search Suggestions.
> +  chrome::search::Mode mode;
> +  mode.mode = chrome::search::Mode::MODE_**SEARCH_SUGGESTIONS;
> +  instant()->model()->**SetPreviewState(mode, 100, INSTANT_SIZE_PERCENT);
> +
> +  // Commit the overlay by pressing 'Enter'.
> +  instant()->CommitIfPossible(**INSTANT_COMMIT_PRESSED_ENTER);
> +
> +  // 'Enter' commits the query as it was typed.
> +  EXPECT_EQ(ASCIIToUTF16("santa"**), omnibox()->GetText());
> +}
> +
> +// Test that omnibox text is correctly set when overlay is committed with
> focus
> +// lost.
> +IN_PROC_BROWSER_TEST_F(**InstantExtendedTest,
> OmniboxTextUponFocusLostCommit**) {
> +  ASSERT_NO_FATAL_FAILURE(**SetupInstant());
> +  FocusOmniboxAndWaitForInstantS**upport();
> +
> +  // Set the text, and wait for suggestions to show up.
> +  SetOmniboxTextAndWaitForInstan**tToShow("johnny");
> +  EXPECT_EQ(ASCIIToUTF16("**johnny"), omnibox()->GetText());
> +
> +  // Set autocomplete text (grey text).
> +  EXPECT_TRUE(ExecuteScript(
> +      "window.chrome.searchBox.**setAutocompleteText('johnny depp',
> 2)"));
> +
> +  // Make the InstantController think we are in Search Suggestions.
> +  chrome::search::Mode mode;
> +  mode.mode = chrome::search::Mode::MODE_**SEARCH_SUGGESTIONS;
> +  instant()->model()->**SetPreviewState(mode, 100, INSTANT_SIZE_PERCENT);
> +
> +  // Commit the overlay by lost focus (e.g. clicking on the page).
> +  instant()->CommitIfPossible(**INSTANT_COMMIT_FOCUS_LOST);
> +
> +  // Search term extraction should kick in with the autocompleted text.
> +  EXPECT_EQ(ASCIIToUTF16("johnny depp"), omnibox()->GetText());
> +}
> Index: chrome/browser/instant/**instant_test_utils.cc
> diff --git a/chrome/browser/instant/**instant_test_utils.cc
> b/chrome/browser/instant/**instant_test_utils.cc
> index 56aed0f51122ebd3989a6f494f229b**859273ad63..**
> 194236319897976be474a3e32af5ef**39ea04e73d 100644
> --- a/chrome/browser/instant/**instant_test_utils.cc
> +++ b/chrome/browser/instant/**instant_test_utils.cc
> @@ -65,7 +65,12 @@ void InstantTestBase::**SetupInstantUsingTemplateURL()
> {
>    ui_test_utils::**WaitForTemplateURLServiceToLoa**d(service);
>
>    TemplateURLData data;
> - 
data.SetURL("http://does/not/**exist?q={searchTerms}<http://does/not/exist?q=%7BsearchTerms%7D>
> ");
> +  // Necessary to use exact URL for search term extraction to work in
> +  // InstantExtended.
> +  data.SetURL(instant_url_.spec(**) + "?espv=1&q={searchTerms}");
> +  data.alternate_urls.push_back(**instant_url_.spec() +
> +                                "?espv=1#q={searchTerms}");
> +  data.search_terms_replacement_**key = "espv";
>    data.instant_url = instant_url_.spec();
>
>    TemplateURL* template_url = new TemplateURL(browser()->**profile(),
> data);
> Index: chrome/browser/instant/**instant_test_utils.h
> diff --git a/chrome/browser/instant/**instant_test_utils.h
> b/chrome/browser/instant/**instant_test_utils.h
> index 7a1880a0393e0b6bf88ca818c947e5**545028173e..**
> 9816cfeae95d70fec78f3d3c2b2dcc**3a7ada1fec 100644
> --- a/chrome/browser/instant/**instant_test_utils.h
> +++ b/chrome/browser/instant/**instant_test_utils.h
> @@ -38,6 +38,18 @@ class InstantTestModelObserver : public
> InstantModelObserver {
>  };
>
>  class InstantTestBase : public InProcessBrowserTest {
> + public:
> +  InstantTestBase() {
> +    // Initialize (but not Start()) the test HTTPS server.
> +    net::TestServer::SSLOptions options;
> +    options.ocsp_status = net::TestServer::SSLOptions::**OCSP_OK;
> +    https_test_server_ = new net::TestServer(
> +        net::TestServer::TYPE_HTTPS,
> +        options,
> +        FilePath(FILE_PATH_LITERAL("**chrome/test/data")));
> +  }
> +
> +
>   protected:
>    void SetupInstant();
>    void SetupInstantUsingTemplateURL()**;
> @@ -72,6 +84,9 @@ class InstantTestBase : public InProcessBrowserTest {
>                           bool expected) WARN_UNUSED_RESULT;
>
>    GURL instant_url_;
> +
> +  // HTTPS Testing server, started on demand.
> +  net::TestServer* https_test_server_;
>  };
>
>  #endif  // CHROME_BROWSER_INSTANT_**INSTANT_TEST_UTILS_H_
>
>
>

[sreeram, 2013-02-01 22:03:24]

Thanks for the tests! I appreciate the effort!

https://codereview.chromium.org/12114050/diff/5001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/12114050/diff/5001/chrome/browser/instant/ins...
chrome/browser/instant/instant_extended_browsertest.cc:104:
instant()->model()->SetPreviewState(mode, 100, INSTANT_SIZE_PERCENT);
These lines (98-104) are hacking the public APIs too much. Sometimes it's
necessary, but wherever possible, I'd prefer to have the process work
end-to-end. I.e., the page should call setAutocompleteText()/show(100%) in
response to onchange() and onnativesuggestions().

https://codereview.chromium.org/12114050/diff/5001/chrome/browser/instant/ins...
chrome/browser/instant/instant_extended_browsertest.cc:110:
EXPECT_EQ(ASCIIToUTF16("santa"), omnibox()->GetText());
You should check that omnibox()->GetInstantSuggestion() has the expected values
(" claus" before the commit, and "" after the commit).

https://codereview.chromium.org/12114050/diff/5001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_test_utils.cc (right):

https://codereview.chromium.org/12114050/diff/5001/chrome/browser/instant/ins...
chrome/browser/instant/instant_test_utils.cc:70: data.SetURL(instant_url_.spec()
+ "?espv=1&q={searchTerms}");
I'd like to keep data.SetURL("http://does/not/exist?q={searchTerms}"). I think
search terms extraction works on alternate URLs, so it should suffice to set
alternate_urls to a real URL, as you have on the next line.

https://codereview.chromium.org/12114050/diff/5001/chrome/browser/instant/ins...
chrome/browser/instant/instant_test_utils.cc:72: "?espv=1#q={searchTerms}");
instant_url_.spec() already has "?espv=1" (in the case of
instant_extended_browsertest.cc), so aren't you adding it twice here?

https://codereview.chromium.org/12114050/diff/5001/chrome/browser/instant/ins...
chrome/browser/instant/instant_test_utils.cc:73:
data.search_terms_replacement_key = "espv";
Don't use "espv". Use some bogus value (here and in
instant_extended_browsertest.cc). Perhaps "strk" (acronym for "search terms
replacement key"). This will let us catch mistakes where we accidentally rely on
the actual string "espv" instead of using the correct
search_terms_replacement_key from the template.

https://codereview.chromium.org/12114050/diff/5001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_test_utils.h (right):

https://codereview.chromium.org/12114050/diff/5001/chrome/browser/instant/ins...
chrome/browser/instant/instant_test_utils.h:44: https_test_server_ = new
net::TestServer(
You could make this an initializer:
InstantTestBase()
    : https_test_server_(new net::TestServer(...)) {
}

This will let you make the pointer a const:

net::TestServer* const https_test_server_;

[Mathieu, 2013-02-04 20:37:36]

Thanks!

https://chromiumcodereview.appspot.com/12114050/diff/5001/chrome/browser/inst...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://chromiumcodereview.appspot.com/12114050/diff/5001/chrome/browser/inst...
chrome/browser/instant/instant_extended_browsertest.cc:104:
instant()->model()->SetPreviewState(mode, 100, INSTANT_SIZE_PERCENT);
On 2013/02/01 22:03:24, sreeram wrote:
> These lines (98-104) are hacking the public APIs too much. Sometimes it's
> necessary, but wherever possible, I'd prefer to have the process work
> end-to-end. I.e., the page should call setAutocompleteText()/show(100%) in
> response to onchange() and onnativesuggestions().

Done.

https://chromiumcodereview.appspot.com/12114050/diff/5001/chrome/browser/inst...
chrome/browser/instant/instant_extended_browsertest.cc:110:
EXPECT_EQ(ASCIIToUTF16("santa"), omnibox()->GetText());
On 2013/02/01 22:03:24, sreeram wrote:
> You should check that omnibox()->GetInstantSuggestion() has the expected
values
> (" claus" before the commit, and "" after the commit).

Done.

https://chromiumcodereview.appspot.com/12114050/diff/5001/chrome/browser/inst...
File chrome/browser/instant/instant_test_utils.cc (right):

https://chromiumcodereview.appspot.com/12114050/diff/5001/chrome/browser/inst...
chrome/browser/instant/instant_test_utils.cc:70: data.SetURL(instant_url_.spec()
+ "?espv=1&q={searchTerms}");
On 2013/02/01 22:03:24, sreeram wrote:
> I'd like to keep data.SetURL("http://does/not/exist?q={searchTerms}"). I think
> search terms extraction works on alternate URLs, so it should suffice to set
> alternate_urls to a real URL, as you have on the next line.

Unfortunately the template URLs are looked up by domain, so having a mismatch
between the "main" URL and the alternate makes it fail.

https://chromiumcodereview.appspot.com/12114050/diff/5001/chrome/browser/inst...
chrome/browser/instant/instant_test_utils.cc:72: "?espv=1#q={searchTerms}");
On 2013/02/01 22:03:24, sreeram wrote:
> instant_url_.spec() already has "?espv=1" (in the case of
> instant_extended_browsertest.cc), so aren't you adding it twice here?

Done.

https://chromiumcodereview.appspot.com/12114050/diff/5001/chrome/browser/inst...
chrome/browser/instant/instant_test_utils.cc:73:
data.search_terms_replacement_key = "espv";
On 2013/02/01 22:03:24, sreeram wrote:
> Don't use "espv". Use some bogus value (here and in
> instant_extended_browsertest.cc). Perhaps "strk" (acronym for "search terms
> replacement key"). This will let us catch mistakes where we accidentally rely
on
> the actual string "espv" instead of using the correct
> search_terms_replacement_key from the template.

Done.

https://chromiumcodereview.appspot.com/12114050/diff/5001/chrome/browser/inst...
File chrome/browser/instant/instant_test_utils.h (right):

https://chromiumcodereview.appspot.com/12114050/diff/5001/chrome/browser/inst...
chrome/browser/instant/instant_test_utils.h:44: https_test_server_ = new
net::TestServer(
On 2013/02/01 22:03:24, sreeram wrote:
> You could make this an initializer:
> InstantTestBase()
>     : https_test_server_(new net::TestServer(...)) {
> }
> 
> This will let you make the pointer a const:
> 
> net::TestServer* const https_test_server_;

Done.

[Mathieu, 2013-02-04 21:15:00]

InstantTest and InstantExtendedTest all pass now, thanks.

[sreeram, 2013-02-04 21:56:18]

lgtm

[commit-bot: I haz the power, 2013-02-04 22:08:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathp@chromium.org/12114050/14001

[commit-bot: I haz the power, 2013-02-04 23:39:36]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-05 00:14:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathp@chromium.org/12114050/14001

[commit-bot: I haz the power, 2013-02-05 01:58:58]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-05 12:58:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathp@chromium.org/12114050/14001

[commit-bot: I haz the power, 2013-02-05 14:39:10]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-05 14:53:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathp@chromium.org/12114050/14001

[commit-bot: I haz the power, 2013-02-05 16:44:48]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-05 17:54:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathp@chromium.org/12114050/14001

[commit-bot: I haz the power, 2013-02-05 19:41:52]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[sreeram, 2013-02-05 20:19:10]

https://codereview.chromium.org/12114050/diff/14001/chrome/browser/instant/in...
File chrome/browser/instant/instant_test_utils.h (right):

https://codereview.chromium.org/12114050/diff/14001/chrome/browser/instant/in...
chrome/browser/instant/instant_test_utils.h:85: net::TestServer* const
https_test_server_;
This should be scoped_ptr so that it gets freed correctly at the end of the
test. Thanks for pointing this out, Shishir!

[sreeram, 2013-02-05 20:38:24]

https://codereview.chromium.org/12114050/diff/14001/chrome/browser/instant/in...
File chrome/browser/instant/instant_test_utils.h (right):

https://codereview.chromium.org/12114050/diff/14001/chrome/browser/instant/in...
chrome/browser/instant/instant_test_utils.h:85: net::TestServer* const
https_test_server_;
On 2013/02/05 20:19:10, sreeram wrote:
> This should be scoped_ptr so that it gets freed correctly at the end of the
> test. Thanks for pointing this out, Shishir!

In fact, why bother with scoped_ptr. Just make it a good old data member:

net::TestServer https_test_server_;

[Mathieu, 2013-02-05 20:43:00]

Sending to CQ now.

https://chromiumcodereview.appspot.com/12114050/diff/14001/chrome/browser/ins...
File chrome/browser/instant/instant_test_utils.h (right):

https://chromiumcodereview.appspot.com/12114050/diff/14001/chrome/browser/ins...
chrome/browser/instant/instant_test_utils.h:85: net::TestServer* const
https_test_server_;
On 2013/02/05 20:38:24, sreeram wrote:
> On 2013/02/05 20:19:10, sreeram wrote:
> > This should be scoped_ptr so that it gets freed correctly at the end of the
> > test. Thanks for pointing this out, Shishir!
> 
> In fact, why bother with scoped_ptr. Just make it a good old data member:
> 
> net::TestServer https_test_server_;

Done. Thanks!

[commit-bot: I haz the power, 2013-02-05 20:43:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathp@chromium.org/12114050/37002

[commit-bot: I haz the power, 2013-02-05 20:58:35]

Failed to trigger a try job on linux_aura
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-02-05 20:58:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathp@chromium.org/12114050/43003

[commit-bot: I haz the power, 2013-02-06 01:59:40]

Change committed as 180856