[Password Manager] Simplify export tests

pulling out setting the password list and allowing to use a
base::MockCallback<WriteCallback> in SetWriteForTesting
directly to reduce noise in the PasswordManagerExporter test code.

Bug: 789561
Change-Id: Ib703b8a720e38f75d57519b9f96f926e93e9a561
Reviewed-on: https://chromium-review.googlesource.com/924422
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539044}

components/password_manager/core/browser/export/password_manager_exporter.cc

@@ -31,13 +31,12 @@ base::LazySingleThreadTaskRunner g_task_runner =
 
 // A wrapper for |write_function|, which can be bound and keep a copy of its
 // data on the closure.
-bool Write(int (*write_function)(const base::FilePath& filename,
-                                 const char* data,
-                                 int size),
-           const base::FilePath& destination,
-           const std::string& serialised) {
+bool Write(
+    password_manager::PasswordManagerExporter::WriteCallback write_function,
+    const base::FilePath& destination,
+    const std::string& serialised) {
   int written =
-      write_function(destination, serialised.c_str(), serialised.size());
+      write_function.Run(destination, serialised.c_str(), serialised.size());
   return written == static_cast<int>(serialised.size());
 }
 
@@ -54,7 +53,7 @@ PasswordManagerExporter::PasswordManagerExporter(
     : credential_provider_interface_(credential_provider_interface),
       on_progress_(std::move(on_progress)),
       last_progress_status_(ExportProgressStatus::NOT_STARTED),
-      write_function_(&base::WriteFile),
+      write_function_(base::BindRepeating(&base::WriteFile)),
       delete_function_(base::BindRepeating(&base::DeleteFile)),
       task_runner_(g_task_runner.Get()),
       weak_factory_(this) {}
@@ -127,16 +126,13 @@ PasswordManagerExporter::GetProgressStatus() {
   return last_progress_status_;
 }
 
-void PasswordManagerExporter::SetWriteForTesting(
-    int (*write_function)(const base::FilePath& filename,
-                          const char* data,
-                          int size)) {
-  write_function_ = write_function;
+void PasswordManagerExporter::SetWriteForTesting(WriteCallback write_function) {
+  write_function_ = std::move(write_function);
 }
 
 void PasswordManagerExporter::SetDeleteForTesting(
     DeleteCallback delete_callback) {
-  delete_function_ = delete_callback;
+  delete_function_ = std::move(delete_callback);
 }
 
 bool PasswordManagerExporter::IsReadyForExport() {

components/password_manager/core/browser/export/password_manager_exporter.h

@@ -29,6 +29,8 @@ class PasswordManagerExporter {
   using ProgressCallback =
       base::RepeatingCallback<void(password_manager::ExportProgressStatus,
                                    const std::string&)>;
+  using WriteCallback =
+      base::RepeatingCallback<int(const base::FilePath&, const char*, int)>;
   using DeleteCallback =
       base::RepeatingCallback<bool(const base::FilePath&, bool)>;
 
@@ -55,9 +57,7 @@ class PasswordManagerExporter {
 
   // Replace the function which writes to the filesystem with a custom action.
   // The return value is -1 on error, otherwise the number of bytes written.
-  void SetWriteForTesting(int (*write_function)(const base::FilePath& filename,
-                                                const char* data,
-                                                int size));
+  void SetWriteForTesting(WriteCallback write_callback);
 
   // Replace the function which writes to the filesystem with a custom action.
   // The return value is true when deleting successfully.
@@ -112,11 +112,9 @@ class PasswordManagerExporter {
   // list. Useful for metrics.
   base::Time export_preparation_started_;
 
-  // The function which does the actual writing. It should point to
+  // The function which does the actual writing. It should wrap
   // base::WriteFile, unless it's changed for testing purposes.
-  int (*write_function_)(const base::FilePath& filename,
-                         const char* data,
-                         int size);
+  WriteCallback write_function_;
 
   // The function which does the actual deleting of a file. It should wrap
   // base::DeleteFile, unless it's changed for testing purposes.

components/password_manager/core/browser/export/password_manager_exporter_unittest.cc

@@ -78,17 +78,6 @@ class FakeCredentialProvider
   DISALLOW_COPY_AND_ASSIGN(FakeCredentialProvider);
 };
 
-// WriteFunction will delegate to this callback, if set. Use for setting
-// expectations for base::WriteFile in PasswordManagerExporter.
-base::MockCallback<WriteCallback>* g_write_callback = nullptr;
-
-// Mock for base::WriteFile. Expectations should be set on |g_write_callback|.
-int WriteFunction(const base::FilePath& filename, const char* data, int size) {
-  if (g_write_callback)
-    return g_write_callback->Get().Run(filename, data, size);
-  return size;
-}
-
 // Creates a hardcoded set of credentials for tests.
 std::vector<std::unique_ptr<autofill::PasswordForm>> CreatePasswordList() {
   auto password_form = std::make_unique<autofill::PasswordForm>();
@@ -108,15 +97,17 @@ class PasswordManagerExporterTest : public testing::Test {
             base::test::ScopedTaskEnvironment::MainThreadType::UI),
         exporter_(&fake_credential_provider_, mock_on_progress_.Get()),
         destination_path_(kNullFileName) {
-    g_write_callback = &mock_write_file_;
-    exporter_.SetWriteForTesting(&WriteFunction);
+    exporter_.SetWriteForTesting(mock_write_file_.Get());
     exporter_.SetDeleteForTesting(mock_delete_file_.Get());
+    password_list_ = CreatePasswordList();
+    fake_credential_provider_.SetPasswordList(password_list_);
   }
 
-  ~PasswordManagerExporterTest() override { g_write_callback = nullptr; }
+  ~PasswordManagerExporterTest() override = default;
 
  protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
+  std::vector<std::unique_ptr<autofill::PasswordForm>> password_list_;
   FakeCredentialProvider fake_credential_provider_;
   base::MockCallback<base::RepeatingCallback<
       void(password_manager::ExportProgressStatus, const std::string&)>>
@@ -132,11 +123,8 @@ class PasswordManagerExporterTest : public testing::Test {
 };
 
 TEST_F(PasswordManagerExporterTest, PasswordExportSetPasswordListFirst) {
-  std::vector<std::unique_ptr<autofill::PasswordForm>> password_list =
-      CreatePasswordList();
-  fake_credential_provider_.SetPasswordList(password_list);
   const std::string serialised(
-      password_manager::PasswordCSVWriter::SerializePasswords(password_list));
+      password_manager::PasswordCSVWriter::SerializePasswords(password_list_));
 
   EXPECT_CALL(mock_write_file_,
               Run(destination_path_, StrEq(serialised), serialised.size()))
@@ -153,7 +141,8 @@ TEST_F(PasswordManagerExporterTest, PasswordExportSetPasswordListFirst) {
 
   scoped_task_environment_.RunUntilIdle();
   histogram_tester_.ExpectUniqueSample(
-      "PasswordManager.ExportedPasswordsPerUserInCSV", password_list.size(), 1);
+      "PasswordManager.ExportedPasswordsPerUserInCSV", password_list_.size(),
+      1);
   histogram_tester_.ExpectTotalCount(
       "PasswordManager.TimeReadingExportedPasswords", 1);
   histogram_tester_.ExpectUniqueSample(
@@ -164,9 +153,6 @@ TEST_F(PasswordManagerExporterTest, PasswordExportSetPasswordListFirst) {
 // When writing fails, we should notify the UI of the failure and try to cleanup
 // a possibly partial passwords file.
 TEST_F(PasswordManagerExporterTest, WriteFileFailed) {
-  std::vector<std::unique_ptr<autofill::PasswordForm>> password_list =
-      CreatePasswordList();
-  fake_credential_provider_.SetPasswordList(password_list);
   const std::string destination_folder_name(
       destination_path_.DirName().BaseName().AsUTF8Unsafe());
 
@@ -193,11 +179,8 @@ TEST_F(PasswordManagerExporterTest, WriteFileFailed) {
 // Test that GetProgressStatus() returns the last ExportProgressStatus sent
 // to the callback.
 TEST_F(PasswordManagerExporterTest, GetProgressReturnsLastCallbackStatus) {
-  std::vector<std::unique_ptr<autofill::PasswordForm>> password_list =
-      CreatePasswordList();
-  fake_credential_provider_.SetPasswordList(password_list);
   const std::string serialised(
-      password_manager::PasswordCSVWriter::SerializePasswords(password_list));
+      password_manager::PasswordCSVWriter::SerializePasswords(password_list_));
   const std::string destination_folder_name(
       destination_path_.DirName().BaseName().AsUTF8Unsafe());
 
@@ -218,10 +201,6 @@ TEST_F(PasswordManagerExporterTest, GetProgressReturnsLastCallbackStatus) {
 }
 
 TEST_F(PasswordManagerExporterTest, DontExportWithOnlyDestination) {
-  std::vector<std::unique_ptr<autofill::PasswordForm>> password_list =
-      CreatePasswordList();
-  fake_credential_provider_.SetPasswordList(password_list);
-
   EXPECT_CALL(mock_write_file_, Run(_, _, _)).Times(0);
   EXPECT_CALL(
       mock_on_progress_,
@@ -239,10 +218,6 @@ TEST_F(PasswordManagerExporterTest, DontExportWithOnlyDestination) {
 }
 
 TEST_F(PasswordManagerExporterTest, CancelAfterPasswords) {
-  std::vector<std::unique_ptr<autofill::PasswordForm>> password_list =
-      CreatePasswordList();
-  fake_credential_provider_.SetPasswordList(password_list);
-
   EXPECT_CALL(mock_write_file_, Run(_, _, _)).Times(0);
   EXPECT_CALL(
       mock_on_progress_,
@@ -260,10 +235,6 @@ TEST_F(PasswordManagerExporterTest, CancelAfterPasswords) {
 }
 
 TEST_F(PasswordManagerExporterTest, CancelWhileExporting) {
-  std::vector<std::unique_ptr<autofill::PasswordForm>> password_list =
-      CreatePasswordList();
-  fake_credential_provider_.SetPasswordList(password_list);
-
   EXPECT_CALL(mock_write_file_, Run(_, _, _)).Times(0);
   EXPECT_CALL(mock_delete_file_, Run(destination_path_, false));
   EXPECT_CALL(
@@ -288,10 +259,6 @@ TEST_F(PasswordManagerExporterTest, CancelWhileExporting) {
 // The "Cancel" button may still be visible on the UI after we've completed
 // exporting. If they choose to cancel, we should clear the file.
 TEST_F(PasswordManagerExporterTest, CancelAfterExporting) {
-  std::vector<std::unique_ptr<autofill::PasswordForm>> password_list =
-      CreatePasswordList();
-  fake_credential_provider_.SetPasswordList(password_list);
-
   EXPECT_CALL(mock_write_file_, Run(_, _, _)).WillOnce(ReturnArg<2>());
   EXPECT_CALL(mock_delete_file_, Run(destination_path_, false));
   EXPECT_CALL(