C++ Readability Review for thestig.

content/browser/media_device_notifications_linux_unittest.cc

 // Copyright (c) 2012 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 <fcntl.h>
 #include <mntent.h>
 #include <stdio.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 

[ajl, 2012/03/23 22:47:39]

The style guide now prefers *not* to have an empty line separating C and C++
standard library includes, see code snippets here:

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Names_and_Order_of_In...

Note that they do still need to be grouped separately, there should just not be
any empty line between the groups.

[Lei Zhang, 2012/03/24 03:01:26]

Done.

[Lei Zhang, 2012/04/09 23:19:30]

Ah, I asked mmentovai@ about this. This is a recent style change in google3, but
Chromium started with an older edition of the google3 style guide and never
picked up this rule. So Chromium code still prefers a blank line between the C
and C++ includes. I'll keep this in mind when/if I work on google3 code.

See
https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_...
for example.

 #include <string>
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/scoped_temp_dir.h"
 #include "base/system_monitor/system_monitor.h"
 #include "base/test/mock_devices_changed_observer.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/media_device_notifications_linux.h"

[ajl, 2012/03/23 22:47:39]

This actually needs to be the very first file included here (note the reference
to testing in the linked style guide section):

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Names_and_Order_of_In...

[Lei Zhang, 2012/03/24 03:01:26]

Done.

 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
 
 namespace {
 
 const char* kValidFS = "vfat";
 const char* kInvalidFS = "invalidfs";
 
 const char* kInvalidPath = "invalid path does not exist";
 
 const char* kDevice1 = "d1";
 const char* kDevice2 = "d2";
 const char* kDevice3 = "d3";
 
 const char* kMountPointA = "mnt_a";
 const char* kMountPointB = "mnt_b";
 
 }  // namespace
 
 namespace content {
 
 class MediaDeviceNotificationsLinuxTest : public testing::Test {

[ajl, 2012/03/23 22:47:39]

Can you put this test code inside an anonymous namespace?  It is preferred if
possible to minimize the scope of the test code, since it shouldn't be
referenced externally anyway.

Usually the only reason you can't do this is if some test class needs to be a
friend of the class it is testing.  I don't think you are doing that.

Note that you can keep your tests inside your project namespace to have
unqualified access to project names - just use an anonymous namespace *inside*
your project namespace.

[Lei Zhang, 2012/03/24 03:01:26]

Done.

  public:
   struct MtabTestData {
     MtabTestData(const char* mount_device,

[ajl, 2012/03/23 22:47:39]

It seems like it could make this code simpler and possibly safer if you passed
and stored these as C++ strings rather than const char* (the usual way would be
to make the arguments references and copy them to non-reference data members in
the constructor).

This would eliminate various tricky details around const char* (NULL, ownership
of the data, length, lack of built in methods for common operations) and also
better follow google convention.

The down side is that it would involve more copying and you would have to
manually convert to const char* when passing as arguments to code that requires
that - I don't think those are very serious problems compared to the advantages,
especially in a test.

[Lei Zhang, 2012/03/24 03:01:26]

Done.

                  const char* mount_point,
                  const char* mount_type)
         : mount_device(mount_device),
           mount_point(mount_point),
           mount_type(mount_type) {
     }
 
     const char* mount_device;
     const char* mount_point;
     const char* mount_type;
   };
 
   MediaDeviceNotificationsLinuxTest()
       : message_loop_(MessageLoop::TYPE_IO),
         file_thread_(BrowserThread::FILE, &message_loop_) {
     system_monitor_.reset(new base::SystemMonitor());

[ajl, 2012/03/23 22:47:39]

Why not do this in the initializer list, instead of resetting in in the
constructor body?

(although this will be moot if you follow my suggestion and change to a default
data member)

[Lei Zhang, 2012/03/24 03:01:26]

It is indeed moot.

   }
   virtual ~MediaDeviceNotificationsLinuxTest() {}
 
  protected:
   virtual void SetUp() {
     mock_devices_changed_observer_.reset(new base::MockDevicesChangedObserver);
     system_monitor_->AddDevicesChangedObserver(
         mock_devices_changed_observer_.get());
 
     // Create and set up a temp dir with files for the test.
     ASSERT_TRUE(scoped_temp_dir_.CreateUniqueTempDir());
     FilePath test_dir = scoped_temp_dir_.path().AppendASCII("test_etc");
     ASSERT_TRUE(file_util::CreateDirectory(test_dir));
     mtab_file_ = test_dir.AppendASCII("test_mtab");
     struct MtabTestData initial_test_data[] = {

[ajl, 2012/03/23 22:47:39]

C++ does not require prepending 'struct' to the name of struct types, and since
this is consistent with how class names are referenced, not including them is
the preference and the overwhelming convention in google code.

So please change this to just 'MtabTestData initial...'.

Same in similar cases elsewhere.

[Lei Zhang, 2012/03/24 03:01:26]

Done.

       MtabTestData("dummydevice", "dummydir", kInvalidFS),
     };
     WriteToMtab(initial_test_data, arraysize(initial_test_data), true);
 
     // Initialize the test subject.
     notifications_ = new MediaDeviceNotificationsLinux(mtab_file_);
     notifications_->Init();
     message_loop_.RunAllPending();
   }
 
@@ skipping 24 matching lines @@
     return_path = return_path.AppendASCII(dir);
     FilePath path(return_path);
     if (with_dcim_dir)
       path = path.AppendASCII("DCIM");
     if (!file_util::CreateDirectory(path))
       return FilePath();
     return return_path;
   }
 
   base::MockDevicesChangedObserver& observer() {
     return *mock_devices_changed_observer_.get();

[ajl, 2012/03/23 22:47:39]

Is get() needed here?  Most smart pointer classes have * overriden to
dereference the wrapped pointer and using that (without get()) is quite
conventional in google code.

(it is more standard to use get(), even when not strictly necessary, when
accessing the pointer value directly)

[Lei Zhang, 2012/03/24 03:01:26]

No, it's not, and I've already removed it in the CL that fixes the test
flakiness.

   }
 
  private:
   // Write the test mtab data to |mtab_file_|.
   // |data| is an array of mtab entries.
   // |data_size| is the array size of |data|.
   // |overwrite| specifies whether to overwrite |mtab_file_|.
   void WriteToMtab(struct MtabTestData* data,

[ajl, 2012/03/23 22:47:39]

Can data be const?  The style guide prefers variables and parameters to be
declared const whenever possible:

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Use_of_const#Use_of_c...

[Lei Zhang, 2012/03/24 03:01:26]

Done.

                    size_t data_size,
                    bool overwrite) {
     FILE* file = setmntent(mtab_file_.value().c_str(), overwrite ? "w" : "a");
     ASSERT_TRUE(file);
 
     struct mntent entry;
     entry.mnt_opts = strdup("rw");
     entry.mnt_freq = 0;
     entry.mnt_passno = 0;
     for (size_t i = 0; i < data_size; ++i) {
       entry.mnt_fsname = strdup(data[i].mount_device);

[ajl, 2012/03/23 22:47:39]

Why does the string need to be copied here?  Is there any reason you can't just
copy the pointer from the original location, given it seems to be in the right
format (null terminated string) already and will live longer than this copy?

It would be nice to get rid of this strdup() if so, especially since it would
eliminate the need for C-style free() calls below (very unusual in google code).

[Lei Zhang, 2012/03/24 03:01:26]

The mntent struct has char* for these fields, whereas the MtabTestData struct
members, converted to std::string, will give a const char* with .c_str().

       entry.mnt_dir = strdup(data[i].mount_point);
       entry.mnt_type = strdup(data[i].mount_type);
       int add_result = addmntent(file, &entry);
       ASSERT_EQ(0, add_result);
       free(entry.mnt_fsname);
       free(entry.mnt_dir);
       free(entry.mnt_type);
     }
     free(entry.mnt_opts);
     int end_result = endmntent(file);
     ASSERT_EQ(1, end_result);
 
     // Need to ensure data reaches disk so the FilePathWatcher fires in time.
     // Otherwise this will cause MediaDeviceNotificationsLinuxTest to be flaky.
     int fd = open(mtab_file_.value().c_str(), O_RDONLY);
     ASSERT_GE(fd, 0);

[ajl, 2012/03/23 22:47:39]

Just FYI: the gunit assertion macros (I'm assuming whatever the Chrome codebase
has is similar) treat the first argument as the 'expected' value and the second
as the 'actual'.  This doesn't really affect the correctness of the test, since
equality is a symmetric operator, but it does affect the error messages that are
printed when assertions fail: If you put the arguments in the opposite order,
the test will say 'expected result B, actual was A' when it actually should be
the opposite.

So, to make the test output nicer, you might want to switch around the order of
arguments here and elsewhere.

[Lei Zhang, 2012/03/24 03:01:26]

I've been pretty good about the _EQ macros, but I tend to forget with the
others, since I think of it as |fd| >= 0, rather than 0 <= |fd|. However, this
particular case got removed in the flakiness test fix CL.

 
     int fsync_result = fsync(fd);

[ajl, 2012/03/23 22:47:39]

Are you sure this temporary is necessary?  Sometimes having a variable like this
can, through its name, give some additional useful information, but I think here
it would be easy enough to understand if you eliminated it and just moved the
fsync call instead the ASSERT.

Same with close_result below, and other similar cases.

[Lei Zhang, 2012/03/24 03:01:26]

This particular instance got removed in the follow up CL that fixed test
flakiness. I fixed other similar cases.

     ASSERT_EQ(0, fsync_result);
 
     int close_result = close(fd);
     ASSERT_EQ(0, close_result);
   }
 
   // The message loop and file thread to run tests on.
   MessageLoop message_loop_;
   BrowserThreadImpl file_thread_;
 
   // SystemMonitor and DevicesChangedObserver to hook together to test.
   scoped_ptr<base::SystemMonitor> system_monitor_;

[ajl, 2012/03/23 22:47:39]

Does this really need to be allocated on the heap, or could you change this to a
direct data member, instead of a scoped_ptr?

If so that is simpler and could be a little more efficient.

[Lei Zhang, 2012/03/24 03:01:26]

Done.

   scoped_ptr<base::MockDevicesChangedObserver> mock_devices_changed_observer_;
 
   // Temporary directory for created test data.
   ScopedTempDir scoped_temp_dir_;
   // Path to the test mtab file.
   FilePath mtab_file_;
 
   scoped_refptr<MediaDeviceNotificationsLinux> notifications_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaDeviceNotificationsLinuxTest);
 };
 
 TEST_F(MediaDeviceNotificationsLinuxTest, BasicAttachDetach) {

[ajl, 2012/03/23 22:47:39]

It would be helpful to readers if you added a short comment before each test
method explaining what it tests in a little more detail than is offered by just
the name.

[Lei Zhang, 2012/03/24 03:01:26]

Done.

   testing::Sequence mock_sequence;
   FilePath test_path = CreateMountPoint(kMountPointA, true);

[ajl, 2012/03/23 22:47:39]

When you pass literals like true as function arguments, you need to have a short
inline comment next to them indicating what they mean. See here (under "NULL,
true/false, 1, 2, 3...") for the recommended style:

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Implementation_Commen...

Same elsewhere

[Lei Zhang, 2012/03/24 03:01:26]

It's a bit tedious to repeat the same comment for all instances. Instead, I just
wrote methods with better names to wrap the method that takes true/false.

   ASSERT_FALSE(test_path.empty());
   struct MtabTestData test_data[] = {
     MtabTestData(kDevice1, kInvalidPath, kValidFS),
     MtabTestData(kDevice2, test_path.value().c_str(), kValidFS),
   };
   EXPECT_CALL(observer(), OnMediaDeviceAttached(0, kDevice2, test_path))

[ajl, 2012/03/23 22:47:39]

Please add a comment explaining what this actually does.  I can't tell to be
honest.  In general gmock syntax is incredibly opaque to readers, so some kind
of comment is necessary.

Same in similar cases elsewhere.

[Lei Zhang, 2012/03/24 03:01:26]

Done. The more complicated cases are already commented.

       .InSequence(mock_sequence);
   WriteToMtabAndRunLoop(test_data, arraysize(test_data), false);
 
   EXPECT_CALL(observer(), OnMediaDeviceDetached(0)).InSequence(mock_sequence);
   WriteToMtabAndRunLoop(NULL, 0, true);
 }
 
 // Only mount points with DCIM directories are recognized.
 TEST_F(MediaDeviceNotificationsLinuxTest, DCIM) {
   testing::Sequence mock_sequence;
@@ skipping 122 matching lines @@
       .Times(1);
   WriteToMtabAndRunLoop(test_data2, arraysize(test_data2), false);
 
   // Detach all devices.
   EXPECT_CALL(observer(), OnMediaDeviceAttached(_, _, _)).Times(0);
   EXPECT_CALL(observer(), OnMediaDeviceDetached(1)).Times(1);
   WriteToMtabAndRunLoop(NULL, 0, true);
 }
 
 }  // namespace content
+