[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}diff --git a/components/password_manager/core/browser/export/password_manager_exporter.cc b/components/password_manager/core/browser/export/password_manager_exporter.cc
index 3265add7..2a330d3 100644
--- a/components/password_manager/core/browser/export/password_manager_exporter.cc
+++ b/components/password_manager/core/browser/export/password_manager_exporter.cc
@@ -31,13 +31,12 @@
// 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 @@
: 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 @@
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() {
diff --git a/components/password_manager/core/browser/export/password_manager_exporter.h b/components/password_manager/core/browser/export/password_manager_exporter.h
index 72ba42d..e4b685a 100644
--- a/components/password_manager/core/browser/export/password_manager_exporter.h
+++ b/components/password_manager/core/browser/export/password_manager_exporter.h
@@ -29,6 +29,8 @@
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 @@
// 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 @@
// 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.
diff --git a/components/password_manager/core/browser/export/password_manager_exporter_unittest.cc b/components/password_manager/core/browser/export/password_manager_exporter_unittest.cc
index ba31cc8e..3de7778f 100644
--- a/components/password_manager/core/browser/export/password_manager_exporter_unittest.cc
+++ b/components/password_manager/core/browser/export/password_manager_exporter_unittest.cc
@@ -78,17 +78,6 @@
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 @@
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 @@
};
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 @@
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 @@
// 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 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, 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, 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, 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 @@
// 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(
