Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2690853008:
Verify FormsTestCase forms submission result. (Closed)
DescriptionVerify FormsTestCase forms submission result.
Add a missing GREYAssert to the |waitWithTimeout:| calls in
|waitForExpectedResponse:| and |waitForTabHistoryView|. Fix silently
failing tests (due to previously missing asserts) by implementing
/printformdata page to display the request method and body.
BUG=685439
Review-Url: https://codereview.chromium.org/2690853008
Cr-Commit-Position: refs/heads/master@{#450717}
Committed: https://chromium.googlesource.com/chromium/src/+/59a7ad896fbc906833b38120fc8776a368fe28d2
Patch Set 1 #
Total comments: 13
Patch Set 2 : Respond to comments. #
Total comments: 14
Patch Set 3 : Respond to comments. #Messages
Total messages: 12 (4 generated)
michaeldo@chromium.org changed reviewers: + eugenebut@chromium.org
https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... File ios/chrome/browser/web/forms_egtest.mm (right): https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:29: const char kHTTPScheme[] = "http://"; kHttpScheme https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:32: const char kHttpServerDomain[] = "localhost"; nit: kHttpServerHost ? https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:80: GURL printFormDataUrl = web::test::HttpServer::MakeUrl( print_form_data_url https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:81: std::string(kHTTPScheme) + kPrintFormDataPath); How about using url::SchemeHostPort instead of strings concatenation? https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:90: const char* formHtml = form_html https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:136: GURL url = web::test::HttpServer::MakeUrl(std::string(kHTTPScheme) + Optional nit: Do you want to create TestResponseProvider::GetPrintFormURL() static function and use it here and inside TestResponseProvider?
https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... File ios/chrome/browser/web/forms_egtest.mm (right): https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:29: const char kHTTPScheme[] = "http://"; On 2017/02/14 21:44:43, Eugene But wrote: > kHttpScheme Done. https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:32: const char kHttpServerDomain[] = "localhost"; On 2017/02/14 21:44:43, Eugene But wrote: > nit: kHttpServerHost ? Done. https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:80: GURL printFormDataUrl = web::test::HttpServer::MakeUrl( On 2017/02/14 21:44:43, Eugene But wrote: > print_form_data_url Done. https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:81: std::string(kHTTPScheme) + kPrintFormDataPath); On 2017/02/14 21:44:43, Eugene But wrote: > How about using url::SchemeHostPort instead of strings concatenation? I agree that url::SchemeHostPort is more ideal, but it seems too fragile to add one call to it here. IMO all MakeUrl calls would need to be migrated at once as the constants would need to be modified to work with it. For example, url::SchemeHostPort expects no "://" with the scheme and no slashes on the path. With the current constants, url::SchemeHostPort simply returns an invalid GURL. Please let me know if you'd like me to make the change still and I can do it in a followup CL or maybe even more ideal add a web::test::HttpServer::SchemeHost method? This would remove the need to ask the server singleton for the port number. https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:90: const char* formHtml = On 2017/02/14 21:44:43, Eugene But wrote: > form_html Done. https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:136: GURL url = web::test::HttpServer::MakeUrl(std::string(kHTTPScheme) + On 2017/02/14 21:44:43, Eugene But wrote: > Optional nit: Do you want to create TestResponseProvider::GetPrintFormURL() > static function and use it here and inside TestResponseProvider? Done. This was a great cleanup point so I similarly did it with TestResponseProvider::GetFormURL()
https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... File ios/chrome/browser/web/forms_egtest.mm (right): https://codereview.chromium.org/2690853008/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:81: std::string(kHTTPScheme) + kPrintFormDataPath); On 2017/02/15 00:56:34, michaeldo wrote: > On 2017/02/14 21:44:43, Eugene But wrote: > > How about using url::SchemeHostPort instead of strings concatenation? > > I agree that url::SchemeHostPort is more ideal, but it seems too fragile to add > one call to it here. IMO all MakeUrl calls would need to be migrated at once as > the constants would need to be modified to work with it. For example, > url::SchemeHostPort expects no "://" with the scheme and no slashes on the path. > With the current constants, url::SchemeHostPort simply returns an invalid GURL. > Please let me know if you'd like me to make the change still and I can do it in > a followup CL or maybe even more ideal add a web::test::HttpServer::SchemeHost > method? This would remove the need to ask the server singleton for the port > number. I suggested to use url::SchemeHostPort to avoid strings concatenations for URLs. But adding static methods which return URLs looks like a better direction. https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... File ios/chrome/browser/web/forms_egtest.mm (right): https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:61: static GURL GetFormURL() { GetFormUrl https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:65: static GURL GetPrintFormDataURL() { GetPrintFormDataUrl https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:79: return url.host() == kHttpServerHost && Now when you have static methods which return forms, do you want to use them here: return url == GetFormUrl() || url == GetPrintFormDataUrl() || url == GetRedirectUrl() || url == GetRedirectFormUrl(); This way you can drop "scheme", "path", and "host" separate constants and simply use old kGenericUrl, kPrintFormDataUrl, etc... https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:246: std::string(kHttpScheme) + kGenericPath)]; Could you please create static method for this URL as well. https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:274: std::string(kHttpScheme) + kGenericPath)]; ditto https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:279: id<GREYMatcher> historyItem = grey_text(base::SysUTF8ToNSString( Optional nit: How about creating a function for this matcher: id<GREYMatcher> HistoryUrlLabel() { return grey_text(base::SysUTF8ToNSString(TestResponseProvider::GetPrintFormDataURL().spec())); } https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:320: [ChromeEarlGrey loadURL:web::test::HttpServer::MakeUrl( Could you please create static method for this URL as well.
https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... File ios/chrome/browser/web/forms_egtest.mm (right): https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:61: static GURL GetFormURL() { On 2017/02/15 01:28:04, Eugene But wrote: > GetFormUrl Done. https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:65: static GURL GetPrintFormDataURL() { On 2017/02/15 01:28:04, Eugene But wrote: > GetPrintFormDataUrl Done. https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:79: return url.host() == kHttpServerHost && On 2017/02/15 01:28:04, Eugene But wrote: > Now when you have static methods which return forms, do you want to use them > here: > > return url == GetFormUrl() || url == GetPrintFormDataUrl() || url == > GetRedirectUrl() || url == GetRedirectFormUrl(); > > This way you can drop "scheme", "path", and "host" separate constants and simply > use old kGenericUrl, kPrintFormDataUrl, etc... Definitely, I agree that is much nicer. https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:246: std::string(kHttpScheme) + kGenericPath)]; On 2017/02/15 01:28:04, Eugene But wrote: > Could you please create static method for this URL as well. Done. https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:274: std::string(kHttpScheme) + kGenericPath)]; On 2017/02/15 01:28:04, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:279: id<GREYMatcher> historyItem = grey_text(base::SysUTF8ToNSString( On 2017/02/15 01:28:04, Eugene But wrote: > Optional nit: How about creating a function for this matcher: > > id<GREYMatcher> HistoryUrlLabel() { > return > grey_text(base::SysUTF8ToNSString(TestResponseProvider::GetPrintFormDataURL().spec())); > } The matcher is only used once, so I'll leave it alone for now. https://codereview.chromium.org/2690853008/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:320: [ChromeEarlGrey loadURL:web::test::HttpServer::MakeUrl( On 2017/02/15 01:28:04, Eugene But wrote: > Could you please create static method for this URL as well. Done.
Thank you! lgtm
The CQ bit was checked by michaeldo@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487173900393280,
"parent_rev": "0894ba17f0f90d3afe45944881ec4685081b6271", "commit_rev":
"59a7ad896fbc906833b38120fc8776a368fe28d2"}
Message was sent while issue was closed.
Description was changed from ========== Verify FormsTestCase forms submission result. Add a missing GREYAssert to the |waitWithTimeout:| calls in |waitForExpectedResponse:| and |waitForTabHistoryView|. Fix silently failing tests (due to previously missing asserts) by implementing /printformdata page to display the request method and body. BUG=685439 ========== to ========== Verify FormsTestCase forms submission result. Add a missing GREYAssert to the |waitWithTimeout:| calls in |waitForExpectedResponse:| and |waitForTabHistoryView|. Fix silently failing tests (due to previously missing asserts) by implementing /printformdata page to display the request method and body. BUG=685439 Review-Url: https://codereview.chromium.org/2690853008 Cr-Commit-Position: refs/heads/master@{#450717} Committed: https://chromium.googlesource.com/chromium/src/+/59a7ad896fbc906833b38120fc87... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/59a7ad896fbc906833b38120fc87... |
