predictors: Basic browsertest checks predictor learning.

chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

Index: chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc b/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8a88a2b9638dcc55f17a5edbdb5a8ff1584894f0
--- /dev/null
+++ b/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
@@ -0,0 +1,200 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/command_line.h"
+#include "chrome/browser/predictors/resource_prefetch_predictor.h"
+#include "chrome/browser/predictors/resource_prefetch_predictor_factory.h"
+#include "chrome/browser/predictors/resource_prefetch_predictor_test_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
+#include "net/test/embedded_test_server/http_request.h"
+#include "net/test/embedded_test_server/http_response.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::UnorderedElementsAreArray;
+
+namespace predictors {
+
+static const char kImageUrl[] = "/predictors/image.png";
+static const char kStyleUrl[] = "/predictors/style.css";
+static const char kScriptUrl[] = "/predictors/script.js";
+static const char kFontUrl[] = "/predictors/font.ttf";
+static const char kHtmlSubresourcesUrl[] = "/predictors/html_subresources.html";
+
+using PageRequestSummary = ResourcePrefetchPredictor::PageRequestSummary;
+using URLRequestSummary = ResourcePrefetchPredictor::URLRequestSummary;
+using net::test_server::HttpRequest;
+using net::test_server::HttpResponse;
+using net::test_server::BasicHttpResponse;

[pasko, 2016/11/03 16:23:13]

the walls of 'using' statements are not adding too much convenience, especially
the ones that are used only once (like the all the net::test_server::* here),
but add a theoretical risk of causing name conflicts in future changes. Please
minimize the amount of type aliases to only leave those that are saving a lot of
line space.

[alexilin, 2016/11/03 18:42:12]

Done.

+
+struct ResourceSummary {
+  ResourceSummary() : is_no_store(false), version(0) {}
+
+  URLRequestSummary request;
+  std::string content;
+  bool is_no_store;
+  size_t version;
+};
+
+// Helper class to track and allow waiting for ResourcePrefetchPredictor events.

[pasko, 2016/11/02 19:37:02]

not only waiting, but also verifying that a PageRequestSummary is learned as
expected. That would be a valuable additions to the comment

[alexilin, 2016/11/03 13:49:08]

That's why I don't like comments: they become stale too fast.
Done.

+class ResourcePrefetchPredictorTestObserver : public TestObserver {
+ public:
+  explicit ResourcePrefetchPredictorTestObserver(
+      ResourcePrefetchPredictor* predictor,
+      const size_t expected_url_visit_count,
+      const PageRequestSummary& expected_summary)
+      : TestObserver(predictor),
+        message_loop_runner_(new content::MessageLoopRunner()),

[pasko, 2016/11/02 19:37:01]

using content::MessageLoopRunner without calling QuitClosure() is confusing
because base::RunLoop is sufficient to solve the problem. Please change to
base::RunLoop.

[alexilin, 2016/11/03 13:49:09]

MessageLoopRunner does additional work internally in comparison with
base::RunLoop. Don't we need it? 
I've thought that using MessageLoopRunner is common pattern for this kind of
test (even without calling QuitClosure).
Btw, base::RunLoop contains QuitClosure() method too, so this is not an
argument.
I'm not very familiar with this message running infrastructure yet, so it'd be
great if you explain me details when we should use MessageLoopRunner instead of
RunLoop.

[pasko, 2016/11/03 16:23:12]

Frankly, this code keeps changing and I lost track, so not an expert here
either. My conclusion is based on these observations:
0. RunLoop solves the problem just as elegantly
1. RunLoop is a commonly recognized pattern, used ~20x more than
MessageLoopRunner
2. MessageLoopRunner uses RunLoop under the scenes with extra 4x of state to
care about
3. chrome/browser/predictors/ is using RunLoop already, but MessageLoopRunner is
a new dependency

[alexilin, 2016/11/03 18:42:12]

Acknowledged.

+        url_visit_count_(expected_url_visit_count),
+        summary_(expected_summary) {}
+
+  ~ResourcePrefetchPredictorTestObserver() override {}

[pasko, 2016/11/02 19:37:02]

is this necessary? does not add to readability

[alexilin, 2016/11/03 13:49:09]

Done.

+
+  // ResourcePrefetchPredictor::Observer

[pasko, 2016/11/02 19:37:02]

// TestObserver implementation.

[alexilin, 2016/11/03 13:49:08]

Done.
Btw, I've seen alike comments with the word "implementation" and without, e.g.
"//content::WebContentsObserver:".

[pasko, 2016/11/03 16:23:13]

me too, also 'blah methods.'. What we do in cases like this, is aiming at being
more consistent with the code closely around. Your variant looks more consistent
with chrome/browser/ and chrome/browser/predictors:

$ git grep '// .*::[A-Za-z]* implementation[.]$' chrome/browser/ | wc -l
442
$ git grep '// .*::[A-Za-z]*:$' chrome/browser/ | wc -l
1310

So then I would suggest changing it to:
// TestObserver:

sorry about the previous misleading suggestion

[alexilin, 2016/11/03 18:42:12]

Actually, your suggestion wasn't completely misleading because I had stale class
name in comment (ResourcePrefetchPredictor::Observer)

+  void OnNavigationLearned(size_t url_visit_count,
+                           const PageRequestSummary& summary) override {
+    EXPECT_EQ(url_visit_count, url_visit_count_);
+    EXPECT_EQ(summary.main_frame_url, summary_.main_frame_url);
+    EXPECT_EQ(summary.initial_url, summary_.initial_url);
+    EXPECT_THAT(summary.subresource_requests,
+                UnorderedElementsAreArray(summary_.subresource_requests));
+    message_loop_runner_->Quit();
+  }
+
+  void Wait() { message_loop_runner_->Run(); }
+
+ private:
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+  size_t url_visit_count_;
+  PageRequestSummary summary_;
+
+  DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorTestObserver);
+};
+
+class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
+ public:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitchASCII(
+        switches::kSpeculativeResourcePrefetching,
+        switches::kSpeculativeResourcePrefetchingEnabled);
+  }
+
+  void SetUpOnMainThread() override {
+    embedded_test_server()->RegisterRequestHandler(
+        base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleRequest,
+                   base::Unretained(this)));
+    ASSERT_TRUE(embedded_test_server()->Start());
+    predictor_ =
+        ResourcePrefetchPredictorFactory::GetForProfile(browser()->profile());
+    ASSERT_TRUE(predictor_);
+  }
+
+  void NavigateToURLAndCheckSubresources(
+      const std::string& main_frame_relative) {
+    GURL main_frame_absolute =
+        embedded_test_server()->GetURL(main_frame_relative);
+    std::vector<URLRequestSummary> url_request_summaries;
+    for (const auto& kv : resources_) {
+      if (kv.second.is_no_store)
+        continue;
+      url_request_summaries.push_back(
+          GetURLRequestSummaryForResource(main_frame_absolute, kv.second));
+    }
+    ResourcePrefetchPredictorTestObserver observer(
+        predictor_, UpdateAndGetVisitCount(main_frame_relative),
+        CreatePageRequestSummary(main_frame_absolute.spec(),
+                                 main_frame_absolute.spec(),
+                                 url_request_summaries));
+    ui_test_utils::NavigateToURL(browser(), main_frame_absolute);
+    observer.Wait();
+  }
+
+  void AddResource(const std::string& relative_url,
+                   content::ResourceType resource_type,
+                   net::RequestPriority priority,
+                   const std::string& mime_type,
+                   bool has_validators = true,

[pasko, 2016/11/02 19:37:01]

the style guide "somewhat discourages" function overloading and default
arguments, see:

https://google.github.io/styleguide/cppguide.html#Function_Overloading

Once the amount of arguments reaches ~7,  maintaining the right order of
parameters becomes very error-prone: there are no compiler-enforced checks to
verify that all these booleans/ints/pointers are passed the way it was intended.
 
Since these default arguments are not used yet, let's omit them at the moment,
this would be simpler.

Later, makes sense to consider having a function to register a ResourceSummary
an return it. If an individual test cares about some particular parts of the
ResourceSummary, they can overwrite that particular default value of that
particular member.

Another option is o have multiple functions under different names to do a
similar thing with different set of arguments (just like the style guide
suggests).

Throwing in lots of default args is not a good option

[alexilin, 2016/11/03 13:49:09]

Many of the tests for ResourcePrefetchPredictors contains functions like this.
For example, look at Create* functions. I agree that they don't looks very good,
but on other hand these are for tests and objects could be constructed directly
so it's just for convenience.

Anyway, I think that the idea of returning ResourceSummary is good in this case.

[pasko, 2016/11/03 16:23:12]

Yes. I did not have a chance to object at the time they were introduced.
Looks nicer indeed.

[alexilin, 2016/11/03 18:42:12]

Acknowledged.

+                   bool always_revalidate = false,
+                   const std::string& content = std::string(),
+                   bool is_no_store = false) {
+    ResourceSummary resource;
+    GURL resource_url = embedded_test_server()->GetURL(relative_url);
+    resource.request = CreateURLRequestSummary(
+        -1, -1, std::string(), resource_url.spec(), resource_type, priority,
+        mime_type, false, std::string(), has_validators, always_revalidate);
+    resource.content = content;
+    resource.is_no_store = is_no_store;
+    resources_[relative_url] = resource;
+  }
+
+ private:
+  URLRequestSummary GetURLRequestSummaryForResource(
+      const GURL& main_frame_url,
+      const ResourceSummary& resource_summary) {
+    URLRequestSummary summary(resource_summary.request);
+    content::WebContents* web_contents =
+        browser()->tab_strip_model()->GetActiveWebContents();
+    int process_id = web_contents->GetRenderProcessHost()->GetID();
+    int frame_id = web_contents->GetMainFrame()->GetRoutingID();
+    summary.navigation_id =
+        CreateNavigationID(process_id, frame_id, main_frame_url.spec());
+    return summary;
+  }
+
+  std::unique_ptr<HttpResponse> HandleRequest(const HttpRequest& request) {

[pasko, 2016/11/02 19:37:02]

A few concerns with manually handling requests in this class:

1. it is a bit manual :) The test server can load files and serve custom
response headers, see for example:
https://codesearch.chromium.org/chromium/src/chrome/test/data/prerender/image...

2. sharing the state of the test fixture (resources_, predictor_,
what-may-come-later) with the test server makes it easy to overlook problems
that happen in reality by intercepting/rewriting all the request-response meat.

Proposal: rely on default handlers as much as possible. That would be a path to
the least surprise.

If we need something fancy we can still have handlers for those and return
nullptr if the path does not belong to them, the test server will just try the
next handlers and then the default handlers, see:

https://codesearch.chromium.org/chromium/src/net/test/embedded_test_server/em...

This proposal is, of course, a tradeoff, for example, it may lead to creating
more files and more custom headers, happy to discuss.

[alexilin, 2016/11/03 13:49:08]

I don't see anything special or wrong in manual handling.
Default request handler use the same functions for setting headers and other
stuff as I do.
https://cs.chromium.org/chromium/src/net/test/embedded_test_server/request_ha...
Creation of special header file for each subresource looks more sophisticated
than just add headers in accordance with function parameters in code. For
example, I could iterate over all possible combinations of parameters
programmatically with this method. Default handler has also the way to pass
custom headers via query parameter but it also doesn't look nice.

From other side, I'm not sure that we have to check predictor behavior depending
on cache-control directives but I find that it'd be nice.

Embedded HTTP server provides a neat and simple way to customize its behavior. I
don't see anything surprising in custom request handler.
I know that it's possible to return nullptr if I don't want to handle some
request and I already use it.

[pasko, 2016/11/03 16:23:12]

What do you mean by 'nice' in this case?
Having flexibility is a good point. I do not object to flexibility when we need
it, just asking to use more standard methods of solving the problem for
non-sophisticated cases. Less coupling of the server side and the test fixture
is also probably good, since we may want to introduce other test fixtures with
the same resource files, then we would want to avoid duplicating the logic in
this function.

With the way I proposed it would be easier to say what a test does by looking at
html and headers. As usual, this favors reading the code at the cost of less
convenient writing, which is a good tradeoff in most cases, because the code is
being read many more times than it is being written.

[alexilin, 2016/11/03 18:42:12]

Pardon, I had in mind "expected_body" parameter but it has different meaning.
1. It's not possible to test redirects with default handler so we'll anyway have
to write a custom one.
2. For many resources I don't interested in theirs content. I just want to get
HTTP_OK response on it (probably with custom headers).
3. Coupling of the server side and the test fixture allows to control server and
test expectations from one place. It leads to less code and more concentration
on the real problem.
4. The custom handler allows to create infinite number of "resources" with
different names and the same content (which not matters) programmatically. Name
matters in that case because it's a key for predictor and cache. Of course, we
can have some manipulations with files from test instead of it but why?
I don't think that my custom handler is so sophisticated.
I admit that it could be surprising to see image.png link in .html file but not
to see image.png in the directory. But there already are browsertests that use
custom handlers.

Anyway, we need to come to a final decision. Your suggestion was to use default
handler and create separate files for each resource + for each custom header (if
needed). If you insist I'll implement it.

[pasko, 2016/11/04 15:05:34]

What makes you say that? How about:

chrome/test/data/prerender/image-redirect.png.mock-http-headers

[alexilin, 2016/11/04 16:07:52]

Oh, I didn't know that HTTP response code is a part of the headers in this
universe.
My bad, sorry.

+    auto resource_it = resources_.find(request.relative_url);
+    if (resource_it == resources_.end())
+      return nullptr;
+
+    const ResourceSummary& summary = resource_it->second;
+    std::unique_ptr<BasicHttpResponse> http_response =

[pasko, 2016/11/03 16:23:12]

nit: since the type should be obvious: auto http_response = ...

[alexilin, 2016/11/03 18:42:12]

Done.

+        base::MakeUnique<BasicHttpResponse>();
+    http_response->set_code(net::HTTP_OK);
+    http_response->set_content_type(summary.request.mime_type);
+    http_response->set_content(summary.content);
+    if (summary.is_no_store)
+      http_response->AddCustomHeader("Cache-Control", "no-store");
+    if (summary.request.has_validators) {
+      http_response->AddCustomHeader(
+          "ETag", base::StringPrintf("'%zu%s'", summary.version,
+                                     request.relative_url.c_str()));
+    }
+    if (summary.request.always_revalidate)
+      http_response->AddCustomHeader("Cache-Control", "no-cache");
+    else
+      http_response->AddCustomHeader("Cache-Control", "max-age=2147483648");
+    return std::move(http_response);
+  }
+
+  size_t UpdateAndGetVisitCount(const std::string& main_frame_relative) {
+    return ++visit_count_[main_frame_relative];
+  }
+
+  ResourcePrefetchPredictor* predictor_;
+  std::map<std::string, ResourceSummary> resources_;
+  std::map<std::string, size_t> visit_count_;

[pasko, 2016/11/02 19:37:01]

This value gets updated, but the test only observes the value 1. It is not
obvious to me how  other tests will handle multiple navigations and whether it
would add much value there to check for visit count.

It is preferable to introduce things on as-needed basis, otherwise we risk
forgetting to remove dead code it the future reviews find other ways to perform
the same checks.

[alexilin, 2016/11/03 13:49:09]

In future tests we definitely will navigate to the same url more than once to
check that prefetch really works.
And in this way we also could check that prefetch doesn't affect history namely
that we count a visit exactly once regardless of whether or not the prefetch
was.

[pasko, 2016/11/03 16:23:13]

I see, makes sense, thanks.

[alexilin, 2016/11/03 18:42:12]

Acknowledged.

+};
+
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, ) {

[pasko, 2016/11/02 19:37:01]

name of the test missing, suggest: ResourcePrefetchPredictorBrowserTest.Simple

[alexilin, 2016/11/03 13:49:09]

Wow, how does it even work? Yeah, I see GTest uses this name only as parameter
of ## macro operator (and #).
Done.

+  // These resources have default priorities that correspond to
+  // blink::typeToPriority function.
+  AddResource(kImageUrl, content::RESOURCE_TYPE_IMAGE, net::LOWEST,
+              "image/png");

[pasko, 2016/11/04 15:05:34]

let's try to remove setting the mime types returned by the server, if that
breaks somewhere, it would be interesting to know why, otherwise just less code.
Testing mime type recognition on this end-to-end test level is not a goal, it
should be covered by the predictor unittest for
ResourcePrefetchPredictor::IsHandledResourceType().

[alexilin, 2016/11/04 16:07:52]

Done.

+  AddResource(kStyleUrl, content::RESOURCE_TYPE_STYLESHEET, net::HIGHEST,
+              "text/css");
+  AddResource(kScriptUrl, content::RESOURCE_TYPE_SCRIPT, net::MEDIUM,
+              "application/javascript");
+  AddResource(kFontUrl, content::RESOURCE_TYPE_FONT_RESOURCE, net::HIGHEST,
+              "application/x-font-truetype");
+  NavigateToURLAndCheckSubresources(kHtmlSubresourcesUrl);
+}
+
+}  // namespace predictors