Improve testing for SDCH.

net/url_request/url_request_http_job_unittest.cc

Index: net/url_request/url_request_http_job_unittest.cc
diff --git a/net/url_request/url_request_http_job_unittest.cc b/net/url_request/url_request_http_job_unittest.cc
index 203d6d077da9e7841eaf023fa530b5669470ef03..437458f208b932f8f82db669bf81558f6e637748 100644
--- a/net/url_request/url_request_http_job_unittest.cc
+++ b/net/url_request/url_request_http_job_unittest.cc
@@ -54,6 +54,37 @@ class URLRequestHttpJobTest : public ::testing::Test {
     context_.set_http_transaction_factory(&network_layer_);
   }
 
+  bool TransactionAcceptsSdchEncoding() {
+    base::WeakPtr<MockNetworkTransaction> xtion(

[mef, 2014/07/23 18:34:30]

nit: maybe name it |transaction|?

[Randy Smith (Not in Mondays), 2014/07/29 23:44:22]

Done.

+        network_layer_.last_transaction());
+    EXPECT_TRUE(xtion);
+    if (!xtion) return false;
+
+    const HttpRequestInfo* request_info = xtion->http_request_info();
+    EXPECT_TRUE(request_info);
+    if (!request_info) return false;
+
+    std::string encoding_headers;
+    bool get_success = request_info->extra_headers.GetHeader(
+        "Accept-Encoding", &encoding_headers);
+    EXPECT_TRUE(get_success);

[mef, 2014/07/23 18:34:30]

nit: replace with ASSERT?

[Randy Smith (Not in Mondays), 2014/07/29 23:44:22]

See comment elsewhere in this review--I don't think that works in helper
routines.

[mef, 2014/07/30 17:14:58]

Hmm, I thought that the only difference between EXPECT_TRUE and ASSERT_TRUE is
fatality.

[Randy Smith (Not in Mondays), 2014/07/31 14:41:54]

The issue is how that fatality is implemented.  We don't crash the program; we
want to execute the rest of the test harness (& e.g. the next test).  But we
don't want to execute any more test code.  So unlike the EXPECT_* macros, the
ASSERT* macros turn into "return X;", and the type and nature of that return
have an implicit assumption that they're returning from a function declared with
TEST*(...).   I think (reading the code) that the type of that return is void,
so if you're inside a helper function that doesn't return a type, it will
compile.  But you'll then continue on to execute all the code following the call
to that helper function.  And if the helper function doesn't return void, you
won't compile.  (I may have the compile-friendly type wrong, but it doesn't
really matter; it's not that things work in that case, they just fail more
subtly :-}.)

See the definition of ASSERT_TRUE in testing/gtest/include/gtest/gtest.h; it
resovles to GTEST_TEST_BOOLEAN_
(testing/gtest/include/gtest/internal/gtest-internal.h) with a "fail" argument
of GTEST_FATAL_FAILURE_ (same file).  GTEST_FATAL_FAILURE involves a return
statement, which is executed if the check fails.

+    if (!get_success) return false;
+
+    // This check isn't wrapped with EXPECT* macros because different
+    // results from this function may be expected in different tests.
+    std::vector<std::string> tokens;
+    size_t num_tokens = Tokenize(encoding_headers, ",", &tokens);
+    for (size_t i = 0; i < num_tokens; i++) {
+      if (!base::strncasecmp(tokens[i].data(), "sdch", tokens[i].length()))
+        return true;
+    }
+    return false;
+  }
+
+  void EnableSdch() {
+    context_.SetSdchManager((scoped_ptr<SdchManager>(new SdchManager)).Pass());

[jar (doing other things), 2014/07/31 03:24:34]

nit: Do you need extra parens? Wouldn't it be readable as:

 context_.SetSdchManager(scoped_ptr<SdchManager>(new SdchManager).Pass());

[Randy Smith (Not in Mondays), 2014/08/11 20:33:25]

Huh, it compiles.  Thanks for the catch; done.

+  }
+
   MockNetworkLayer network_layer_;
   TestURLRequestContext context_;
   TestDelegate delegate_;
@@ -125,6 +156,24 @@ TEST_F(URLRequestHttpJobTest, SetSubsequentTransactionPriority) {
   EXPECT_EQ(LOW, network_layer_.last_transaction()->priority());
 }
 
+// Confirm we do advertise SDCH encoding in the case of a GET.
+TEST_F(URLRequestHttpJobTest, SdchAdvertisementGet) {
+  EnableSdch();
+  req_.set_method("GET");               // Redundant with default.

[jar (doing other things), 2014/07/31 03:24:34]

nit: 2 spaces before comments... unless you're lining them up.

[Randy Smith (Not in Mondays), 2014/08/11 20:33:25]

Done.

+  scoped_refptr<TestURLRequestHttpJob> job(new TestURLRequestHttpJob(&req_));
+  job->Start();
+  EXPECT_TRUE(TransactionAcceptsSdchEncoding());
+}
+
+// Confirm we don't advertise SDCH encoding in the case of a POST.
+TEST_F(URLRequestHttpJobTest, SdchAdvertisementPost) {
+  EnableSdch();
+  req_.set_method("POST");
+  scoped_refptr<TestURLRequestHttpJob> job(new TestURLRequestHttpJob(&req_));
+  job->Start();
+  EXPECT_FALSE(TransactionAcceptsSdchEncoding());
+}
+
 // This base class just serves to set up some things before the TestURLRequest
 // constructor is called.
 class URLRequestHttpJobWebSocketTestBase : public ::testing::Test {