Readability review.

remoting/host/host_extension_session_manager_unittest.cc

 // Copyright 2014 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 "remoting/codec/video_encoder.h"
 #include "remoting/host/fake_host_extension.h"
 #include "remoting/host/host_extension_session_manager.h"
 #include "remoting/host/host_mock_objects.h"
 #include "remoting/proto/control.pb.h"
 #include "remoting/protocol/protocol_mock_objects.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_capturer.h"
 
 namespace remoting {
 
 class HostExtensionSessionManagerTest : public testing::Test {
  public:
   HostExtensionSessionManagerTest()
       : extension1_("ext1", "cap1"),
         extension2_("ext2", ""),

[Peter Kasting, 2014/08/22 06:38:29]

Nit: Prefer std::string() over ""

[Wez, 2014/08/25 23:23:11]

Done.

         extension3_("ext3", "cap3") {
     extensions_.push_back(&extension1_);
     extensions_.push_back(&extension2_);
     extensions_.push_back(&extension3_);
   }
   virtual ~HostExtensionSessionManagerTest() {}
 
  protected:

[Peter Kasting, 2014/08/22 06:38:29]

Nit: In general, the Google style guide prefers private data members to
protected ones.  While protected members are allowed when using gtest "for
technical reasons", it's usually still preferable to make them private and
provide accessors as necessary.

[Wez, 2014/08/25 23:23:11]

For a general purpose class I'd agree, but this is a test fixture so it's only
purpose is as a container for members that are common across several tests;
making them private seems to add a lot of boilerplate for no clear benefit.

[Peter Kasting, 2014/08/25 23:39:06]

The main benefits that normally accrue involve clarifying access patterns to the
members.  If lots of tests need to read and write each separate member it's not
much of a win, but if for example the tests only ever read these (and the
constructor does all the writing), having the tests only have access to getters
can make it clearer to e.g. someone trying to add more functionality to the test
fixture later that this is the case, and prevent someone from writing a test
case that writes to these when it's not supposed to.

While you can proceed with the code as you have it, I think the fact that the
Google style guide allows this "for technical reasons" rather than giving a more
readability-focused argument like the one you make suggests that the style guide
authors didn't really allow this case because direct member access in tests
allows avoiding ugly boilerplate, but rather because certain kinds of testing
are hard to do at all without this exception.

Up to you.

[Wez, 2014/08/26 19:22:25]

OK, thanks for clarifying.

   // Fake HostExtensions for testing.
   FakeExtension extension1_;
   FakeExtension extension2_;
   FakeExtension extension3_;
   std::vector<HostExtension*> extensions_;
 
   // Mocks of interfaces provided by ClientSession.
   MockClientSessionControl client_session_control_;
   protocol::MockClientStub client_stub_;
 };

[Peter Kasting, 2014/08/22 06:38:29]

Nit: DISALLOW_COPY_AND_ASSIGN()

[Wez, 2014/08/25 23:23:11]

Done.

 
 // Verifies that messages are passed to be handled by the correct extension.

[Peter Kasting, 2014/08/22 06:38:29]

Nit: I'm not sure what this sentence means, but it seems like removing "passed
to be" would make it clearer?

[Wez, 2014/08/25 23:23:11]

Done.

 TEST_F(HostExtensionSessionManagerTest, ExtensionMessages_MessageHandled) {
   HostExtensionSessionManager extension_manager(extensions_,
                                                 &client_session_control_);
   extension_manager.OnNegotiatedCapabilities(
       &client_stub_, extension_manager.GetCapabilities());
 
   protocol::ExtensionMessage message;
   message.set_type("ext2");
   extension_manager.OnExtensionMessage(message);
 
@@ skipping 18 matching lines @@
   EXPECT_FALSE(extension2_.has_handled_message());
   EXPECT_FALSE(extension3_.has_handled_message());
 }
 
 // Verifies that the correct set of capabilities are reported to the client,
 // based on the registered extensions.
 TEST_F(HostExtensionSessionManagerTest, ExtensionCapabilities_AreReported) {
   HostExtensionSessionManager extension_manager(extensions_,
                                                 &client_session_control_);
 
   EXPECT_EQ(extension_manager.GetCapabilities(), "cap1 cap3");

[Peter Kasting, 2014/08/22 06:38:29]

Would it be equally valid to return "cap3 cap1"?  Should the test account for
this?

[Wez, 2014/08/25 23:23:11]

Good point; have updated the test to account for that.

 }
 
 // Verifies that an extension is not instantiated if the client does not
 // support its required capability, and that it does not receive messages.
 TEST_F(HostExtensionSessionManagerTest, ExtensionCapabilities_AreChecked) {
   HostExtensionSessionManager extension_manager(extensions_,
                                                 &client_session_control_);
   extension_manager.OnNegotiatedCapabilities(&client_stub_, "cap1");
 
   protocol::ExtensionMessage message;
@@ skipping 62 matching lines @@
       scoped_ptr<webrtc::DesktopCapturer>());
   extension_manager.OnCreateVideoEncoder(scoped_ptr<VideoEncoder>());
 
   EXPECT_FALSE(extension1_.has_wrapped_video_encoder());
   EXPECT_FALSE(extension1_.has_wrapped_video_capturer());
   EXPECT_TRUE(extension2_.has_wrapped_video_encoder());
   EXPECT_TRUE(extension2_.has_wrapped_video_capturer());
   EXPECT_FALSE(extension3_.was_instantiated());
 }
 
 // Verifies that if an extension reports that they modify the video pipeline

[Peter Kasting, 2014/08/22 06:38:29]

Nit: they modify -> it modifies

[Wez, 2014/08/25 23:23:11]

Done.

 // then ResetVideoPipeline() is called on the ClientSessionControl interface.
 TEST_F(HostExtensionSessionManagerTest, CallsResetVideoPipeline) {
   HostExtensionSessionManager extension_manager(extensions_,
                                                 &client_session_control_);
 
   EXPECT_CALL(client_session_control_, ResetVideoPipeline());
 
   // Set up only the first extension to request to modify the video pipeline.
   extension1_.set_steal_video_capturer(true);
 
   extension_manager.OnNegotiatedCapabilities(&client_stub_, "cap1");
 }
 
 
 }  // namespace remoting
+