New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 758703 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: ----
Type: ----



Sign in to add a comment

EmbeddedSearchAPIOnlyAvailableOnNTP times out on Windows 7.

Project Member Reported by fdoray@chromium.org, Aug 24 2017

Issue description

Project Member

Comment 1 by bugdroid1@chromium.org, Aug 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f71dd5dbdbadae253b7c1d2223eebb8f171704e2

commit f71dd5dbdbadae253b7c1d2223eebb8f171704e2
Author: Francois Doray <fdoray@chromium.org>
Date: Thu Aug 24 21:08:36 2017

Revert "Cleanup Local NTP browser tests"

This reverts commit 0c2810e9a90f125298b3c0d35f3e1a5e21cf092d.

Reason for revert:  https://crbug.com/758703 

Bug:  758703 

Original change's description:
> Cleanup Local NTP browser tests
> 
> This gets rid of the use of InstantTestBase in local NTP browser tests.
> InstantTestBase is confusing and not really needed.
> Additional cleanups:
> - Many tests now use the actual local NTP rather than local_ntp_browsertest.html.
> - Use ScopedFeatureList instead of manually setting cmdline params.
> - In LoadsWithoutError smoke tests, actually check that we got a [non-]Google NTP.
> 
> Bug: none
> Change-Id: I9924796ea179361fd5c297eea7f01d3bb3b39799
> Reviewed-on: https://chromium-review.googlesource.com/632236
> Commit-Queue: Marc Treib <treib@chromium.org>
> Reviewed-by: Chris Pickel <sfiera@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#497044}

TBR=treib@chromium.org,sfiera@chromium.org

Change-Id: I49dff2a1329e607e81c5753f8a32dd2c9415ac32
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: none
Reviewed-on: https://chromium-review.googlesource.com/634303
Reviewed-by: Francois Doray <fdoray@chromium.org>
Commit-Queue: Francois Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497188}
[modify] https://crrev.com/f71dd5dbdbadae253b7c1d2223eebb8f171704e2/chrome/browser/search/instant_service.h
[modify] https://crrev.com/f71dd5dbdbadae253b7c1d2223eebb8f171704e2/chrome/browser/ui/search/instant_controller.h
[modify] https://crrev.com/f71dd5dbdbadae253b7c1d2223eebb8f171704e2/chrome/browser/ui/search/instant_test_base.h
[modify] https://crrev.com/f71dd5dbdbadae253b7c1d2223eebb8f171704e2/chrome/browser/ui/search/local_ntp_browsertest.cc

Comment 2 by treib@chromium.org, Aug 25 2017

Components: UI>Browser>NewTabPage Tests
Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
It looks like my change might just have made the test slower for some reason: On the Win dbg bot, it always takes around 20s to complete. With this CL, there were a few runs where it first timed out, then passed in ~35s on the second try.
Now to find out what made it so much slower. Additionally, maybe the test should be split up anyway, since 20s is already kinda excessive.

Comment 3 by treib@chromium.org, Aug 25 2017

Status: Started (was: Available)

Comment 4 by treib@chromium.org, Aug 25 2017

Found the culprit: Navigating to chrome://settings is very slow on Windows debug. chrome://settings was just used as an arbitrary non-NTP URL, so I'll switch to some other URL instead.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1bbcd34ede823f908dcb6bc711dd1b803862cd5d

commit 1bbcd34ede823f908dcb6bc711dd1b803862cd5d
Author: Marc Treib <treib@chromium.org>
Date: Fri Aug 25 17:26:01 2017

Reland "Cleanup Local NTP browser tests"

This is a reland of 0c2810e9a90f125298b3c0d35f3e1a5e21cf092d

Changes from the original CL (which is patch set 1 here):
The original CL used "chrome://settings" as an arbitrary non-NTP page to
navigate to. Turns out loading "chrome://settings" is very slow on Windows
debug bots, causing the test to time out often.
Instead, this CL uses an arbitrary page from a test server, which loads much
faster.

Original change's description:
> Cleanup Local NTP browser tests
>
> This gets rid of the use of InstantTestBase in local NTP browser tests.
> InstantTestBase is confusing and not really needed.
> Additional cleanups:
> - Many tests now use the actual local NTP rather than local_ntp_browsertest.html.
> - Use ScopedFeatureList instead of manually setting cmdline params.
> - In LoadsWithoutError smoke tests, actually check that we got a [non-]Google NTP.
>
> Bug: none
> Change-Id: I9924796ea179361fd5c297eea7f01d3bb3b39799
> Reviewed-on: https://chromium-review.googlesource.com/632236
> Commit-Queue: Marc Treib <treib@chromium.org>
> Reviewed-by: Chris Pickel <sfiera@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#497044}

Bug:  758703 
Change-Id: I9b2b5b565c2eb631a29a62968fe6ccb5a00faeda
Reviewed-on: https://chromium-review.googlesource.com/635763
Reviewed-by: Chris Pickel <sfiera@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497449}
[modify] https://crrev.com/1bbcd34ede823f908dcb6bc711dd1b803862cd5d/chrome/browser/search/instant_service.h
[modify] https://crrev.com/1bbcd34ede823f908dcb6bc711dd1b803862cd5d/chrome/browser/ui/search/instant_controller.h
[modify] https://crrev.com/1bbcd34ede823f908dcb6bc711dd1b803862cd5d/chrome/browser/ui/search/instant_test_base.h
[modify] https://crrev.com/1bbcd34ede823f908dcb6bc711dd1b803862cd5d/chrome/browser/ui/search/local_ntp_browsertest.cc

Comment 6 by treib@chromium.org, Aug 25 2017

Status: Fixed (was: Started)

Sign in to add a comment