InstantExtended: tests!

Scaffolding and very basic tests for Instant Extended.


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

[samarth, 2012-12-21 23:43:13]

Here's a first cut at tests. Obviously, there's a lot more to test here, but
wanted to get this in so others can also start writing tests.

Thanks,
Samarth

[samarth, 2013-01-02 18:06:31]

s/sky/sreeram/

Sreeram is back, woohoo.

[sreeram, 2013-01-02 18:12:10]

https://codereview.chromium.org/11592004/diff/2001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11592004/diff/2001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:179: void SetTestingMode();
I'd prefer instead to have the tests set --instant-url, which would also bypass
the HTTPS check. The general idea being to keep test-specific hooks as minimal
as possible; something like |in_testing_mode_| is too pervasive for my taste.

[samarth, 2013-01-02 23:35:15]

https://codereview.chromium.org/11592004/diff/2001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11592004/diff/2001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:179: void SetTestingMode();
On 2013/01/02 18:12:10, sreeram wrote:
> I'd prefer instead to have the tests set --instant-url, which would also
bypass
> the HTTPS check. The general idea being to keep test-specific hooks as minimal
> as possible; something like |in_testing_mode_| is too pervasive for my taste.

Ah yes, forget about that. I agree: that's much better.

We should probably still have at least one test that exercises the TemplateURL
path -- added such a test to the regular Instant tests.

[sreeram, 2013-01-08 01:10:08]

IIRC, if you don't install any TemplateURL at all, then when Update() is called,
template_url will be blank. This means GetInstantURL() will return false (which
is intended).

So, I suspect, the tests will fail as is. If so, the fix is simple: Just call
SetupInstantUsingTemplateURL() from within SetupInstant().

https://codereview.chromium.org/11592004/diff/8001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_browsertest.cc (right):

https://codereview.chromium.org/11592004/diff/8001/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:89: // Test that Instant works
when the url set via a TemplateURL (as opposed to
Typo: url set -> URL is set

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

https://codereview.chromium.org/11592004/diff/8001/chrome/browser/instant/ins...
chrome/browser/instant/instant_extended_browsertest.cc:67:
IN_PROC_BROWSER_TEST_F(InstantExtendedTest, InputShowsDropdown) {
Nit: dropdown -> overlay (or preview), here and below.

[samarth, 2013-01-14 23:59:46]

On 2013/01/08 01:10:08, sreeram wrote:
> IIRC, if you don't install any TemplateURL at all, then when Update() is
called,
> template_url will be blank. This means GetInstantURL() will return false
(which
> is intended).
> 
> So, I suspect, the tests will fail as is. If so, the fix is simple: Just call
> SetupInstantUsingTemplateURL() from within SetupInstant().

Hmm, turns this isn't necessary for any of the tests except for the recently
added InstantTest.History.  Made the change though.

[samarth, 2013-01-14 23:59:59]

https://codereview.chromium.org/11592004/diff/8001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_browsertest.cc (right):

https://codereview.chromium.org/11592004/diff/8001/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:89: // Test that Instant works
when the url set via a TemplateURL (as opposed to
On 2013/01/08 01:10:08, sreeram wrote:
> Typo: url set -> URL is set

Done.

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

https://codereview.chromium.org/11592004/diff/8001/chrome/browser/instant/ins...
chrome/browser/instant/instant_extended_browsertest.cc:67:
IN_PROC_BROWSER_TEST_F(InstantExtendedTest, InputShowsDropdown) {
On 2013/01/08 01:10:08, sreeram wrote:
> Nit: dropdown -> overlay (or preview), here and below.

Done.

[sreeram, 2013-01-17 23:54:18]

https://codereview.chromium.org/11592004/diff/16001/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11592004/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:1: // Copyright 2012 The
Chromium Authors. All rights reserved.
2012 -> 2013.

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

https://codereview.chromium.org/11592004/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_test_utils.h:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
2012 -> 2013

https://codereview.chromium.org/11592004/diff/16001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/11592004/diff/16001/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:185: 'browser/instant/instant_extended_browsertest.cc',
You should add instant_test_utils.h also to this list.

https://codereview.chromium.org/11592004/diff/16001/chrome/test/data/instant_...
File chrome/test/data/instant_extended.html (right):

https://codereview.chromium.org/11592004/diff/16001/chrome/test/data/instant_...
chrome/test/data/instant_extended.html:3: <script
src="instant_extended.js"></script>
Just inline the script here, and get rid of the separate file for the JS. The
HTML parts of this file are never going to become much bigger, so it will just
be a container for the script, which is fine (instant.html is written the same
way).

Perhaps you made the JS a separate file for editing (syntax highlighting)? At
least vim does the right thing even when the JS is inlined.

https://codereview.chromium.org/11592004/diff/16001/chrome/test/data/instant_...
File chrome/test/data/instant_extended.js (right):

https://codereview.chromium.org/11592004/diff/16001/chrome/test/data/instant_...
chrome/test/data/instant_extended.js:18: console.log('native');
Remove this? (I assume you added it for debugging.)

[samarth, 2013-01-18 00:06:45]

https://codereview.chromium.org/11592004/diff/16001/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11592004/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:1: // Copyright 2012 The
Chromium Authors. All rights reserved.
On 2013/01/17 23:54:18, sreeram wrote:
> 2012 -> 2013.

Done.

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

https://codereview.chromium.org/11592004/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_test_utils.h:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
On 2013/01/17 23:54:18, sreeram wrote:
> 2012 -> 2013

Done.

https://codereview.chromium.org/11592004/diff/16001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/11592004/diff/16001/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:185: 'browser/instant/instant_extended_browsertest.cc',
On 2013/01/17 23:54:18, sreeram wrote:
> You should add instant_test_utils.h also to this list.

Done.

https://codereview.chromium.org/11592004/diff/16001/chrome/test/data/instant_...
File chrome/test/data/instant_extended.html (right):

https://codereview.chromium.org/11592004/diff/16001/chrome/test/data/instant_...
chrome/test/data/instant_extended.html:3: <script
src="instant_extended.js"></script>
On 2013/01/17 23:54:18, sreeram wrote:
> Just inline the script here, and get rid of the separate file for the JS. The
> HTML parts of this file are never going to become much bigger, so it will just
> be a container for the script, which is fine (instant.html is written the same
> way).
> 
> Perhaps you made the JS a separate file for editing (syntax highlighting)? At
> least vim does the right thing even when the JS is inlined.

Yeah, that's why, but inlined it.

https://codereview.chromium.org/11592004/diff/16001/chrome/test/data/instant_...
File chrome/test/data/instant_extended.js (right):

https://codereview.chromium.org/11592004/diff/16001/chrome/test/data/instant_...
chrome/test/data/instant_extended.js:18: console.log('native');
On 2013/01/17 23:54:18, sreeram wrote:
> Remove this? (I assume you added it for debugging.)

Done.

[sreeram, 2013-01-18 16:26:35]

LGTM

Please merge the changes from http://crrev.com/177433, http://crrev.com/177424
and http://crrev.com/177021.

[samarth, 2013-01-18 19:50:12]

Rebased and sending to CQ.  Thanks for for the reviews!

[commit-bot: I haz the power, 2013-01-18 19:51:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/11592004/22003

[commit-bot: I haz the power, 2013-01-18 20:20:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-18 21:09:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/11592004/28009

[commit-bot: I haz the power, 2013-01-18 23:32:09]

Change committed as 177798