Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2295543002:
More Blimp browser tests for navigation (Closed)
|
Created:
4 years, 3 months ago by Brian Goldman Modified:
4 years, 3 months ago Reviewers:
Kevin M CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMore Blimp browser tests for navigation
The Blimp browser tests now cover every action of NavigationFeature.
GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack.
Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases.
Tested log output for the invalid GoBack and GoForward cases with the following:
blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoBack | grep Ignoring
blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoForward | grep Ignoring
BUG=
Committed: https://crrev.com/be8b5afc756176296478bc57170bddb9ef5f77db
Cr-Commit-Position: refs/heads/master@{#415368}
Patch Set 1 #
Total comments: 8
Patch Set 2 : review changes #
Total comments: 2
Patch Set 3 : unused #
Total comments: 2
Patch Set 4 : log #Messages
Total messages: 24 (12 generated)
Description was changed from ========== Blimp browser tests for GoForward and Reload BUG= ========== to ========== Blimp browser tests for GoForward and Reload The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. BUG= ==========
bgoldman@chromium.org changed reviewers: + kmarshall@chromium.org
https://codereview.chromium.org/2295543002/diff/1/blimp/engine/browser_tests/... File blimp/engine/browser_tests/engine_browsertest.cc (right): https://codereview.chromium.org/2295543002/diff/1/blimp/engine/browser_tests/... blimp/engine/browser_tests/engine_browsertest.cc:122: EXPECT_EQ("page1", last_page_title_); Turn these into named constants that are declared at the top, e.g. kPage1, kPageTitle1 https://codereview.chromium.org/2295543002/diff/1/blimp/engine/browser_tests/... blimp/engine/browser_tests/engine_browsertest.cc:151: Would it make sense to also test going back with an empty stack, or going forward w/o first going back?
https://codereview.chromium.org/2295543002/diff/1/blimp/engine/browser_tests/... File blimp/engine/browser_tests/engine_browsertest.cc (right): https://codereview.chromium.org/2295543002/diff/1/blimp/engine/browser_tests/... blimp/engine/browser_tests/engine_browsertest.cc:122: EXPECT_EQ("page1", last_page_title_); On 2016/08/29 23:31:52, Kevin M wrote: > Turn these into named constants that are declared at the top, e.g. kPage1, > kPageTitle1 This might just be a standard Chromium test style thing, but I have to ask: why? The constant names wouldn't be any shorter or more clear. https://codereview.chromium.org/2295543002/diff/1/blimp/engine/browser_tests/... blimp/engine/browser_tests/engine_browsertest.cc:151: On 2016/08/29 23:31:52, Kevin M wrote: > Would it make sense to also test going back with an empty stack, or going > forward w/o first going back? I'm almost certain that right now in Blimp if the engine gets a go-back request with an empty stack, it hits a DCHECK and dies. Forward might be the same; I haven't looked. I could do a separate CL to add a non-crash behavior (no-op?) and a browser test.
https://codereview.chromium.org/2295543002/diff/1/blimp/engine/browser_tests/... File blimp/engine/browser_tests/engine_browsertest.cc (right): https://codereview.chromium.org/2295543002/diff/1/blimp/engine/browser_tests/... blimp/engine/browser_tests/engine_browsertest.cc:122: EXPECT_EQ("page1", last_page_title_); On 2016/08/29 23:38:16, Brian Goldman wrote: > On 2016/08/29 23:31:52, Kevin M wrote: > > Turn these into named constants that are declared at the top, e.g. kPage1, > > kPageTitle1 > > This might just be a standard Chromium test style thing, but I have to ask: why? > The constant names wouldn't be any shorter or more clear. It is a general styleguide rule in Chromium/Google. Typos in named values are statically caught by the compiler, whereas accidentally mangled literals lead to headaches at runtime. https://codereview.chromium.org/2295543002/diff/1/blimp/engine/browser_tests/... blimp/engine/browser_tests/engine_browsertest.cc:151: On 2016/08/29 23:38:16, Brian Goldman wrote: > On 2016/08/29 23:31:52, Kevin M wrote: > > Would it make sense to also test going back with an empty stack, or going > > forward w/o first going back? > > I'm almost certain that right now in Blimp if the engine gets a go-back request > with an empty stack, it hits a DCHECK and dies. Forward might be the same; I > haven't looked. I could do a separate CL to add a non-crash behavior (no-op?) > and a browser test. OK, could you investigate it for potentially adding to this CL? My preference would be to not have super fine-grained CLs.
Description was changed from ========== Blimp browser tests for GoForward and Reload The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. BUG= ========== to ========== Blimp browser tests for GoForward and Reload The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also includes no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. BUG= ==========
Description was changed from ========== Blimp browser tests for GoForward and Reload The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also includes no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. BUG= ========== to ========== Blimp browser tests for GoForward and Reload The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. BUG= ==========
https://codereview.chromium.org/2295543002/diff/1/blimp/engine/browser_tests/... File blimp/engine/browser_tests/engine_browsertest.cc (right): https://codereview.chromium.org/2295543002/diff/1/blimp/engine/browser_tests/... blimp/engine/browser_tests/engine_browsertest.cc:122: EXPECT_EQ("page1", last_page_title_); On 2016/08/29 23:41:30, Kevin M wrote: > On 2016/08/29 23:38:16, Brian Goldman wrote: > > On 2016/08/29 23:31:52, Kevin M wrote: > > > Turn these into named constants that are declared at the top, e.g. kPage1, > > > kPageTitle1 > > > > This might just be a standard Chromium test style thing, but I have to ask: > why? > > The constant names wouldn't be any shorter or more clear. > > It is a general styleguide rule in Chromium/Google. Typos in named values are > statically caught by the compiler, whereas accidentally mangled literals lead to > headaches at runtime. Done. https://codereview.chromium.org/2295543002/diff/1/blimp/engine/browser_tests/... blimp/engine/browser_tests/engine_browsertest.cc:151: On 2016/08/29 23:41:30, Kevin M wrote: > On 2016/08/29 23:38:16, Brian Goldman wrote: > > On 2016/08/29 23:31:52, Kevin M wrote: > > > Would it make sense to also test going back with an empty stack, or going > > > forward w/o first going back? > > > > I'm almost certain that right now in Blimp if the engine gets a go-back > request > > with an empty stack, it hits a DCHECK and dies. Forward might be the same; I > > haven't looked. I could do a separate CL to add a non-crash behavior (no-op?) > > and a browser test. > > OK, could you investigate it for potentially adding to this CL? My preference > would be to not have super fine-grained CLs. Done.
https://codereview.chromium.org/2295543002/diff/20001/blimp/engine/browser_te... File blimp/engine/browser_tests/engine_browsertest.cc (right): https://codereview.chromium.org/2295543002/diff/20001/blimp/engine/browser_te... blimp/engine/browser_tests/engine_browsertest.cc:99: void ExpectNoPageLoad() { Never called?
https://codereview.chromium.org/2295543002/diff/20001/blimp/engine/browser_te... File blimp/engine/browser_tests/engine_browsertest.cc (right): https://codereview.chromium.org/2295543002/diff/20001/blimp/engine/browser_te... blimp/engine/browser_tests/engine_browsertest.cc:99: void ExpectNoPageLoad() { On 2016/08/30 00:52:39, Kevin M wrote: > Never called? Oh whoops, this was a helper than turned out to be unhelpful. Deleted.
Description was changed from ========== Blimp browser tests for GoForward and Reload The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. BUG= ========== to ========== More Blimp browser tests for navigation The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. BUG= ==========
lgtm https://codereview.chromium.org/2295543002/diff/40001/blimp/engine/session/ta... File blimp/engine/session/tab.cc (right): https://codereview.chromium.org/2295543002/diff/40001/blimp/engine/session/ta... blimp/engine/session/tab.cc:78: if (!web_contents_->GetController().CanGoBack()) { Add DLOG(ERROR) in here so that we call attention to this in debug builds, also in GoForward.
https://codereview.chromium.org/2295543002/diff/40001/blimp/engine/session/ta... File blimp/engine/session/tab.cc (right): https://codereview.chromium.org/2295543002/diff/40001/blimp/engine/session/ta... blimp/engine/session/tab.cc:78: if (!web_contents_->GetController().CanGoBack()) { On 2016/08/30 17:28:59, Kevin M wrote: > Add DLOG(ERROR) in here so that we call attention to this in debug builds, also > in GoForward. Done.
Description was changed from ========== More Blimp browser tests for navigation The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. BUG= ========== to ========== More Blimp browser tests for navigation The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. Tested log output for the invalid GoBack and GoForward cases with the following: out-linux/Debug/blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoForward | grep Ignoring out-linux/Debug/blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoForward | grep Ignoring BUG= ==========
Description was changed from ========== More Blimp browser tests for navigation The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. Tested log output for the invalid GoBack and GoForward cases with the following: out-linux/Debug/blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoForward | grep Ignoring out-linux/Debug/blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoForward | grep Ignoring BUG= ========== to ========== More Blimp browser tests for navigation The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. Tested log output for the invalid GoBack and GoForward cases with the following: out-linux/Debug/blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoBack | grep Ignoring out-linux/Debug/blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoForward | grep Ignoring BUG= ==========
The CQ bit was checked by bgoldman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/2295543002/#ps60001 (title: "log")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== More Blimp browser tests for navigation The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. Tested log output for the invalid GoBack and GoForward cases with the following: out-linux/Debug/blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoBack | grep Ignoring out-linux/Debug/blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoForward | grep Ignoring BUG= ========== to ========== More Blimp browser tests for navigation The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. Tested log output for the invalid GoBack and GoForward cases with the following: blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoBack | grep Ignoring blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoForward | grep Ignoring BUG= ==========
Message was sent while issue was closed.
Description was changed from ========== More Blimp browser tests for navigation The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. Tested log output for the invalid GoBack and GoForward cases with the following: blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoBack | grep Ignoring blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoForward | grep Ignoring BUG= ========== to ========== More Blimp browser tests for navigation The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. Tested log output for the invalid GoBack and GoForward cases with the following: blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoBack | grep Ignoring blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoForward | grep Ignoring BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== More Blimp browser tests for navigation The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. Tested log output for the invalid GoBack and GoForward cases with the following: blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoBack | grep Ignoring blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoForward | grep Ignoring BUG= ========== to ========== More Blimp browser tests for navigation The Blimp browser tests now cover every action of NavigationFeature. GoForward is an additional step in the existing GoBack test, rather than a new test, because you can't test GoForward without GoBack. Also adds no-op behavior on the engine for when the client requests GoBack or GoForward but those operations aren't valid, and tests for those cases. Tested log output for the invalid GoBack and GoForward cases with the following: blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoBack | grep Ignoring blimp_browsertests --single_process --gtest_filter=EngineBrowserTest.InvalidGoForward | grep Ignoring BUG= Committed: https://crrev.com/be8b5afc756176296478bc57170bddb9ef5f77db Cr-Commit-Position: refs/heads/master@{#415368} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/be8b5afc756176296478bc57170bddb9ef5f77db Cr-Commit-Position: refs/heads/master@{#415368} |
