Skip to content

Commit

Permalink
[Extensions] Clean up test classes in FileSystemChooseEntryFunction.
Browse files Browse the repository at this point in the history 
This function had a series of classes to manage setting and resetting
testing options that relied on global variables and obscured what was
actually happening.

This CL changes this to use an explicit test struct with options
that can be initialized in the test. It then uses base::AutoReset
to set and reset the static pointer to the test struct. This is
much cleaner and clearer than the current code.

Bug: n/a
Change-Id: I6311aec3fa5bb43de82549a66700a00129dbc5f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4609708
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Reviewed-by: Tim <tjudkins@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Auto-Submit: David Bertoni <dbertoni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1159057}
  • Loading branch information
David Bertoni authored and Chromium LUCI CQ committed Jun 16, 2023
1 parent 5462f89 commit e44b7e0
Show file tree
Hide file tree
Showing 8 changed files with 273 additions and 233 deletions.
12 changes: 8 additions & 4 deletions apps/app_restore_service_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,10 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, FileAccessIsSavedToPrefs) {
ASSERT_TRUE(
base::CreateTemporaryFileInDir(temp_directory.GetPath(), &temp_file));

FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
temp_file);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &temp_file};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
FileSystemChooseEntryFunction::RegisterTempExternalFileSystemForTest(
"temp", temp_directory.GetPath());

Expand Down Expand Up @@ -158,8 +160,10 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, MAYBE_FileAccessIsRestored) {
ASSERT_TRUE(
base::CreateTemporaryFileInDir(temp_directory.GetPath(), &temp_file));

FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
temp_file);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &temp_file};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
FileSystemChooseEntryFunction::RegisterTempExternalFileSystemForTest(
"temp", temp_directory.GetPath());

Expand Down
234 changes: 161 additions & 73 deletions chrome/browser/extensions/api/file_system/file_system_apitest.cc
View file

Large diffs are not rendered by default.

71 changes: 48 additions & 23 deletions chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -310,8 +310,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
FileSystemApiOpenExistingFileTest) {
base::FilePath test_file =
GetDriveMountPoint().AppendASCII("root/open_existing.txt");
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
test_file);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &test_file};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_existing",
{.launch_as_platform_app = true}))
<< message_;
Expand All @@ -321,8 +323,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
FileSystemApiOpenExistingFileWithWriteTest) {
base::FilePath test_file =
GetDriveMountPoint().AppendASCII("root/open_existing.txt");
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
test_file);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &test_file};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_existing_with_write",
{.launch_as_platform_app = true}))
<< message_;
Expand All @@ -334,7 +338,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
GetDriveMountPoint().AppendASCII("root/open_existing.txt");
ASSERT_TRUE(base::PathService::OverrideAndCreateIfNeeded(
chrome::DIR_USER_DOCUMENTS, test_file.DirName(), true, false));
FileSystemChooseEntryFunction::SkipPickerAndSelectSuggestedPathForTest picker;
const FileSystemChooseEntryFunction::TestOptions test_options{
.use_suggested_path = true};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(
RunExtensionTest("api_test/file_system/open_multiple_with_suggested_name",
{.launch_as_platform_app = true}))
Expand All @@ -350,8 +357,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
std::vector<base::FilePath> test_files;
test_files.push_back(test_file1);
test_files.push_back(test_file2);
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathsForTest picker(
test_files);
const FileSystemChooseEntryFunction::TestOptions test_options{
.paths_to_be_picked = &test_files};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_multiple_existing",
{.launch_as_platform_app = true}))
<< message_;
Expand All @@ -361,8 +370,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
FileSystemApiOpenDirectoryTest) {
base::FilePath test_directory =
GetDriveMountPoint().AppendASCII("root/subdir");
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
test_directory);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &test_directory};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_directory",
{.launch_as_platform_app = true}))
<< message_;
Expand All @@ -372,8 +383,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
FileSystemApiOpenDirectoryWithWriteTest) {
base::FilePath test_directory =
GetDriveMountPoint().AppendASCII("root/subdir");
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
test_directory);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &test_directory};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(RunExtensionTest("api_test/file_system/open_directory_with_write",
{.launch_as_platform_app = true}))
<< message_;
Expand All @@ -383,8 +396,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
FileSystemApiOpenDirectoryWithoutPermissionTest) {
base::FilePath test_directory =
GetDriveMountPoint().AppendASCII("root/subdir");
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
test_directory);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &test_directory};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(
RunExtensionTest("api_test/file_system/open_directory_without_permission",
{.launch_as_platform_app = true}))
Expand All @@ -395,8 +410,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
FileSystemApiOpenDirectoryWithOnlyWritePermissionTest) {
base::FilePath test_directory =
GetDriveMountPoint().AppendASCII("root/subdir");
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
test_directory);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &test_directory};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(
RunExtensionTest("api_test/file_system/open_directory_with_only_write",
{.launch_as_platform_app = true}))
Expand All @@ -407,8 +424,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
FileSystemApiSaveNewFileTest) {
base::FilePath test_file =
GetDriveMountPoint().AppendASCII("root/save_new.txt");
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
test_file);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &test_file};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(RunExtensionTest("api_test/file_system/save_new",
{.launch_as_platform_app = true}))
<< message_;
Expand All @@ -418,8 +437,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
FileSystemApiSaveExistingFileTest) {
base::FilePath test_file =
GetDriveMountPoint().AppendASCII("root/save_existing.txt");
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
test_file);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &test_file};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(RunExtensionTest("api_test/file_system/save_existing",
{.launch_as_platform_app = true}))
<< message_;
Expand All @@ -429,8 +450,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
FileSystemApiSaveNewFileWithWriteTest) {
base::FilePath test_file =
GetDriveMountPoint().AppendASCII("root/save_new.txt");
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
test_file);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &test_file};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(RunExtensionTest("api_test/file_system/save_new_with_write",
{.launch_as_platform_app = true}))
<< message_;
Expand All @@ -440,8 +463,10 @@ IN_PROC_BROWSER_TEST_F(FileSystemApiTestForDrive,
FileSystemApiSaveExistingFileWithWriteTest) {
base::FilePath test_file =
GetDriveMountPoint().AppendASCII("root/save_existing.txt");
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
test_file);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &test_file};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(RunExtensionTest("api_test/file_system/save_existing_with_write",
{.launch_as_platform_app = true}))
<< message_;
Expand Down
6 changes: 4 additions & 2 deletions chrome/browser/extensions/api/image_writer_private/image_writer_private_apitest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,10 @@ IN_PROC_BROWSER_TEST_F(ImageWriterPrivateApiTest, TestWriteFromFile) {
"test_temp", test_utils_.GetTempDir());

base::FilePath selected_image(test_utils_.GetImagePath());
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
selected_image);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &selected_image};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);

#if !BUILDFLAG(IS_CHROMEOS_ASH)
auto set_up_utility_client_callbacks = [](FakeImageWriterClient* client) {
Expand Down
6 changes: 4 additions & 2 deletions chrome/browser/extensions/native_bindings_apitest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,10 @@ IN_PROC_BROWSER_TEST_F(NativeBindingsApiTest, FileSystemApiGetDisplayPath) {
FileSystemChooseEntryFunction::RegisterTempExternalFileSystemForTest(
"test_root", test_dir);
base::FilePath test_file = test_dir.AppendASCII("text.txt");
FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest picker(
test_file);
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &test_file};
auto reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);
ASSERT_TRUE(RunExtensionTest("native_bindings/instance_of",
{.launch_as_platform_app = true}))
<< message_;
Expand Down
7 changes: 5 additions & 2 deletions chrome/browser/pdf/pdf_extension_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1914,8 +1914,11 @@ IN_PROC_BROWSER_TEST_F(PDFExtensionSaveTest, MAYBE_Save) {
base::FilePath save_path = GetDownloadDir().AppendASCII("edited.pdf");
ASSERT_FALSE(base::PathExists(save_path));

using FileChooser = extensions::FileSystemChooseEntryFunction;
FileChooser::SkipPickerAndAlwaysSelectPathForTest file_picker(save_path);
using extensions::FileSystemChooseEntryFunction;
const FileSystemChooseEntryFunction::TestOptions test_options{
.path_to_be_picked = &save_path};
auto auto_reset_options =
FileSystemChooseEntryFunction::SetOptionsForTesting(test_options);

MimeHandlerViewGuest* guest = LoadTestComboBoxPdfGetMimeHandlerView();
ClickLeftSideOfEditableComboBox(guest);
Expand Down
104 changes: 23 additions & 81 deletions extensions/browser/api/file_system/file_system_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,22 +103,6 @@ namespace ChooseEntry = file_system::ChooseEntry;

namespace {

bool g_skip_picker_for_test = false;
bool g_use_suggested_path_for_test = false;
const base::FilePath* g_path_to_be_picked_for_test = nullptr;
const std::vector<base::FilePath>* g_paths_to_be_picked_for_test = nullptr;
bool g_skip_directory_confirmation_for_test = false;
bool g_allow_directory_access_for_test = false;

void ResetTestValuesToDefaults() {
g_skip_picker_for_test = false;
g_use_suggested_path_for_test = false;
g_path_to_be_picked_for_test = nullptr;
g_paths_to_be_picked_for_test = nullptr;
g_skip_directory_confirmation_for_test = false;
g_allow_directory_access_for_test = false;
}

// Expand the mime-types and extensions provided in an AcceptOption, returning
// them within the passed extension vector. Returns false if no valid types
// were found.
Expand Down Expand Up @@ -443,21 +427,32 @@ ExtensionFunction::ResponseAction FileSystemIsWritableEntryFunction::Run() {
return RespondNow(WithArguments(is_writable));
}

const FileSystemChooseEntryFunction::TestOptions*
FileSystemChooseEntryFunction::g_test_options = nullptr;

base::AutoReset<const FileSystemChooseEntryFunction::TestOptions*>
FileSystemChooseEntryFunction::SetOptionsForTesting(
const TestOptions& options) {
CHECK_EQ(nullptr, g_test_options);
return base::AutoReset<const TestOptions*>(&g_test_options, &options);
}

void FileSystemChooseEntryFunction::ShowPicker(
const ui::SelectFileDialog::FileTypeInfo& file_type_info,
ui::SelectFileDialog::Type picker_type,
const base::FilePath& initial_path) {
// TODO(michaelpg): Use the FileSystemDelegate to override functionality for
// tests instead of using global variables.
if (g_skip_picker_for_test) {
if (g_test_options) {
std::vector<base::FilePath> test_paths;
if (g_use_suggested_path_for_test)
if (g_test_options->use_suggested_path) {
CHECK(!g_test_options->path_to_be_picked &&
!g_test_options->paths_to_be_picked);
test_paths.push_back(initial_path);
else if (g_path_to_be_picked_for_test)
test_paths.push_back(*g_path_to_be_picked_for_test);
else if (g_paths_to_be_picked_for_test)
test_paths = *g_paths_to_be_picked_for_test;

} else if (g_test_options->path_to_be_picked) {
CHECK(!g_test_options->paths_to_be_picked);
test_paths.push_back(*g_test_options->path_to_be_picked);
} else if (g_test_options->paths_to_be_picked) {
test_paths = *g_test_options->paths_to_be_picked;
}
content::GetUIThreadTaskRunner({})->PostTask(
FROM_HERE,
test_paths.size() > 0
Expand All @@ -467,7 +462,6 @@ void FileSystemChooseEntryFunction::ShowPicker(
&FileSystemChooseEntryFunction::FileSelectionCanceled, this));
return;
}

FileSystemDelegate* delegate =
ExtensionsAPIClient::Get()->GetFileSystemDelegate();
DCHECK(delegate);
Expand All @@ -485,59 +479,6 @@ void FileSystemChooseEntryFunction::ShowPicker(
}
}

FileSystemChooseEntryFunction::SkipPickerBaseForTest*
FileSystemChooseEntryFunction::SkipPickerBaseForTest::g_picker = nullptr;

FileSystemChooseEntryFunction::SkipPickerBaseForTest::SkipPickerBaseForTest() {
CHECK(!g_picker);
g_picker = this;
}

FileSystemChooseEntryFunction::SkipPickerBaseForTest::~SkipPickerBaseForTest() {
DCHECK_EQ(this, g_picker);
ResetTestValuesToDefaults();
g_picker = nullptr;
}

FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest::
SkipPickerAndAlwaysSelectPathForTest(const base::FilePath& path,
bool skip_dir_confirmation,
bool allow_directory_access)
: path_(path) {
g_skip_picker_for_test = true;
g_path_to_be_picked_for_test = &path_;
g_skip_directory_confirmation_for_test = skip_dir_confirmation;
g_allow_directory_access_for_test = allow_directory_access;
}

FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest::
~SkipPickerAndAlwaysSelectPathForTest() = default;

FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathsForTest::
SkipPickerAndAlwaysSelectPathsForTest(
const std::vector<base::FilePath>& paths)
: paths_(paths) {
g_skip_picker_for_test = true;
g_paths_to_be_picked_for_test = &paths_;
}

FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathsForTest::
~SkipPickerAndAlwaysSelectPathsForTest() = default;

FileSystemChooseEntryFunction::SkipPickerAndSelectSuggestedPathForTest::
SkipPickerAndSelectSuggestedPathForTest() {
g_skip_picker_for_test = true;
g_use_suggested_path_for_test = true;
}

FileSystemChooseEntryFunction::SkipPickerAndAlwaysCancelForTest::
~SkipPickerAndAlwaysCancelForTest() = default;

FileSystemChooseEntryFunction::SkipPickerAndAlwaysCancelForTest::
SkipPickerAndAlwaysCancelForTest() {
g_skip_picker_for_test = true;
}

// static
void FileSystemChooseEntryFunction::RegisterTempExternalFileSystemForTest(
const std::string& name,
Expand Down Expand Up @@ -614,9 +555,10 @@ void FileSystemChooseEntryFunction::ConfirmDirectoryAccessAsync(
if (check_path != graylisted_path && !check_path.IsParent(graylisted_path))
continue;

if (g_skip_directory_confirmation_for_test) {
if (g_allow_directory_access_for_test)
if (g_test_options && g_test_options->skip_directory_confirmation) {
if (g_test_options->allow_directory_access) {
break;
}
content::GetUIThreadTaskRunner({})->PostTask(
FROM_HERE,
base::BindOnce(&FileSystemChooseEntryFunction::FileSelectionCanceled,
Expand Down

0 comments on commit e44b7e0

Please sign in to comment.