Skip to content

Commit

Permalink
Clean up some chrome/browser/download tests.
Browse files Browse the repository at this point in the history 
- Use ASSERT_TRUE() to avoid potential crashes.
- Use EXPECT_FALSE(cond) instead of EXPECT_TRUE(!cond).
- Use ADD_FAILURE() instead of EXPECT_TRUE(false).
- Simplify DownloadsHistoryDataCollector::WaitForDownloadInfo().
- Shorten a 61 char long variable name.

Change-Id: I23d6d5cad353da95df43bbb6a1f4372b08ff7407
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2241332
Reviewed-by: Xing Liu <xingliu@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#777402}
  • Loading branch information
@leizleiz
leizleiz authored and Commit Bot committed Jun 11, 2020
1 parent 4b34482 commit de96132
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 77 deletions.
20 changes: 9 additions & 11 deletions chrome/browser/download/download_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,18 +359,18 @@ class DownloadsHistoryDataCollector {
explicit DownloadsHistoryDataCollector(Profile* profile)
: profile_(profile) {}

bool WaitForDownloadInfo(std::vector<history::DownloadRow>* results) {
EXPECT_TRUE(results);
std::vector<history::DownloadRow> WaitForDownloadInfo() {
std::vector<history::DownloadRow> results;
HistoryServiceFactory::GetForProfile(profile_,
ServiceAccessType::EXPLICIT_ACCESS)
->QueryDownloads(base::BindLambdaForTesting(
[&](std::vector<history::DownloadRow> rows) {
*results = std::move(rows);
results = std::move(rows);
base::RunLoop::QuitCurrentWhenIdleDeprecated();
}));

content::RunMessageLoop();
return true;
return results;
}

private:
Expand Down Expand Up @@ -2011,7 +2011,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CloseNewTab4) {
manager->GetAllDownloads(&items);
ASSERT_NE(0u, items.size());
DownloadItem* item = items[0];
EXPECT_TRUE(item);
ASSERT_TRUE(item);

// When the download is canceled, the second tab should close.
EXPECT_EQ(item->GetState(), DownloadItem::CANCELLED);
Expand Down Expand Up @@ -2116,9 +2116,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, MAYBE_DownloadHistoryCheck) {
// Get what was stored in the history.
observer.WaitForStored();
// Get the details on what was stored into the history.
std::vector<history::DownloadRow> downloads_in_database;
ASSERT_TRUE(DownloadsHistoryDataCollector(
browser()->profile()).WaitForDownloadInfo(&downloads_in_database));
std::vector<history::DownloadRow> downloads_in_database =
DownloadsHistoryDataCollector(browser()->profile()).WaitForDownloadInfo();
ASSERT_EQ(1u, downloads_in_database.size());

// Confirm history storage is what you expect for an interrupted slow download
Expand Down Expand Up @@ -2181,9 +2180,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadHistoryDangerCheck) {

// Get history details and confirm it's what you expect.
observer.WaitForStored();
std::vector<history::DownloadRow> downloads_in_database;
ASSERT_TRUE(DownloadsHistoryDataCollector(
browser()->profile()).WaitForDownloadInfo(&downloads_in_database));
std::vector<history::DownloadRow> downloads_in_database =
DownloadsHistoryDataCollector(browser()->profile()).WaitForDownloadInfo();
ASSERT_EQ(1u, downloads_in_database.size());
history::DownloadRow& row1(downloads_in_database[0]);
base::FilePath file(FILE_PATH_LITERAL("downloads/dangerous/dangerous.swf"));
Expand Down
10 changes: 5 additions & 5 deletions chrome/browser/download/download_dir_policy_handler_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,19 +89,19 @@ TEST_F(DownloadDirPolicyHandlerTest, SetDownloadToDrive) {
const base::Value* value = NULL;
bool prompt_for_download;
EXPECT_TRUE(store_->GetValue(prefs::kPromptForDownload, &value));
EXPECT_TRUE(value);
ASSERT_TRUE(value);
EXPECT_TRUE(value->GetAsBoolean(&prompt_for_download));
EXPECT_FALSE(prompt_for_download);

bool disable_drive;
EXPECT_TRUE(store_->GetValue(drive::prefs::kDisableDrive, &value));
EXPECT_TRUE(value);
ASSERT_TRUE(value);
EXPECT_TRUE(value->GetAsBoolean(&disable_drive));
EXPECT_FALSE(disable_drive);

std::string download_directory;
EXPECT_TRUE(store_->GetValue(prefs::kDownloadDefaultDirectory, &value));
EXPECT_TRUE(value);
ASSERT_TRUE(value);
EXPECT_TRUE(value->GetAsString(&download_directory));
EXPECT_EQ(download_dir_util::kDriveNamePolicyVariableName,
download_directory);
Expand All @@ -125,7 +125,7 @@ TEST_F(DownloadDirPolicyHandlerTest, SetDownloadToDrive) {

EXPECT_TRUE(
recommended_store_->GetValue(prefs::kDownloadDefaultDirectory, &value));
EXPECT_TRUE(value);
ASSERT_TRUE(value);
EXPECT_TRUE(value->GetAsString(&download_directory));
EXPECT_EQ(std::string(download_dir_util::kDriveNamePolicyVariableName) +
kRelativeToDriveRoot,
Expand All @@ -141,7 +141,7 @@ TEST_F(DownloadDirPolicyHandlerTest, SetDownloadToDrive) {

EXPECT_TRUE(
recommended_store_->GetValue(prefs::kDownloadDefaultDirectory, &value));
EXPECT_TRUE(value);
ASSERT_TRUE(value);
EXPECT_TRUE(value->GetAsString(&download_directory));
EXPECT_EQ(kUserIDHash, download_directory);
}
Expand Down
92 changes: 37 additions & 55 deletions chrome/browser/download/download_frame_policy_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,10 +290,7 @@ class SubframeSameFrameDownloadBrowserTest_Sandbox
: public DownloadFramePolicyBrowserTest,
public ::testing::WithParamInterface<
std::tuple<DownloadSource,
bool /*
enable_blocking_downloads_in_sandbox
*/
,
bool /* enable_blocking_downloads_in_sandbox */,
SandboxOption,
bool /* is_cross_origin */>> {
void SetUpCommandLine(base::CommandLine* command_line) override {
Expand Down Expand Up @@ -364,22 +361,18 @@ class SubframeSameFrameDownloadBrowserTest_AdFrame
: public DownloadFramePolicyBrowserTest,
public ::testing::WithParamInterface<std::tuple<
DownloadSource,
bool /*
enable_blocking_downloads_in_ad_frame_without_user_activation
*/
,
bool /* block_downloads_in_ad_frame_without_user_activation */,
bool /* is_ad_frame */,
bool /* is_cross_origin */,
bool /* initiate_with_gesture */>> {
public:
SubframeSameFrameDownloadBrowserTest_AdFrame() {
bool enable_blocking_downloads_in_ad_frame_without_user_activation;
std::tie(std::ignore,
enable_blocking_downloads_in_ad_frame_without_user_activation,
bool block_downloads_in_ad_frame_without_user_activation;
std::tie(std::ignore, block_downloads_in_ad_frame_without_user_activation,
std::ignore, std::ignore, std::ignore) = GetParam();
scoped_feature_list_.InitWithFeatureState(
blink::features::kBlockingDownloadsInAdFrameWithoutUserActivation,
enable_blocking_downloads_in_ad_frame_without_user_activation);
block_downloads_in_ad_frame_without_user_activation);
}

private:
Expand All @@ -390,25 +383,22 @@ class SubframeSameFrameDownloadBrowserTest_AdFrame
// correctly. This test specifically tests ad related behaviors.
IN_PROC_BROWSER_TEST_P(SubframeSameFrameDownloadBrowserTest_AdFrame, Download) {
DownloadSource source;
bool enable_blocking_downloads_in_ad_frame_without_user_activation;
bool block_downloads_in_ad_frame_without_user_activation;
bool is_ad_frame;
bool is_cross_origin;
bool initiate_with_gesture;
std::tie(source,
enable_blocking_downloads_in_ad_frame_without_user_activation,
std::tie(source, block_downloads_in_ad_frame_without_user_activation,
is_ad_frame, is_cross_origin, initiate_with_gesture) = GetParam();
SCOPED_TRACE(
::testing::Message()
<< "source = " << source << ", "
<< "is_ad_frame = " << is_ad_frame << ", "
<< "enable_blocking_downloads_in_ad_frame_without_user_activation = "
<< enable_blocking_downloads_in_ad_frame_without_user_activation << ", "
<< "is_cross_origin = " << is_cross_origin << ", "
<< "initiate_with_gesture = " << initiate_with_gesture);

bool expect_download =
!enable_blocking_downloads_in_ad_frame_without_user_activation ||
initiate_with_gesture || !is_ad_frame;
SCOPED_TRACE(::testing::Message()
<< "source = " << source << ", "
<< "is_ad_frame = " << is_ad_frame << ", "
<< "block_downloads_in_ad_frame_without_user_activation = "
<< block_downloads_in_ad_frame_without_user_activation << ", "
<< "is_cross_origin = " << is_cross_origin << ", "
<< "initiate_with_gesture = " << initiate_with_gesture);

bool expect_download = !block_downloads_in_ad_frame_without_user_activation ||
initiate_with_gesture || !is_ad_frame;
bool expect_download_in_ad_frame_without_user_activation =
is_ad_frame && !initiate_with_gesture;

Expand Down Expand Up @@ -452,9 +442,7 @@ INSTANTIATE_TEST_SUITE_P(
class OtherFrameNavigationDownloadBrowserTest_Sandbox
: public DownloadFramePolicyBrowserTest,
public ::testing::WithParamInterface<
std::tuple<bool /* enable_blocking_downloads_in_sandbox
*/
,
std::tuple<bool /* enable_blocking_downloads_in_sandbox */,
bool /* is_cross_origin */,
OtherFrameNavigationType>> {
void SetUpCommandLine(base::CommandLine* command_line) override {
Expand Down Expand Up @@ -530,20 +518,18 @@ INSTANTIATE_TEST_SUITE_P(
class OtherFrameNavigationDownloadBrowserTest_AdFrame
: public DownloadFramePolicyBrowserTest,
public ::testing::WithParamInterface<std::tuple<
bool /* enable_blocking_downloads_in_ad_frame_without_user_activation
*/
,
bool /* block_downloads_in_ad_frame_without_user_activation */,
bool /* is_cross_origin */,
bool /* initiate_with_gesture */,
OtherFrameNavigationType>> {
public:
OtherFrameNavigationDownloadBrowserTest_AdFrame() {
bool enable_blocking_downloads_in_ad_frame_without_user_activation;
std::tie(enable_blocking_downloads_in_ad_frame_without_user_activation,
std::ignore, std::ignore, std::ignore) = GetParam();
bool block_downloads_in_ad_frame_without_user_activation;
std::tie(block_downloads_in_ad_frame_without_user_activation, std::ignore,
std::ignore, std::ignore) = GetParam();
scoped_feature_list_.InitWithFeatureState(
blink::features::kBlockingDownloadsInAdFrameWithoutUserActivation,
enable_blocking_downloads_in_ad_frame_without_user_activation);
block_downloads_in_ad_frame_without_user_activation);
}

private:
Expand All @@ -554,20 +540,19 @@ class OtherFrameNavigationDownloadBrowserTest_AdFrame
// only one frame being ad. Also covers the remote frame navigation path.
IN_PROC_BROWSER_TEST_P(OtherFrameNavigationDownloadBrowserTest_AdFrame,
Download) {
bool enable_blocking_downloads_in_ad_frame_without_user_activation;
bool block_downloads_in_ad_frame_without_user_activation;
bool is_cross_origin;
bool initiate_with_gesture;
OtherFrameNavigationType other_frame_navigation_type;
std::tie(enable_blocking_downloads_in_ad_frame_without_user_activation,
is_cross_origin, initiate_with_gesture,
other_frame_navigation_type) = GetParam();
SCOPED_TRACE(
::testing::Message()
<< "enable_blocking_downloads_in_ad_frame_without_user_activation = "
<< enable_blocking_downloads_in_ad_frame_without_user_activation << ", "
<< "is_cross_origin = " << is_cross_origin << ", "
<< "initiate_with_gesture = " << initiate_with_gesture << ", "
<< "other_frame_navigation_type = " << other_frame_navigation_type);
std::tie(block_downloads_in_ad_frame_without_user_activation, is_cross_origin,
initiate_with_gesture, other_frame_navigation_type) = GetParam();
SCOPED_TRACE(::testing::Message()
<< "block_downloads_in_ad_frame_without_user_activation = "
<< block_downloads_in_ad_frame_without_user_activation << ", "
<< "is_cross_origin = " << is_cross_origin << ", "
<< "initiate_with_gesture = " << initiate_with_gesture << ", "
<< "other_frame_navigation_type = "
<< other_frame_navigation_type);

bool prevent_frame_busting =
other_frame_navigation_type ==
Expand All @@ -586,8 +571,7 @@ IN_PROC_BROWSER_TEST_P(OtherFrameNavigationDownloadBrowserTest_AdFrame,
bool expect_gesture = initiate_with_gesture && !is_cross_origin;

bool expect_download =
!enable_blocking_downloads_in_ad_frame_without_user_activation ||
expect_gesture;
!block_downloads_in_ad_frame_without_user_activation || expect_gesture;

SetNumDownloadsExpectation(expect_download);

Expand Down Expand Up @@ -649,9 +633,7 @@ class TopFrameSameFrameDownloadBrowserTest
: public DownloadFramePolicyBrowserTest,
public ::testing::WithParamInterface<
std::tuple<DownloadSource,
bool /* enable_blocking_downloads_in_sandbox
*/
,
bool /* enable_blocking_downloads_in_sandbox */,
SandboxOption>> {
void SetUpCommandLine(base::CommandLine* command_line) override {
bool enable_blocking_downloads_in_sandbox;
Expand Down Expand Up @@ -756,7 +738,7 @@ IN_PROC_BROWSER_TEST_P(
content::JsReplace("document.querySelector('iframe').src = $1",
download_url)));
navigation_observer.WaitForNavigationFinished();
EXPECT_TRUE(!navigation_observer.was_successful());
EXPECT_FALSE(navigation_observer.was_successful());

GetHistogramTester()->ExpectBucketCount(
"Blink.UseCounter.Features", blink::mojom::WebFeature::kDownloadInSandbox,
Expand Down Expand Up @@ -799,7 +781,7 @@ IN_PROC_BROWSER_TEST_P(DownloadFramePolicyBrowserTest_UpdateIframeSandboxFlags,
EXPECT_TRUE(ExecJs(GetSubframeRfh(),
content::JsReplace("top.location = $1", download_url)));
navigation_observer.WaitForNavigationFinished();
EXPECT_TRUE(!navigation_observer.was_successful());
EXPECT_FALSE(navigation_observer.was_successful());

GetHistogramTester()->ExpectBucketCount(
"Blink.UseCounter.Features", blink::mojom::WebFeature::kDownloadInSandbox,
Expand Down
8 changes: 3 additions & 5 deletions chrome/browser/download/download_history_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ class FakeHistoryAdapter : public DownloadHistory::HistoryAdapter {

void ExpectQueryDownloadsDone() {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
EXPECT_TRUE(!expect_query_downloads_.has_value());
EXPECT_FALSE(expect_query_downloads_.has_value());
}

void FailCreateDownload() {
Expand Down Expand Up @@ -164,10 +164,8 @@ class FakeHistoryAdapter : public DownloadHistory::HistoryAdapter {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
IdSet differences = base::STLSetDifference<IdSet>(ids, remove_downloads_);
for (auto different = differences.begin(); different != differences.end();
++different) {
EXPECT_TRUE(false) << *different;
}
for (int different : differences)
ADD_FAILURE() << different;
remove_downloads_.clear();
}

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/download/offline_item_utils_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ TEST_F(OfflineItemUtilsTest, BasicConversions) {
EXPECT_EQ(allow_metered, offline_item.allow_metered);
EXPECT_EQ(received_bytes, offline_item.received_bytes);
EXPECT_EQ(received_bytes, offline_item.progress.value);
EXPECT_TRUE(offline_item.progress.max.has_value());
ASSERT_TRUE(offline_item.progress.max.has_value());
EXPECT_EQ(total_bytes, offline_item.progress.max.value());
EXPECT_EQ(OfflineItemProgressUnit::BYTES, offline_item.progress.unit);
EXPECT_EQ(time_remaining_ms, offline_item.time_remaining_ms);
Expand Down

0 comments on commit de96132

Please sign in to comment.