New issue
Advanced search Search tips

Issue 650253 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 631966



Sign in to add a comment

Add browser tests for the resource_prefetch_predictor

Project Member Reported by lizeb@chromium.org, Sep 26 2016

Issue description

There are currently only unit tests, this needs higher level ones as well.
 
Status: Started (was: Assigned)
Status: Assigned (was: Started)
Labels: -Pri-2 Pri-1
Status: Started (was: Assigned)
These tests is necessary to ensure that ResourcePrefetchPredictor really works and fetches pages subresources before first bytes of a main page arrive.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 4 2016

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

commit e636e3bcc172f220dfadfd2121d238c9908185bb
Author: alexilin <alexilin@chromium.org>
Date: Fri Nov 04 19:07:18 2016

predictors: Basic browsertest checks predictor learning.

First stage for implementing ResourcePrefetchPredictor browsertests is to check
that the prefetch database is really filled after some navigations in browser.
Browsertest class provides convenient way to declare expected resources in the
test and then checks expectations internally that allows to write readable
tests.

BUG= 650253 

Review-Url: https://codereview.chromium.org/2449323005
Cr-Commit-Position: refs/heads/master@{#429966}

[add] https://crrev.com/e636e3bcc172f220dfadfd2121d238c9908185bb/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
[modify] https://crrev.com/e636e3bcc172f220dfadfd2121d238c9908185bb/chrome/browser/predictors/resource_prefetch_predictor_test_util.cc
[modify] https://crrev.com/e636e3bcc172f220dfadfd2121d238c9908185bb/chrome/browser/predictors/resource_prefetch_predictor_test_util.h
[modify] https://crrev.com/e636e3bcc172f220dfadfd2121d238c9908185bb/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
[modify] https://crrev.com/e636e3bcc172f220dfadfd2121d238c9908185bb/chrome/test/BUILD.gn
[add] https://crrev.com/e636e3bcc172f220dfadfd2121d238c9908185bb/chrome/test/data/predictors/html_subresources.html
[modify] https://crrev.com/e636e3bcc172f220dfadfd2121d238c9908185bb/testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 5 2016

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

commit 3250e71949fa7d226e9c843ee28277b988306229
Author: mathp <mathp@chromium.org>
Date: Sat Nov 05 03:11:19 2016

Revert of predictors: Basic browsertest checks predictor learning. (patchset #9 id:160001 of https://codereview.chromium.org/2449323005/ )

Reason for revert:
Created a flaky test: first occurrence at

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%281%29/builds/28933

Original issue's description:
> predictors: Basic browsertest checks predictor learning.
>
> First stage for implementing ResourcePrefetchPredictor browsertests is to check
> that the prefetch database is really filled after some navigations in browser.
> Browsertest class provides convenient way to declare expected resources in the
> test and then checks expectations internally that allows to write readable
> tests.
>
> BUG= 650253 
>
> Committed: https://crrev.com/e636e3bcc172f220dfadfd2121d238c9908185bb
> Cr-Commit-Position: refs/heads/master@{#429966}

TBR=pasko@chromium.org,lizeb@chromium.org,alexilin@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 650253 

Review-Url: https://codereview.chromium.org/2470143007
Cr-Commit-Position: refs/heads/master@{#430137}

[delete] https://crrev.com/fe64f860fd727cae179210713faee87b6c9e9209/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
[modify] https://crrev.com/3250e71949fa7d226e9c843ee28277b988306229/chrome/browser/predictors/resource_prefetch_predictor_test_util.cc
[modify] https://crrev.com/3250e71949fa7d226e9c843ee28277b988306229/chrome/browser/predictors/resource_prefetch_predictor_test_util.h
[modify] https://crrev.com/3250e71949fa7d226e9c843ee28277b988306229/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
[modify] https://crrev.com/3250e71949fa7d226e9c843ee28277b988306229/chrome/test/BUILD.gn
[delete] https://crrev.com/fe64f860fd727cae179210713faee87b6c9e9209/chrome/test/data/predictors/html_subresources.html
[modify] https://crrev.com/3250e71949fa7d226e9c843ee28277b988306229/testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 8 2016

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

commit f2d707775a8184e7bd18d2334ff2a679f3a90439
Author: alexilin <alexilin@chromium.org>
Date: Tue Nov 08 10:01:15 2016

Reland of predictors: Basic browsertest checks predictor learning.

Reason for reland:
The reason of the test flakiness was fixed.

ResourcePrefetchPredictor needs to be initialized before learn any
data. Initialization starts when the first main frame load complete for
given profile. In our case it was about:blank page after tab creation.
This initialization requires work in DB thread to be done. So there was
a race condition in which next navigation could appear earlier than
initialization in DB thread will be done.
This bug could be easily reproduced by adding sleep in
ResourcePrefetchPredictorTables::GetAllData function.

For now we explicitly check that initialization of
ResourcePrefetchPredictor was done before any navigation.

Original issue's description:
> Revert of predictors: Basic browsertest checks predictor learning. (patchset #9 id:160001 of https://codereview.chromium.org/2449323005/ )
>
> Reason for revert:
> Created a flaky test: first occurrence at
>
> https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%281%29/builds/28933
>
> Original issue's description:
> > predictors: Basic browsertest checks predictor learning.
> >
> > First stage for implementing ResourcePrefetchPredictor browsertests is to check
> > that the prefetch database is really filled after some navigations in browser.
> > Browsertest class provides convenient way to declare expected resources in the
> > test and then checks expectations internally that allows to write readable
> > tests.
> >
> > BUG= 650253 
> >
> > Committed: https://crrev.com/e636e3bcc172f220dfadfd2121d238c9908185bb
> > Cr-Commit-Position: refs/heads/master@{#429966}
>
> TBR=pasko@chromium.org,lizeb@chromium.org,alexilin@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 650253 
>
> Committed: https://crrev.com/3250e71949fa7d226e9c843ee28277b988306229
> Cr-Commit-Position: refs/heads/master@{#430137}

BUG= 650253 

Review-Url: https://codereview.chromium.org/2482833002
Cr-Commit-Position: refs/heads/master@{#430561}

[modify] https://crrev.com/f2d707775a8184e7bd18d2334ff2a679f3a90439/chrome/browser/predictors/resource_prefetch_predictor.cc
[modify] https://crrev.com/f2d707775a8184e7bd18d2334ff2a679f3a90439/chrome/browser/predictors/resource_prefetch_predictor.h
[add] https://crrev.com/f2d707775a8184e7bd18d2334ff2a679f3a90439/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
[modify] https://crrev.com/f2d707775a8184e7bd18d2334ff2a679f3a90439/chrome/browser/predictors/resource_prefetch_predictor_test_util.cc
[modify] https://crrev.com/f2d707775a8184e7bd18d2334ff2a679f3a90439/chrome/browser/predictors/resource_prefetch_predictor_test_util.h
[modify] https://crrev.com/f2d707775a8184e7bd18d2334ff2a679f3a90439/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
[modify] https://crrev.com/f2d707775a8184e7bd18d2334ff2a679f3a90439/chrome/test/BUILD.gn
[add] https://crrev.com/f2d707775a8184e7bd18d2334ff2a679f3a90439/chrome/test/data/predictors/html_subresources.html
[modify] https://crrev.com/f2d707775a8184e7bd18d2334ff2a679f3a90439/testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter

Patched version of browsertests is still a little flaky. (see flakiness dashboard http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=browser_tests&tests=ResourcePrefetchPredictorBrowserTest.*)
There are three different sources of flakiness:
1. The browser made request to the same image twice (then expectations fails):
ChromiumOS: https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20(1)/builds/29128/steps/browser_tests%20on%20Ubuntu-12.04/logs/stdio
Mac: https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/31477/steps/browser_tests%20on%20Mac-10.9/logs/stdio
Win: https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20(1)/builds/59730/steps/browser_tests%20on%20Windows-7-SP1/logs/stdio
2. Request to image had a higher priority than we expected:
Mac: https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/8928/steps/browser_tests%20on%20Mac-10.10/logs/stdio
Win: https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20(dbg)(1)/builds/54504/steps/browser_tests%20on%20Windows-7-SP1/logs/stdio
3. Unknown timeout error in the run where navigation didn't even started:
Linux: https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/48689/steps/browser_tests%20on%20Ubuntu-12.04/logs/stdio

It seems like  issue 3 . is a tester problem but 1. 2. are real. I'm not sure how critical it is because it happens very rare. 
For now I don't know how to change our expectations in tests to avoid these problems (except to not check net::RequestPriority at all that helps with 2.).

Comment 8 by lizeb@chromium.org, Nov 10 2016

Cc: -alexilin@chromium.org lizeb@chromium.org
Owner: alexilin@chromium.org

Comment 9 by pasko@chromium.org, Nov 14 2016

Cc: pasko@chromium.org
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 22 2016

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

commit bf5b8e0f7794b08c10af2bb682bee81978df84c5
Author: alexilin <alexilin@chromium.org>
Date: Tue Nov 22 18:58:19 2016

predictors: Add browsertest that checks redirects.

Make sure that ResourcePrefetchPredictor learns about subresources when a main
resource has redirects.

BUG= 650253 

Review-Url: https://codereview.chromium.org/2519963002
Cr-Commit-Position: refs/heads/master@{#433932}

[modify] https://crrev.com/bf5b8e0f7794b08c10af2bb682bee81978df84c5/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 29 2016

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

commit 3fbb26424d252cec6fb1a20f94f47a6211dcf80e
Author: alexilin <alexilin@chromium.org>
Date: Tue Nov 29 10:38:11 2016

predictors: Ignore repeating subresources while checking.

This eliminates one of the sources of flakiness in
ResourcePrefetchPredictorBrowserTest.
It seems impossible to avoid an emergence of several requests belonging to the
same subresource but we could just ignore them as real code does.
The real code also ignores repeating entries of the same subresource but doesn't
modify collection passing to test observer.

BUG= 650253 

Review-Url: https://codereview.chromium.org/2529263003
Cr-Commit-Position: refs/heads/master@{#434951}

[modify] https://crrev.com/3fbb26424d252cec6fb1a20f94f47a6211dcf80e/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Nov 30 2016

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

commit 2ccb52078094f6e0af7384ace1e4f14237a193b0
Author: alexilin <alexilin@chromium.org>
Date: Wed Nov 30 14:13:41 2016

predictors: Delete unnecessary parameter from RequestHandler

We don't need to pass the server as parameter to RequestHandler since
crrev.com/2522663004 was landed.

BUG= 650253 

Review-Url: https://codereview.chromium.org/2545483002
Cr-Commit-Position: refs/heads/master@{#435252}

[modify] https://crrev.com/2ccb52078094f6e0af7384ace1e4f14237a193b0/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Dec 6 2016

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

commit 7d03e6b973c673b3fcfd24c3e962f59875078fb8
Author: alexilin <alexilin@chromium.org>
Date: Tue Dec 06 10:43:59 2016

predictors: Add browsertests that check fetching through javascript.

Make sure that ResourcePrefetchPredictor awares about javascript initiated
requests + test for iframe.

BUG= 650253 

Review-Url: https://codereview.chromium.org/2540803002
Cr-Commit-Position: refs/heads/master@{#436566}

[modify] https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
[add] https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8/chrome/test/data/predictors/append_child.html
[add] https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8/chrome/test/data/predictors/append_child.js
[add] https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8/chrome/test/data/predictors/document_write.html
[add] https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8/chrome/test/data/predictors/document_write.js
[add] https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8/chrome/test/data/predictors/html_iframe.html
[modify] https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8/chrome/test/data/predictors/html_subresources.html
[add] https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8/chrome/test/data/predictors/inner_html.html
[add] https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8/chrome/test/data/predictors/inner_html.js
[add] https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8/chrome/test/data/predictors/xhr.html
[add] https://crrev.com/7d03e6b973c673b3fcfd24c3e962f59875078fb8/chrome/test/data/predictors/xhr.js

Project Member

Comment 14 by bugdroid1@chromium.org, Dec 12 2016

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

commit 23b1517fb1deb148503f67f1bc9f38283f4e89a0
Author: ahemery <ahemery@chromium.org>
Date: Mon Dec 12 15:29:49 2016

Summary
-----
Allows for custom comparison of results and exotic navigation setup.

Details
-----
- New browser tests will be needed to test edge cases. For example verifying behavior during cross tab navigation. Useful for regression with PlzNavigate.
- To be able to add the test cases we need to have some more customization
than what the current test framework provides. Specifically, partial
comparison and new navigation types. Implemented that.

BUG= 650253 

Review-Url: https://codereview.chromium.org/2561493003
Cr-Commit-Position: refs/heads/master@{#437877}

[modify] https://crrev.com/23b1517fb1deb148503f67f1bc9f38283f4e89a0/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Dec 12 2016

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

commit 54585e4c963fadabad06fd785ba30df548f9ade6
Author: alexilin <alexilin@chromium.org>
Date: Mon Dec 12 23:38:58 2016

predictors: Fuzzy comparison of request priority for images.

The browser tests for ResourcePrefetchPredictor check that the actual
request priority is equal to the expected value. But for the images the
priority cannot be precisely defined because there is a race between the
preload scanner issuing fetch requests and frame view performing layout
and promoting priorities of all visible images.

BUG= 650253 , 673028 

Review-Url: https://codereview.chromium.org/2569723002
Cr-Commit-Position: refs/heads/master@{#437958}

[modify] https://crrev.com/54585e4c963fadabad06fd785ba30df548f9ade6/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Dec 19 2016

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

commit c7c1e0389fa3b0d68e9b940b0929e2e3dd4bde9a
Author: alexilin <alexilin@chromium.org>
Date: Mon Dec 19 17:31:28 2016

predictors: Add browsertest that tests prefetching.

Before this we only had tests for filling predictor database. This CL
adds test that prefills database and then activates prefetching on the
same web page. Then it checks that cache actually has been populated by
prefetched resources.

BUG= 650253 

Review-Url: https://codereview.chromium.org/2553083002
Cr-Commit-Position: refs/heads/master@{#439494}

[modify] https://crrev.com/c7c1e0389fa3b0d68e9b940b0929e2e3dd4bde9a/chrome/browser/predictors/resource_prefetch_predictor.cc
[modify] https://crrev.com/c7c1e0389fa3b0d68e9b940b0929e2e3dd4bde9a/chrome/browser/predictors/resource_prefetch_predictor.h
[modify] https://crrev.com/c7c1e0389fa3b0d68e9b940b0929e2e3dd4bde9a/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
[modify] https://crrev.com/c7c1e0389fa3b0d68e9b940b0929e2e3dd4bde9a/chrome/browser/predictors/resource_prefetcher_manager.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Dec 22 2016

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

commit f15203e94ad5245c9ab50fbc0e1385ef403d0903
Author: alexilin <alexilin@chromium.org>
Date: Thu Dec 22 11:09:33 2016

predictors: Wait for the cache cleaning synchronously.

chrome::ClearCache() is asynchronous so there was possibility that cache is
cleared after prefetching causing flakiness.
This problem was almost hidden by that fact that the last navigation (after
prefetching) also caused prefetching. So the probability that cache is cleared
after the second prefetching was even less.
This CL also changes command line flag to disable the second prefetching because
it only tangles the tests.

BUG= 650253 

Review-Url: https://codereview.chromium.org/2596893002
Cr-Commit-Position: refs/heads/master@{#440379}

[modify] https://crrev.com/f15203e94ad5245c9ab50fbc0e1385ef403d0903/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Dec 27 2016

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

commit 820fc0653f57cfa4d75396b2606116430cb67aeb
Author: alexilin <alexilin@chromium.org>
Date: Tue Dec 27 08:20:53 2016

predictors: ResourcePrefetchPredictorBrowserTest improvements.

This CL contains 4 minor changes bundled together.
- ResourceSummary flags refactoring. Now they have more clean and strict
meanings + additional descriptions in comments.
- New check at HTTP server side that there is no resource requests after
prefetch making the tests more strict.
- New test with "Cache-control: no-cache" resources.
- Respectful handling of validation requests.

BUG= 650253 

Review-Url: https://codereview.chromium.org/2592243003
Cr-Commit-Position: refs/heads/master@{#440740}

[modify] https://crrev.com/820fc0653f57cfa4d75396b2606116430cb67aeb/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Jan 9 2017

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

commit 7e5dcb16fb03140fb729fbf58b03bb02ebe9df0e
Author: alexilin <alexilin@chromium.org>
Date: Mon Jan 09 16:19:59 2017

predictors: browser test for client-side redirect.

ResourcePrefetchPredictor doesn't receive the information from observers about
redirects initiated from javascript. However such redirects initiate the new
navigation that treated by predictor as usual.
This browser test documents the current behavior that could be a subject of
change.

Also, this CL changes all existing redirect tests to be cross-host. Redirect to
the same host could be ambiguous because of host-based learning.

BUG= 650253 

Review-Url: https://codereview.chromium.org/2609403007
Cr-Commit-Position: refs/heads/master@{#442259}

[modify] https://crrev.com/7e5dcb16fb03140fb729fbf58b03bb02ebe9df0e/chrome/browser/predictors/resource_prefetch_predictor.h
[modify] https://crrev.com/7e5dcb16fb03140fb729fbf58b03bb02ebe9df0e/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
[modify] https://crrev.com/7e5dcb16fb03140fb729fbf58b03bb02ebe9df0e/chrome/test/data/predictors/html_subresources.html
[add] https://crrev.com/7e5dcb16fb03140fb729fbf58b03bb02ebe9df0e/chrome/test/data/predictors/javascript_redirect.html
[add] https://crrev.com/7e5dcb16fb03140fb729fbf58b03bb02ebe9df0e/chrome/test/data/predictors/javascript_redirect.js

Status: Fixed (was: Started)
Mark this as done because all basic use-cases were covered.

Sign in to add a comment