Improve testing for SDCH.

net/url_request/url_request_http_job_unittest.cc

 // Copyright (c) 2013 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 "net/url_request/url_request_http_job.h"
 
 #include <cstddef>
 
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
@@ skipping 36 matching lines @@
 class URLRequestHttpJobTest : public ::testing::Test {
  protected:
   URLRequestHttpJobTest()
       : req_(GURL("http://www.example.com"),
              DEFAULT_PRIORITY,
              &delegate_,
              &context_) {
     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_;
   TestURLRequest req_;
 };
 
 // Make sure that SetPriority actually sets the URLRequestHttpJob's
 // priority, both before and after start.
 TEST_F(URLRequestHttpJobTest, SetPriorityBasic) {
   scoped_refptr<TestURLRequestHttpJob> job(new TestURLRequestHttpJob(&req_));
@@ skipping 51 matching lines @@
 
   job->Kill();
   network_layer_.ClearLastTransaction();
 
   // Creates a second transaction.
   job->Start();
   ASSERT_TRUE(network_layer_.last_transaction());
   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 {
  protected:
   URLRequestHttpJobWebSocketTestBase() : socket_data_(NULL, 0, NULL, 0),
                                          context_(true) {
     // A Network Delegate is required for the WebSocketHandshakeStreamBase
     // object to be passed on to the HttpNetworkTransaction.
     context_.set_network_delegate(&network_delegate_);
 
@@ skipping 142 matching lines @@
   req_.SetLoadFlags(LOAD_DISABLE_CACHE);
   job->Start();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(URLRequestStatus::IO_PENDING, req_.status().status());
   EXPECT_TRUE(fake_handshake_stream->initialize_stream_was_called());
 }
 
 }  // namespace
 
 }  // namespace net