win: Implementation of CrashReportDatabase for Windows (for C++ Windows readability review)

client/crash_report_database_test.cc

 // Copyright 2015 The Crashpad Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
 //     http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
@@ skipping 25 matching lines @@
 #endif
 }
 
 void CreateFile(const base::FilePath& path) {
   FileHandle handle = LoggingOpenFileForWrite(path,
                                               FileWriteMode::kCreateOrFail,
                                               FilePermissions::kWorldReadable);
 #if defined(OS_POSIX)
   ASSERT_GE(handle, 0);
 #elif defined(OS_WIN)
   ASSERT_NE(handle, nullptr);

[Peter Kasting, 2015/02/13 00:32:38]

Nit: (expected, actual) for gtest ASSERT/EXPECT_EQ/NE

[scottmg, 2015/02/13 06:34:01]

So yoda ugly! Done.

[Mark Mentovai, 2015/02/13 14:08:37]

Peter Kasting wrote:
This is only true for gtest’s _EQ macros. None of the other ones have this
property, including _NE.

Reference: https://code.google.com/p/googletest/wiki/Primer#Binary_Comparison

Note that a failed _EQ assertion prints

x_test.cc:123: Failure
Value of: x
  Actual: 0
Expected: 1
Which is: 1
[  FAILED  ]

and a failed _NE prints

x_test.cc:123: Failure
Expected: (0) != (0), actual: 0 vs 0
[  FAILED  ]

without any reference to which side is “expected” and which side is “actual”.

I probably wouldn’t have said anything at all, but I note that this function is
now totally unused, and we can remove it altogether.

[scottmg, 2015/02/13 17:26:41]

Thanks, removed.

 #endif
   ASSERT_TRUE(
       LoggingWriteFile(handle, path.value().c_str(), path.value().length()));
   ASSERT_TRUE(LoggingCloseFile(handle));
 }
 
 class CrashReportDatabaseTest : public testing::Test {
  protected:
   // testing::Test:
   void SetUp() override {
     db_ = CrashReportDatabase::Initialize(path());
     ASSERT_TRUE(db_.get());

[Peter Kasting, 2015/02/13 00:32:38]

I believe .get() is unnecessary here (multiple places)

[scottmg, 2015/02/13 06:34:01]

Done. (2 locations)

   }
 
   void ResetDatabase() {
     db_.reset();
   }
 
   CrashReportDatabase* db() const { return db_.get(); }

[Peter Kasting, 2015/02/13 00:32:38]

Const functions should not return pointers to non-const objects; make the
returned pointer const or make the function non-const.

[scottmg, 2015/02/13 06:34:01]

Done.

   const base::FilePath& path() const { return temp_dir_.path(); }
 
   void CreateCrashReport(CrashReportDatabase::Report* report) {
     CrashReportDatabase::NewReport* new_report;

[Peter Kasting, 2015/02/13 00:32:38]

Nit: Should this be initialized?  You don't do it here but you do atop
UploadReport() below.  I would be consistent.

[scottmg, 2015/02/13 06:34:01]

Done.

     EXPECT_EQ(CrashReportDatabase::kNoError,

[Peter Kasting, 2015/02/13 00:32:38]

Change to ASSERT_EQ or add an ASSERT_TRUE(new_report) after this line since you
unconditionally deref |new_report| below.

[scottmg, 2015/02/13 06:34:01]

Done.

               db_->PrepareNewCrashReport(&new_report));
     const char kTest[] = "test";
     ASSERT_TRUE(LoggingWriteFile(new_report->handle, kTest, sizeof(kTest)));
 
     UUID uuid;
     EXPECT_EQ(CrashReportDatabase::kNoError,
               db_->FinishedWritingCrashReport(new_report, &uuid));
 
     EXPECT_EQ(CrashReportDatabase::kNoError,
               db_->LookUpCrashReport(uuid, report));
     ExpectPreparedCrashReport(*report);
     ASSERT_TRUE(FileExistsAtPath(report->file_path));
   }
 
   void UploadReport(const UUID& uuid, bool successful, const std::string& id) {
     const CrashReportDatabase::Report* report = nullptr;
     EXPECT_EQ(CrashReportDatabase::kNoError,
               db_->GetReportForUploading(uuid, &report));
     EXPECT_TRUE(report);

[Peter Kasting, 2015/02/13 00:32:38]

Again, this should be ASSERT_TRUE, or the EXPECT_EQ above should be ASSERT_EQ
and this should be removed.  Whatever you do, be consistent with
CreateCrashReport().

Similar comments apply in the ErrorWritingCrashReport test below.

[scottmg, 2015/02/13 06:34:01]

Done.

     EXPECT_NE(UUID(), report->uuid);
     EXPECT_FALSE(report->file_path.empty());
     EXPECT_TRUE(FileExistsAtPath(report->file_path))
         << report->file_path.value();
     EXPECT_GT(report->creation_time, 0);
     EXPECT_EQ(CrashReportDatabase::kNoError,
               db_->RecordUploadAttempt(report, successful, id));
   }
 
   void ExpectPreparedCrashReport(const CrashReportDatabase::Report& report) {
     EXPECT_NE(UUID(), report.uuid);
     EXPECT_FALSE(report.file_path.empty());
     EXPECT_TRUE(FileExistsAtPath(report.file_path)) << report.file_path.value();
     EXPECT_TRUE(report.id.empty());
     EXPECT_GT(report.creation_time, 0);
     EXPECT_FALSE(report.uploaded);
     EXPECT_EQ(0, report.last_upload_attempt_time);
     EXPECT_EQ(0, report.upload_attempts);
   }
 
   void RelocateDatabase() {
     ResetDatabase();
     temp_dir_.Rename();
     SetUp();
   }
 
  private:
   ScopedTempDir temp_dir_;
   scoped_ptr<CrashReportDatabase> db_;
 };

[Peter Kasting, 2015/02/13 00:32:38]

DISALLOW_COPY_AND_ASSIGN()

[scottmg, 2015/02/13 06:34:01]

Done.

 
 TEST_F(CrashReportDatabaseTest, Initialize) {
   // Initialize the database for the first time, creating it.
   EXPECT_TRUE(db());
 
   // Close and reopen the database at the same path.
   ResetDatabase();
   EXPECT_FALSE(db());
   auto db = CrashReportDatabase::Initialize(path());
   EXPECT_TRUE(db.get());

[Peter Kasting, 2015/02/13 00:32:38]

This should be ASSERT_TRUE since you deref |db| unconditionally below.

[scottmg, 2015/02/13 06:34:01]

Done.

 
   std::vector<const CrashReportDatabase::Report> reports;

[Peter Kasting, 2015/02/13 00:32:38]

This is out-of-scope for the current CL, but consider adding a typedef for this
type (e.g. CrashReportDatabase::Reports) and then making use of it in this file
and in the CrashReportDatabase subclasses.

While this would perhaps enhance readability and maintainability on its own, it
has an optional side nicety.  In a number of places where you currently do
something like:

  std::vector<const CrashReportDatabase::Report> reports;
  ...db()->GetPendingReports(&reports);
  EXPECT_EQ(4, reports.size());

  ...

  reports.clear();
  db()->GetPendingReports(&reports);
  EXPECT_EQ(4, reports.size());

This would give you the option of making each block a local scope, a la:

  {
    CrashReportDatabase::Reports reports;
    ...db()->GetPendingReports(&reports);
    EXPECT_EQ(4, reports.size());
  }

...without the verbosity of the ugly vector<> type each time.  This might help
callers to see which pieces of which tests were self-contained and which ones
fetched report vectors to be used by the rest of the test code.

[scottmg, 2015/02/13 06:34:01]

That seems reasonable. I sort of dislike hiding vector behind a typedef that
makes it less clear what you're working with. I will defer that to a future
change in any case, and Mark/Robert's preference.

[Peter Kasting, 2015/02/13 22:30:32]

It's a minor thing.  It wouldn't normally be an issue if functions returned this
type now that we have "auto", but since the ones in question here still need to
take variables as outparams, it's still a factor.

One reason I like "Reports" over "vector<Report>" is that, largely, callers
shouldn't really care what precise type of container is being used.  If for some
reason it becomes appropriate to use a list instead, it's easier to do that with
typedefs in place than without.  However, if a reader needs to understand that
the underlying container is really a vector, then obviously this makes that more
opaque.

[scottmg, 2015/02/13 23:37:16]

Acknowledged.

   EXPECT_EQ(CrashReportDatabase::kNoError, db->GetPendingReports(&reports));
   EXPECT_TRUE(reports.empty());

[Peter Kasting, 2015/02/13 00:32:38]

Either change this to ASSERT_TRUE or add a "reports.clear()" call just after,
since if the vector is non-empty here, GetCompletedReports() below will
DCHECK().

[scottmg, 2015/02/13 06:34:01]

They're slightly unrelated checks, so I went with clear().

   EXPECT_EQ(CrashReportDatabase::kNoError, db->GetCompletedReports(&reports));
   EXPECT_TRUE(reports.empty());
 }
 
 TEST_F(CrashReportDatabaseTest, NewCrashReport) {
   CrashReportDatabase::NewReport* new_report;
   EXPECT_EQ(CrashReportDatabase::kNoError,
             db()->PrepareNewCrashReport(&new_report));
   EXPECT_TRUE(FileExistsAtPath(new_report->path)) << new_report->path.value();
   UUID uuid;
@@ skipping 36 matching lines @@
     CreateCrashReport(&report);
     uuid = report.uuid;
   }
 
   {
     CrashReportDatabase::Report report;
     EXPECT_EQ(CrashReportDatabase::kNoError,
               db()->LookUpCrashReport(uuid, &report));
     EXPECT_EQ(uuid, report.uuid);
     EXPECT_NE(std::string::npos, report.file_path.value().find(path().value()));
     EXPECT_EQ("", report.id);

[Peter Kasting, 2015/02/13 00:32:38]

Nit: "" -> std::string() (multiple places)

You could also use EXPECT_TRUE(report.id.empty()) or similar in many of these
places if you wish.

[scottmg, 2015/02/13 06:34:01]

Done.

     EXPECT_FALSE(report.uploaded);
     EXPECT_EQ(0, report.last_upload_attempt_time);
     EXPECT_EQ(0, report.upload_attempts);
   }
 
   UploadReport(uuid, true, "test");
 
   {
     CrashReportDatabase::Report report;
     EXPECT_EQ(CrashReportDatabase::kNoError,
@@ skipping 11 matching lines @@
   std::vector<CrashReportDatabase::Report> reports(3);
   CreateCrashReport(&reports[0]);
   CreateCrashReport(&reports[1]);
   CreateCrashReport(&reports[2]);
 
   // Record two attempts: one successful, one not.
   UploadReport(reports[1].uuid, false, "");
   UploadReport(reports[2].uuid, true, "abc123");
 
   std::vector<CrashReportDatabase::Report> query(3);
 

[Peter Kasting, 2015/02/13 00:32:38]

Nit: I would remove this blank line

[scottmg, 2015/02/13 06:34:01]

I hate to dissent for something so minor, but because it's used in subsequent
blocks that are separated by blanks, I feel not attaching it to line 223 is
better.

[Peter Kasting, 2015/02/13 22:30:32]

In that case, I wonder why you don't have a blank line after the declaration of
|reports| above?  It seems like being consistent might be better.

I think removing the blank might be more consistent with "declare as close to
first use as possible", but that's a very weak argument, so I'm OK with either
route.

[scottmg, 2015/02/13 23:37:16]

Fair enough. Done.

   EXPECT_EQ(CrashReportDatabase::kNoError,
             db()->LookUpCrashReport(reports[0].uuid, &query[0]));
   EXPECT_EQ(CrashReportDatabase::kNoError,
             db()->LookUpCrashReport(reports[1].uuid, &query[1]));
   EXPECT_EQ(CrashReportDatabase::kNoError,
             db()->LookUpCrashReport(reports[2].uuid, &query[2]));
 
   EXPECT_EQ("", query[0].id);
   EXPECT_EQ("", query[1].id);
   EXPECT_EQ("abc123", query[2].id);
@@ skipping 99 matching lines @@
   ASSERT_EQ(1u, completed.size());
 
   for (const auto& report : pending)
     EXPECT_NE(report_1_uuid, report.uuid);
   EXPECT_EQ(report_1_uuid, completed[0].uuid);
   EXPECT_EQ("report1", completed[0].id);
   EXPECT_EQ(true, completed[0].uploaded);
   EXPECT_GT(completed[0].last_upload_attempt_time, 0);
   EXPECT_EQ(1, completed[0].upload_attempts);
 
   const CrashReportDatabase::Report completed_report_1 = completed[0];

[Peter Kasting, 2015/02/13 00:32:38]

You don't seem to use this.

[scottmg, 2015/02/13 06:34:01]

Done.

 
   // Fail to upload one report.
   UploadReport(report_2_uuid, false, "");
 
   pending.clear();
   EXPECT_EQ(CrashReportDatabase::kNoError,
             db()->GetPendingReports(&pending));
   completed.clear();
   EXPECT_EQ(CrashReportDatabase::kNoError,
             db()->GetCompletedReports(&completed));
@@ skipping 101 matching lines @@
   CrashReportDatabase::Report report;
   EXPECT_EQ(CrashReportDatabase::kNoError,
             db()->LookUpCrashReport(uuid, &report));
   ExpectPreparedCrashReport(report);
   EXPECT_TRUE(FileExistsAtPath(report.file_path)) << report.file_path.value();
 }
 
 }  // namespace
 }  // namespace test
 }  // namespace crashpad
+// EOF comment added for readability review.