Skip to content

GitLab

T
tangled
Commit a44e9ca9 authored by mgiuca's avatar mgiuca Committed by Commit bot
Browse files
Options

Clean up app list webstore_provider_browsertest. 

Record whether a query is pending and use it to decide when to wait
(previously, it would wait if ANY previous query was ever pending, which
"just happened" to work for the specific test cases).

Changed some ASSERT into EXPECT.

Use more descriptive special strings for HTTP errors, rather than the
exact same string as the query, which is confusing.

Document the test cases properly.

Reformat the source according to ClangFormat.

BUG=481837

Review URL: https://codereview.chromium.org/1133543003

Cr-Commit-Position: refs/heads/master@{#329327}
parent b716b0b7
Hide whitespace changes
Inline Side-by-side
Showing with 98 additions and 67 deletions
chrome/browser/ui/app_list/search/webstore/webstore_provider.cc
View file @ a44e9ca9
...@@ -54,8 +54,10 @@ extensions::Manifest::Type ParseItemType(const std::string& item_type_str) { ...@@ -54,8 +54,10 @@ extensions::Manifest::Type ParseItemType(const std::string& item_type_str) {
WebstoreProvider::WebstoreProvider(Profile* profile, WebstoreProvider::WebstoreProvider(Profile* profile,
AppListControllerDelegate* controller) AppListControllerDelegate* controller)
: WebserviceSearchProvider(profile), : WebserviceSearchProvider(profile),
controller_(controller){} controller_(controller),
query_pending_(false) {
}
WebstoreProvider::~WebstoreProvider() {} WebstoreProvider::~WebstoreProvider() {}
...@@ -84,6 +86,7 @@ void WebstoreProvider::Start(bool /*is_voice_query*/, ...@@ -84,6 +86,7 @@ void WebstoreProvider::Start(bool /*is_voice_query*/,
profile_->GetRequestContext())); profile_->GetRequestContext()));
} }
query_pending_ = true;
StartThrottledQuery(base::Bind(&WebstoreProvider::StartQuery, StartThrottledQuery(base::Bind(&WebstoreProvider::StartQuery,
base::Unretained(this))); base::Unretained(this)));
...@@ -111,6 +114,7 @@ void WebstoreProvider::OnWebstoreSearchFetched( ...@@ -111,6 +114,7 @@ void WebstoreProvider::OnWebstoreSearchFetched(
scoped_ptr<base::DictionaryValue> json) { scoped_ptr<base::DictionaryValue> json) {
ProcessWebstoreSearchResults(json.get()); ProcessWebstoreSearchResults(json.get());
cache_->Put(WebserviceCache::WEBSTORE, query_, json.Pass()); cache_->Put(WebserviceCache::WEBSTORE, query_, json.Pass());
query_pending_ = false;
if (!webstore_search_fetched_callback_.is_null()) if (!webstore_search_fetched_callback_.is_null())
webstore_search_fetched_callback_.Run(); webstore_search_fetched_callback_.Run();
......
chrome/browser/ui/app_list/search/webstore/webstore_provider.h
View file @ a44e9ca9
...@@ -58,6 +58,9 @@ class WebstoreProvider : public WebserviceSearchProvider{ ...@@ -58,6 +58,9 @@ class WebstoreProvider : public WebserviceSearchProvider{
// The current query. // The current query.
std::string query_; std::string query_;
// Whether there is currently a query pending.
bool query_pending_;
DISALLOW_COPY_AND_ASSIGN(WebstoreProvider); DISALLOW_COPY_AND_ASSIGN(WebstoreProvider);
}; };
......
chrome/browser/ui/app_list/search/webstore/webstore_provider_browsertest.cc
View file @ a44e9ca9
...@@ -35,41 +35,43 @@ namespace test { ...@@ -35,41 +35,43 @@ namespace test {
namespace { namespace {
// Mock results. // Mock results.
const char kOneResult[] = "{" const char kOneResult[] =
"{"
"\"search_url\": \"http://host/search\"," "\"search_url\": \"http://host/search\","
"\"results\":[" "\"results\":["
"{" " {"
"\"id\": \"app1_id\"," " \"id\": \"app1_id\","
"\"localized_name\": \"app1 name\"," " \"localized_name\": \"app1 name\","
"\"icon_url\": \"http://host/icon\"," " \"icon_url\": \"http://host/icon\","
"\"is_paid\": false" " \"is_paid\": false"
"}" " }"
"]}"; "]}";
const char kThreeResults[] = "{" const char kThreeResults[] =
"{"
"\"search_url\": \"http://host/search\"," "\"search_url\": \"http://host/search\","
"\"results\":[" "\"results\":["
"{" " {"
"\"id\": \"app1_id\"," " \"id\": \"app1_id\","
"\"localized_name\": \"one\"," " \"localized_name\": \"one\","
"\"icon_url\": \"http://host/icon1\"," " \"icon_url\": \"http://host/icon1\","
"\"is_paid\": true," " \"is_paid\": true,"
"\"item_type\": \"PLATFORM_APP\"" " \"item_type\": \"PLATFORM_APP\""
"}," " },"
"{" " {"
"\"id\": \"app2_id\"," " \"id\": \"app2_id\","
"\"localized_name\": \"two\"," " \"localized_name\": \"two\","
"\"icon_url\": \"http://host/icon2\"," " \"icon_url\": \"http://host/icon2\","
"\"is_paid\": false," " \"is_paid\": false,"
"\"item_type\": \"HOSTED_APP\"" " \"item_type\": \"HOSTED_APP\""
"}," " },"
"{" " {"
"\"id\": \"app3_id\"," " \"id\": \"app3_id\","
"\"localized_name\": \"three\"," " \"localized_name\": \"three\","
"\"icon_url\": \"http://host/icon3\"," " \"icon_url\": \"http://host/icon3\","
"\"is_paid\": false," " \"is_paid\": false,"
"\"item_type\": \"LEGACY_PACKAGED_APP\"" " \"item_type\": \"LEGACY_PACKAGED_APP\""
"}" " }"
"]}"; "]}";
struct ParsedSearchResult { struct ParsedSearchResult {
...@@ -81,17 +83,31 @@ struct ParsedSearchResult { ...@@ -81,17 +83,31 @@ struct ParsedSearchResult {
size_t num_actions; size_t num_actions;
}; };
ParsedSearchResult kParsedOneResult[] = {{"app1_id", "app1 name", ParsedSearchResult kParsedOneResult[] = {{"app1_id",
"http://host/icon", false, "app1 name",
Manifest::TYPE_UNKNOWN, 1}}; "http://host/icon",
false,
ParsedSearchResult kParsedThreeResults[] = { Manifest::TYPE_UNKNOWN,
{"app1_id", "one", "http://host/icon1", true, Manifest::TYPE_PLATFORM_APP, 1}};
1},
{"app2_id", "two", "http://host/icon2", false, Manifest::TYPE_HOSTED_APP, ParsedSearchResult kParsedThreeResults[] = {{"app1_id",
1}, "one",
{"app3_id", "three", "http://host/icon3", false, "http://host/icon1",
Manifest::TYPE_LEGACY_PACKAGED_APP, 1}}; true,
Manifest::TYPE_PLATFORM_APP,
1},
{"app2_id",
"two",
"http://host/icon2",
false,
Manifest::TYPE_HOSTED_APP,
1},
{"app3_id",
"three",
"http://host/icon3",
false,
Manifest::TYPE_LEGACY_PACKAGED_APP,
1}};
} // namespace } // namespace
...@@ -131,7 +147,7 @@ class WebstoreProviderTest : public InProcessBrowserTest { ...@@ -131,7 +147,7 @@ class WebstoreProviderTest : public InProcessBrowserTest {
const std::string& mock_server_response) { const std::string& mock_server_response) {
webstore_provider_->Start(false, base::UTF8ToUTF16(query)); webstore_provider_->Start(false, base::UTF8ToUTF16(query));
if (webstore_provider_->webstore_search_ && !mock_server_response.empty()) { if (webstore_provider_->query_pending_ && !mock_server_response.empty()) {
mock_server_response_ = mock_server_response; mock_server_response_ = mock_server_response;
DCHECK(!run_loop_); DCHECK(!run_loop_);
...@@ -163,7 +179,7 @@ class WebstoreProviderTest : public InProcessBrowserTest { ...@@ -163,7 +179,7 @@ class WebstoreProviderTest : public InProcessBrowserTest {
ASSERT_EQ(expected_result_size, webstore_provider_->results().size()); ASSERT_EQ(expected_result_size, webstore_provider_->results().size());
for (size_t i = 0; i < expected_result_size; ++i) { for (size_t i = 0; i < expected_result_size; ++i) {
const SearchResult* result = webstore_provider_->results()[i]; const SearchResult* result = webstore_provider_->results()[i];
ASSERT_EQ(extensions::Extension::GetBaseURLFromExtensionId( EXPECT_EQ(extensions::Extension::GetBaseURLFromExtensionId(
expected_results[i].id).spec(), expected_results[i].id).spec(),
result->id()); result->id());
EXPECT_EQ(std::string(expected_results[i].title), EXPECT_EQ(std::string(expected_results[i].title),
...@@ -197,9 +213,9 @@ class WebstoreProviderTest : public InProcessBrowserTest { ...@@ -197,9 +213,9 @@ class WebstoreProviderTest : public InProcessBrowserTest {
scoped_ptr<BasicHttpResponse> response(new BasicHttpResponse); scoped_ptr<BasicHttpResponse> response(new BasicHttpResponse);
if (request.relative_url.find("/jsonsearch?") != std::string::npos) { if (request.relative_url.find("/jsonsearch?") != std::string::npos) {
if (mock_server_response_ == "404") { if (mock_server_response_ == "ERROR_NOT_FOUND") {
response->set_code(net::HTTP_NOT_FOUND); response->set_code(net::HTTP_NOT_FOUND);
} else if (mock_server_response_ == "500") { } else if (mock_server_response_ == "ERROR_INTERNAL_SERVER_ERROR") {
response->set_code(net::HTTP_INTERNAL_SERVER_ERROR); response->set_code(net::HTTP_INTERNAL_SERVER_ERROR);
} else { } else {
response->set_code(net::HTTP_OK); response->set_code(net::HTTP_OK);
...@@ -233,21 +249,29 @@ IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, Basic) { ...@@ -233,21 +249,29 @@ IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, Basic) {
const ParsedSearchResult* expected_results; const ParsedSearchResult* expected_results;
size_t expected_result_size; size_t expected_result_size;
} kTestCases[] = { } kTestCases[] = {
// "Search in web store" result with query text itself is used for // Note: If a search results in an error, or returns 0 results, we expect
// synchronous placeholder, bad server response etc. // the webstore provider to leave a placeholder "search in web store"
{"synchronous", "", "synchronous", NULL, 0 }, // result with the same title as the search query. So all cases where
{"404", "404", "404", NULL, 0 }, // |expected_result_titles| == |query| means we are expecting an error.
{"500", "500", "500", NULL, 0 },
{"bad json", "invalid json", "bad json", NULL, 0 }, // A search that returns 0 results.
// Good results. {"synchronous", "", "synchronous", NULL, 0},
{"1 result", kOneResult, "app1 name", kParsedOneResult, 1 }, // Getting an error response from the server (note: the responses
{"3 result", kThreeResults, "one,two,three", kParsedThreeResults, 3 }, // "ERROR_NOT_FOUND" and "ERROR_INTERNAL_SERVER_ERROR" are treated
// specially by HandleResponse).
{"404", "ERROR_NOT_FOUND", "404", NULL, 0},
{"500", "ERROR_INTERNAL_SERVER_ERROR", "500", NULL, 0},
// Getting bad JSON from the server.
{"bad json", "invalid json", "bad json", NULL, 0},
// Good results.
{"1 result", kOneResult, "app1 name", kParsedOneResult, 1},
{"3 result", kThreeResults, "one,two,three", kParsedThreeResults, 3},
}; };
for (size_t i = 0; i < arraysize(kTestCases); ++i) { for (size_t i = 0; i < arraysize(kTestCases); ++i) {
if (kTestCases[i].expected_result_titles) { if (kTestCases[i].expected_result_titles) {
RunQuery(kTestCases[i].query, kTestCases[i].mock_server_response); RunQuery(kTestCases[i].query, kTestCases[i].mock_server_response);
ASSERT_EQ(kTestCases[i].expected_result_titles, GetResultTitles()) EXPECT_EQ(kTestCases[i].expected_result_titles, GetResultTitles())
<< "Case " << i << ": q=" << kTestCases[i].query; << "Case " << i << ": q=" << kTestCases[i].query;
if (kTestCases[i].expected_results) { if (kTestCases[i].expected_results) {
...@@ -262,17 +286,17 @@ IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, NoSearchForSensitiveData) { ...@@ -262,17 +286,17 @@ IN_PROC_BROWSER_TEST_F(WebstoreProviderTest, NoSearchForSensitiveData) {
// None of the following input strings should be accepted because they may // None of the following input strings should be accepted because they may
// contain private data. // contain private data.
const char* inputs[] = { const char* inputs[] = {
// file: scheme is bad. // file: scheme is bad.
"file://filename", "file://filename",
"FILE://filename", "FILE://filename",
// URLs with usernames, ports, queries or refs are bad. // URLs with usernames, ports, queries or refs are bad.
"http://username:password@hostname/", "http://username:password@hostname/",
"http://www.example.com:1000", "http://www.example.com:1000",
"http://foo:1000", "http://foo:1000",
"http://hostname/?query=q", "http://hostname/?query=q",
"http://hostname/path#ref", "http://hostname/path#ref",
// A https URL with path is bad. // A https URL with path is bad.
"https://hostname/path", "https://hostname/path",
}; };
for (size_t i = 0; i < arraysize(inputs); ++i) { for (size_t i = 0; i < arraysize(inputs); ++i) {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment