Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2605223002:
Created test for no-op history state operations. (Closed)
|
Created:
3 years, 11 months ago by kkhorimoto Modified:
3 years, 11 months ago Reviewers:
Eugene But (OOO till 7-30) CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreated test for no-op history state operations.
BUG=673022
Review-Url: https://codereview.chromium.org/2605223002
Cr-Commit-Position: refs/heads/master@{#445656}
Committed: https://chromium.googlesource.com/chromium/src/+/2a90b3aac07b05d5639f5de565c33a66ecc84ff3
Patch Set 1 #
Total comments: 24
Patch Set 2 : eugene's comments #
Total comments: 17
Patch Set 3 : eugene's comments #
Dependent Patchsets: Messages
Total messages: 13 (5 generated)
kkhorimoto@chromium.org changed reviewers: + eugenebut@chromium.org
https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... File ios/web/navigation/history_state_operations_inttest.mm (right): https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:10: #include "ios/web/public/navigation_item.h" s/include/import https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:11: #include "ios/web/public/navigation_manager.h" ditto https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:15: #include "ios/web/public/web_state/web_state.h" ditto https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:61: void ExpectPageLoad(const GURL& url) { Could you please avoid code duplication. Same code present here: https://codereview.chromium.org/2599233002/patch/20001/30002 https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:85: class HistoryStateOperationsTest : public web::WebIntTest { Please explain in the comments the purpose of this test. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:85: class HistoryStateOperationsTest : public web::WebIntTest { This class have many common things from this CL: https://codereview.chromium.org/2599233002/patch/20001/30002 Can you create a common class to avoid code duplication? https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:121: web::NavigationItem* current_item() { s/current_item/GetLastCommittedItem When I see current_item I always have to look up the implementation, because implementations of this method are all different :) https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:143: // Set the parameters to use for state operations on the test page. Could you please be more specific with comments. Does it actually perform replace state or something? https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:157: base::test::ios::WaitUntilCondition(^bool { Per comments in my previous CLs, could you please use API that synchronously execute JS. Here and everywhere in tests. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:222: TEST_F(HistoryStateOperationsTest, NoOps) { Looks like this test have multiple test cases inside. Do you want to split it? https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:223: std::string empty_state(""); std::string empty_state; Same comment for the next line. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:225: GURL unresolvable_url("http://www.google.invalid"); Does this URL hit network? Do you need to use localhost URLs instead?
https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... File ios/web/navigation/history_state_operations_inttest.mm (right): https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:85: class HistoryStateOperationsTest : public web::WebIntTest { On 2016/12/29 17:06:50, Eugene But wrote: > This class have many common things from this CL: > https://codereview.chromium.org/2599233002/patch/20001/30002 > Can you create a common class to avoid code duplication? Or maybe move this stuff to web::WebIntTest?
https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... File ios/web/navigation/history_state_operations_inttest.mm (right): https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:10: #include "ios/web/public/navigation_item.h" On 2016/12/29 17:06:50, Eugene But wrote: > s/include/import Done. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:11: #include "ios/web/public/navigation_manager.h" On 2016/12/29 17:06:50, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:15: #include "ios/web/public/web_state/web_state.h" On 2016/12/29 17:06:50, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:61: void ExpectPageLoad(const GURL& url) { On 2016/12/29 17:06:50, Eugene But wrote: > Could you please avoid code duplication. Same code present here: > https://codereview.chromium.org/2599233002/patch/20001/30002 Done. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:85: class HistoryStateOperationsTest : public web::WebIntTest { On 2016/12/29 17:31:53, Eugene But wrote: > On 2016/12/29 17:06:50, Eugene But wrote: > > This class have many common things from this CL: > > https://codereview.chromium.org/2599233002/patch/20001/30002 > > Can you create a common class to avoid code duplication? > Or maybe move this stuff to web::WebIntTest? Done. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:121: web::NavigationItem* current_item() { On 2016/12/29 17:06:50, Eugene But wrote: > s/current_item/GetLastCommittedItem > > When I see current_item I always have to look up the implementation, because > implementations of this method are all different :) Done. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:143: // Set the parameters to use for state operations on the test page. On 2016/12/29 17:06:50, Eugene But wrote: > Could you please be more specific with comments. Does it actually perform > replace state or something? Done. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:157: base::test::ios::WaitUntilCondition(^bool { On 2016/12/29 17:06:50, Eugene But wrote: > Per comments in my previous CLs, could you please use API that synchronously > execute JS. Here and everywhere in tests. Done. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:222: TEST_F(HistoryStateOperationsTest, NoOps) { On 2016/12/29 17:06:50, Eugene But wrote: > Looks like this test have multiple test cases inside. Do you want to split it? Done. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:223: std::string empty_state(""); On 2016/12/29 17:06:50, Eugene But wrote: > std::string empty_state; > > Same comment for the next line. Done. https://codereview.chromium.org/2605223002/diff/1/ios/web/navigation/history_... ios/web/navigation/history_state_operations_inttest.mm:225: GURL unresolvable_url("http://www.google.invalid"); On 2016/12/29 17:06:50, Eugene But wrote: > Does this URL hit network? Do you need to use localhost URLs instead? No, the special "invalid" TLD prevents JS from resolving the URL, so this results in a JAvaScript exception before any network activity has a chance to occur.
lgtm https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... File ios/web/navigation/history_state_operations_inttest.mm (right): https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:5: #import "base/mac/bind_objc_block.h" Do you need this and many other includes in this list? https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:35: const char kHistoryStateOperationsTestURL[] = Optional nit: Camel case Url would be more appropriate for C++ tests. https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:39: const char kPushStateID[] = "push-state"; Same thing for Id https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:69: // Set the parameters to use for state operations on the test page. This s/Set/Sets https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:102: return operation_was_no_op; Can this ever return false? Should the result be void? https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:122: // Reload the page and perform the same check with Should this be a separate test? Smaller tests are usually more robust, which is important for tests that are part of CQ. https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:145: LoadUrl(state_operations_url()); Should this be a separate test? Or at least have "// Reload the page and perform the same check with" comment? https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:157: GURL different_origin_url("http://localhost:1000"); Do you want to use |HttpServer::GetPort() + 1|? https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:166: LoadUrl(state_operations_url()); Should this be a separate test? Or at least have "// Reload the page and perform the same check with" comment?
https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... File ios/web/navigation/history_state_operations_inttest.mm (right): https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:5: #import "base/mac/bind_objc_block.h" On 2017/01/21 02:15:23, Eugene But wrote: > Do you need this and many other includes in this list? Nope, removed. https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:35: const char kHistoryStateOperationsTestURL[] = On 2017/01/21 02:15:23, Eugene But wrote: > Optional nit: Camel case Url would be more appropriate for C++ tests. Done. https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:39: const char kPushStateID[] = "push-state"; On 2017/01/21 02:15:24, Eugene But wrote: > Same thing for Id Done. https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:69: // Set the parameters to use for state operations on the test page. This On 2017/01/21 02:15:23, Eugene But wrote: > s/Set/Sets Done. https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:122: // Reload the page and perform the same check with On 2017/01/21 02:15:23, Eugene But wrote: > Should this be a separate test? Smaller tests are usually more robust, which is > important for tests that are part of CQ. Done. https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:145: LoadUrl(state_operations_url()); On 2017/01/21 02:15:23, Eugene But wrote: > Should this be a separate test? Or at least have "// Reload the page and perform > the same check with" comment? Done. https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:157: GURL different_origin_url("http://localhost:1000"); On 2017/01/21 02:15:23, Eugene But wrote: > Do you want to use |HttpServer::GetPort() + 1|? Done. https://codereview.chromium.org/2605223002/diff/20001/ios/web/navigation/hist... ios/web/navigation/history_state_operations_inttest.mm:166: LoadUrl(state_operations_url()); On 2017/01/21 02:15:23, Eugene But wrote: > Should this be a separate test? Or at least have "// Reload the page and perform > the same check with" comment? Done.
The CQ bit was checked by kkhorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2605223002/#ps40001 (title: "eugene's comments")
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": 1485230899500030,
"parent_rev": "39ce616fc15f3ea70afbfd3813b50e2207a62314", "commit_rev":
"2a90b3aac07b05d5639f5de565c33a66ecc84ff3"}
Message was sent while issue was closed.
Description was changed from ========== Created test for no-op history state operations. BUG=673022 ========== to ========== Created test for no-op history state operations. BUG=673022 Review-Url: https://codereview.chromium.org/2605223002 Cr-Commit-Position: refs/heads/master@{#445656} Committed: https://chromium.googlesource.com/chromium/src/+/2a90b3aac07b05d5639f5de565c3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2a90b3aac07b05d5639f5de565c3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
