Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1426683005:
[Contextual Search] Improve testing & add regressions tests (Closed)
|
Created:
5 years, 1 month ago by pedro (no code reviews) Modified:
5 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Contextual Search] Improve testing & add regressions tests
This CL adds tests for regressions that have been fixed
recently, some of them involving ContentViewCore.
In order to be able to properly instrument the ContentViewCore,
changes in the testing framework have been made. There are now
a couple of wrapper classes: OverlayPanelContentWrapper and
ContentViewCoreWrapper, which allow us to test for example
if the onShow() method was ever called, or if URLs have been
properly removed from local history when not seen.
This CL also introduces a few more helpers to simulate searches
caused by gestures (long press and tap) in a way that it
encapsulates the simulation logic and prevent flakiness. We
were manually doing a search term resolution by calling a
fakeResponse() method. Sometimes we are calling this when
not necessary (on long press) and tests don't call it in the
same order. We are also assuming it completed when the Bar
peeks (which works but under the wrong assumption). There are
now simulateLongPressSearch(id) and simulateTapSearch(id)
methods that simulate the search and handle the resolution
automatically when the system requests one.
There are several helper methods in ContextualSearchManagerTest
that are similar but do slightly different things, and some
assumptions are not quite correct which can and may be causing
falkiness in some tests. A couple of examples are: tests rely
on the change of state of the Panel in order to determine that
animations/actions are done. The closed state was being changed
before destroying the Contents. This is now fixed.
Another example is the tapBasePageToClosePanel() helper method
that wasn't properly working when the Panel was peeking while
a long press selection is present. I believe this is the cause
of the flakiness of 3 of our tests.
The new search simulation method should help making our tests
more robust and a followup CL is required to change existing
tests to use the new methods, once it has been confirmed that
the new methods work properly on existing test bots.
BUG=543319
Committed: https://crrev.com/7e0c3b88ed5970327b2aa7962a1bc336b71bc427
Cr-Commit-Position: refs/heads/master@{#357143}
Patch Set 1 #
Total comments: 52
Patch Set 2 : Addressed Donn's comments #
Total comments: 2
Patch Set 3 : Addressed Donn's comments & fixed test #
Total comments: 3
Patch Set 4 : Sync & rebase #
Messages
Total messages: 26 (7 generated)
Description was changed from ========== [Contextual Search] Improve testing & add regressions tests This CL adds tests for regressions that have been fixed recently, some of them involving ContentViewCore. In order to be able to properly instrument the ContentViewCore, changes in the testing framework have been made. There are now a couple of wrapper classes: OverlayPanelContentWrapper and ContentViewCoreWrapper, which allow us to test for example if the onShow() method was ever called, or if URLs have been properly removed from local history when not seen. This CL also introduces a few more helpers to simulate searches caused by gestures (long press and tap) in a way that it encapsulates the simulation logic and prevent flakiness. We were manually doing a search term resolution by calling a fakeResponse() method. Sometimes we are calling this when not necessary (on long press) and tests don't call it in the same order. We are also assuming it completed when the Bar peeks (which works but under the wrong assumption). There are now simulateLongPressSearch(id) and simulateTapSearch(id) methods that simulate the search and handle the resolution automatically when the system requests one. There are several helper methods in ContextualSearchManagerTest that are similar but do slightly different things, and some assumptions are not quite correct which can and may be causing falkiness in some tests. A couple of examples are: tests rely on the change of state of the Panel in order to determine that animations/actions are done. The closed state was being changed before destroying the Contents. This is now fixed. Another example is the tapBasePageToClosePanel() helper method that wasn't properly working when the Panel was peeking while a long press selection is present. I believe this is the cause of the flakiness of 3 of our tests. The new search simulation method should help making our tests more robust and a followup CL is required to change existing tests to use the new methods, once it has been confirmed that the new methods work properly on existing test bots. BUG=543319 ========== to ========== [Contextual Search] Improve testing & add regressions tests This CL adds tests for regressions that have been fixed recently, some of them involving ContentViewCore. In order to be able to properly instrument the ContentViewCore, changes in the testing framework have been made. There are now a couple of wrapper classes: OverlayPanelContentWrapper and ContentViewCoreWrapper, which allow us to test for example if the onShow() method was ever called, or if URLs have been properly removed from local history when not seen. This CL also introduces a few more helpers to simulate searches caused by gestures (long press and tap) in a way that it encapsulates the simulation logic and prevent flakiness. We were manually doing a search term resolution by calling a fakeResponse() method. Sometimes we are calling this when not necessary (on long press) and tests don't call it in the same order. We are also assuming it completed when the Bar peeks (which works but under the wrong assumption). There are now simulateLongPressSearch(id) and simulateTapSearch(id) methods that simulate the search and handle the resolution automatically when the system requests one. There are several helper methods in ContextualSearchManagerTest that are similar but do slightly different things, and some assumptions are not quite correct which can and may be causing falkiness in some tests. A couple of examples are: tests rely on the change of state of the Panel in order to determine that animations/actions are done. The closed state was being changed before destroying the Contents. This is now fixed. Another example is the tapBasePageToClosePanel() helper method that wasn't properly working when the Panel was peeking while a long press selection is present. I believe this is the cause of the flakiness of 3 of our tests. The new search simulation method should help making our tests more robust and a followup CL is required to change existing tests to use the new methods, once it has been confirmed that the new methods work properly on existing test bots. BUG=543319 ==========
pedrosimonetti@chromium.org changed reviewers: + aurimas@chromium.org, donnd@chromium.org, dtrainor@google.com, thestig@chromium.org
donnd@ and aurimas@: please take a look at this entire CL. dtrainor@: please take a look at changes in OverlayPanelContent.java. thestig@chromium.org: please take a look at changes in tap_test.html
pedrosimonetti@chromium.org changed reviewers: + dtrainor@chromium.org - dtrainor@google.com
Oops, added the right dtrainor@ now.
Oops, added the right dtrainor@ now.
You don't need me. chrome/test/data/OWNERS contains '*'
Pedro, thanks so much for starting this cleanup of our instrumentation tests! This was clearly a significant effort and looks like it will really make our tests much more readable and useful. This cleanup seems like a big step towards shifting to testing aspects, which is a huge improvement! As we've discussed in the past, we should strive for tests that only validate one aspect of our system. Instead, when we first built our system we wrote tests for each UX sequence, and asserted that all associated aspects worked for that sequence. As you know, that's been problematic as we've added multiple aspects to each action. E.g. one bug may cause many tests to fail. I think we should also have a goal to make our individual tests as readable as possible. To that end, I think each test should be a sequence of 1) Description of what's being tested, 2) An Action, 3) A minimal set of assertions. For #1 we should focus on saying what's expected and why, without mentioning the action (since that should be evident from the code). For #2 we should ideally have a single helper method that does that action and asserts all invariants for that action, e.g. swipePanelUp. For #3, since each test only tests one aspect, we should only assert the aspect under test. I think the new tests you've added are great, but could be even better if we try to stick to this template. I've added specific suggestions along those line here. Thanks again for doing this, it's super-important to have tests we can trust, and you've really enhanced our test infrastructure here. https://codereview.chromium.org/1426683005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java (right): https://codereview.chromium.org/1426683005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:821: // We should only set the state at the end of this method, in oder to make sure the nit: typo at end, change "the" to "that". https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java (right): https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:171: mDidFinishResolution = false; I don't think you need this, so I'd remove it. If you want to explicitly init all members then add mDidStartResolution too. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:179: // requests one, and ist does not finish until the simulated resolution happens. nit: typo. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:240: if (!mDidFinishResolution) { Do you think it would be helpful to issue a warning if this condition failed? https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:241: mActiveFakeTapSearch = null; Wouldn't it be better to move this below the handleSearchTermResolutionResponse call, since we still have an active search until after the response has been handled? https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:221: * @throws InterruptedException Nit: I think you can put both of these on one line. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:382: * Fakes navigation of the Content View with the given httpResult code. Nit: Change "with the given httpResult code" to something like "with the given failure boolean". https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:402: public void setIsFocusedNodeEditableForTest(boolean isFocusedNodeEditable) { Nit: add simple javadoc. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2211: * @SmallTest Why are these annotations in the comment? Looks like this test is not disabled, so I'm confused by this. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2221: assertEquals(1, mFakeServer.getLoadedUrlCount()); I don't think we should be checking the loadedUrlCount here, since you have a separate test for that below. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2225: assertContentViewCoreVisible(); Isn't it an invariant that whenever we expand, the contentViewCore is made visible? I would think we'd want to just include this assert in tapPeekingBarToExpandAndAssert. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2230: assertNoContentViewCore(); Similarly once the panel is closed there should be no CVC, so this assert should be rolled in to the tapBasePageToClosePanel call? https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2237: * @SmallTest Here too. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2247: // Expand Panel and check that the Content is now created and visible. Here's an example of the annoying nit about test-step descriptions focusing on what we're checking rather than what's being done (which should be evident from the code). Maybe change to "The CVC should be visible once the panel has been expanded". https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2256: assertEquals(1, mFakeServer.getLoadedUrlCount()); I don't think we need this line either. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2260: * Tests swiping the overlay open, after an initial tap that activates the peeking card. Nit: update to say what were testing, rather than how we're testing it. The title does that pretty well! :-) https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2275: swipePanelUp(); Here's an example of where we could make these tests more readable. We should have a single method swipePanelUpAndAssert, or maybe even update swipePanelUp to always assert that it did expand. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2281: swipePanelDown(); Here too, it would be nicer to have just one action line and one or more asserts rather than the boilerplate swipePanelDown followed by waitForPanelToPeekAndAssert. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2347: assertContentViewCoreCreated(); Maybe these three can be combined? I would think that assertNeverCalledContentViewCoreOnShow would imply that it's not visible, and probably that it was created. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2387: public void testChainedSearchLoadsContentCorrectly() nit: I'd rename this to something like testChainedSearchLoadsCorrectSearchTerm, since we can't really check everything associated with loading content correctly. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2411: // Now simulate a long press, without closing the Panel first. Nit: change "without closing the Panel first" to "leaving the Panel peeking"? https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2521: assertEquals(1, mFakeServer.getLoadedUrlCount()); Wouldn't it be better to skip this check, and instead check that the url was not null when we get url1? https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2528: assertEquals(2, mFakeServer.getLoadedUrlCount()); Similarly, I'd skip this and check that url2 is not the same as url1 here. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2535: assertEquals(3, mFakeServer.getLoadedUrlCount()); I'd move this down to the bottom with the other final asserts. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2536: String url3 = mFakeServer.getLoadedUrl(); Similarly check that url3 is unique, otherwise the hasRemovedUrl calls might not tell us what we need to know. https://codereview.chromium.org/1426683005/diff/1/chrome/test/data/android/co... File chrome/test/data/android/contextualsearch/tap_test.html (right): https://codereview.chromium.org/1426683005/diff/1/chrome/test/data/android/co... chrome/test/data/android/contextualsearch/tap_test.html:25: <div><span id="search">Search</span> <span id="term">Term</span> <span id="resolution">Resolution</span></div> Are these spans supposed to be close enough that taps from one to the next are within our "near" threshold? If we depend on this, please add a comment here.
PTAL https://codereview.chromium.org/1426683005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java (right): https://codereview.chromium.org/1426683005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:821: // We should only set the state at the end of this method, in oder to make sure the On 2015/10/27 19:18:51, Donn Denman wrote: > nit: typo at end, change "the" to "that". Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java (right): https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:171: mDidFinishResolution = false; On 2015/10/27 19:18:51, Donn Denman wrote: > I don't think you need this, so I'd remove it. If you want to explicitly init > all members then add mDidStartResolution too. Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:179: // requests one, and ist does not finish until the simulated resolution happens. On 2015/10/27 19:18:51, Donn Denman wrote: > nit: typo. Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:240: if (!mDidFinishResolution) { On 2015/10/27 19:18:51, Donn Denman wrote: > Do you think it would be helpful to issue a warning if this condition failed? I don't think this is necessary. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:241: mActiveFakeTapSearch = null; On 2015/10/27 19:18:51, Donn Denman wrote: > Wouldn't it be better to move this below the handleSearchTermResolutionResponse > call, since we still have an active search until after the response has been > handled? Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:221: * @throws InterruptedException On 2015/10/27 19:18:52, Donn Denman wrote: > Nit: I think you can put both of these on one line. By looking at other files using multiple @throws tag, it seems multiple tags are being used, so I'll keep this way. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:382: * Fakes navigation of the Content View with the given httpResult code. On 2015/10/27 19:18:52, Donn Denman wrote: > Nit: Change "with the given httpResult code" to something like "with the given > failure boolean". Changed the comment. No mention to the param anymore, since it's explained right below. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:402: public void setIsFocusedNodeEditableForTest(boolean isFocusedNodeEditable) { On 2015/10/27 19:18:52, Donn Denman wrote: > Nit: add simple javadoc. Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2211: * @SmallTest On 2015/10/27 19:18:51, Donn Denman wrote: > Why are these annotations in the comment? Looks like this test is not disabled, > so I'm confused by this. Thanks for noticing this. Moved to the proper place. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2221: assertEquals(1, mFakeServer.getLoadedUrlCount()); On 2015/10/27 19:18:52, Donn Denman wrote: > I don't think we should be checking the loadedUrlCount here, since you have a > separate test for that below. Good point. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2225: assertContentViewCoreVisible(); On 2015/10/27 19:18:52, Donn Denman wrote: > Isn't it an invariant that whenever we expand, the contentViewCore is made > visible? I would think we'd want to just include this assert in > tapPeekingBarToExpandAndAssert. I think that is an implementation detail of the Content and therefore we should test them separately. Otherwise a bug in the Content visibility could affect all tests that expand the Panel. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2230: assertNoContentViewCore(); On 2015/10/27 19:18:52, Donn Denman wrote: > Similarly once the panel is closed there should be no CVC, so this assert should > be rolled in to the tapBasePageToClosePanel call? Same as above. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2237: * @SmallTest On 2015/10/27 19:18:52, Donn Denman wrote: > Here too. Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2247: // Expand Panel and check that the Content is now created and visible. On 2015/10/27 19:18:52, Donn Denman wrote: > Here's an example of the annoying nit about test-step descriptions focusing on > what we're checking rather than what's being done (which should be evident from > the code). Maybe change to "The CVC should be visible once the panel has been > expanded". I changed comments to reflect that. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2256: assertEquals(1, mFakeServer.getLoadedUrlCount()); On 2015/10/27 19:18:51, Donn Denman wrote: > I don't think we need this line either. Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2260: * Tests swiping the overlay open, after an initial tap that activates the peeking card. On 2015/10/27 19:18:52, Donn Denman wrote: > Nit: update to say what were testing, rather than how we're testing it. The > title does that pretty well! :-) Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2275: swipePanelUp(); On 2015/10/27 19:18:51, Donn Denman wrote: > Here's an example of where we could make these tests more readable. We should > have a single method swipePanelUpAndAssert, or maybe even update swipePanelUp to > always assert that it did expand. I agree but I think this belongs to a separate CL, since this one is already too big and these changes would require changing existing tests, which I'm trying to avoid to do now. Also, I think we should have a small number of helper methods and use them consistently. I think we have too many methods, and variations of similar methods. I already have a TODO to address that. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2281: swipePanelDown(); On 2015/10/27 19:18:52, Donn Denman wrote: > Here too, it would be nicer to have just one action line and one or more asserts > rather than the boilerplate swipePanelDown followed by > waitForPanelToPeekAndAssert. Same as above. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2347: assertContentViewCoreCreated(); On 2015/10/27 19:18:52, Donn Denman wrote: > Maybe these three can be combined? I would think that > assertNeverCalledContentViewCoreOnShow would imply that it's not visible, and > probably that it was created. Good point. There's now a assertContentViewCoreCreatedButNeverMadeVisible() which makes three asserts. I also noticed that the assertNeverCalledContentViewCoreOnShow() wasn't actually testing what it was supposed to test. In theory a content could be created, made visible, and destroyed, then another content is created, and we would not know that the first content was made visible. So I fixed that as well, and now the FakeServer keeps track if onShow() was ever called in any ContentViewCore. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2387: public void testChainedSearchLoadsContentCorrectly() On 2015/10/27 19:18:51, Donn Denman wrote: > nit: I'd rename this to something like testChainedSearchLoadsCorrectSearchTerm, > since we can't really check everything associated with loading content > correctly. Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2411: // Now simulate a long press, without closing the Panel first. On 2015/10/27 19:18:52, Donn Denman wrote: > Nit: change "without closing the Panel first" to "leaving the Panel peeking"? Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2521: assertEquals(1, mFakeServer.getLoadedUrlCount()); On 2015/10/27 19:18:52, Donn Denman wrote: > Wouldn't it be better to skip this check, and instead check that the url was not > null when we get url1? Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2528: assertEquals(2, mFakeServer.getLoadedUrlCount()); On 2015/10/27 19:18:52, Donn Denman wrote: > Similarly, I'd skip this and check that url2 is not the same as url1 here. Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2535: assertEquals(3, mFakeServer.getLoadedUrlCount()); On 2015/10/27 19:18:51, Donn Denman wrote: > I'd move this down to the bottom with the other final asserts. Done. https://codereview.chromium.org/1426683005/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2536: String url3 = mFakeServer.getLoadedUrl(); On 2015/10/27 19:18:52, Donn Denman wrote: > Similarly check that url3 is unique, otherwise the hasRemovedUrl calls might not > tell us what we need to know. Done. https://codereview.chromium.org/1426683005/diff/1/chrome/test/data/android/co... File chrome/test/data/android/contextualsearch/tap_test.html (right): https://codereview.chromium.org/1426683005/diff/1/chrome/test/data/android/co... chrome/test/data/android/contextualsearch/tap_test.html:25: <div><span id="search">Search</span> <span id="term">Term</span> <span id="resolution">Resolution</span></div> On 2015/10/27 19:18:52, Donn Denman wrote: > Are these spans supposed to be close enough that taps from one to the next are > within our "near" threshold? If we depend on this, please add a comment here. Done.
LGTM, thanks again for the cleanup! P.S. I wondered about the ever shown, but didn't quite catch that. Glad you did. https://codereview.chromium.org/1426683005/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1426683005/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2278: * Tests swiping the overlay open, after an initial long-press that activates the peeking card, Update this one too. E.g. Tests swiping the panel up and down with long-press triggering will only load the Content once.
aurimas@, please take a look. dtrainor@, please review changes in OverlayPanelContent.java https://codereview.chromium.org/1426683005/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1426683005/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2278: * Tests swiping the overlay open, after an initial long-press that activates the peeking card, On 2015/10/29 22:36:13, Donn Denman wrote: > Update this one too. E.g. > Tests swiping the panel up and down with long-press triggering will only load > the Content once. Done.
lgtm https://codereview.chromium.org/1426683005/diff/30006/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1426683005/diff/30006/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:340: private void assertContentViewCoreCreated() { I don't really like these one line methods. Can we just use assertNotNull(getPanelContentViewCore()) inline? Same with methods below.
aurimas@, see my comment. https://codereview.chromium.org/1426683005/diff/30006/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1426683005/diff/30006/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:340: private void assertContentViewCoreCreated() { On 2015/10/29 23:16:06, aurimas wrote: > I don't really like these one line methods. Can we just use > assertNotNull(getPanelContentViewCore()) inline? Same with methods below. Could you elaborate on why you don't like them? I think they provide a good readability value, making tests easier to read and write (specially on code editors that support autocompletion). This also makes things more encapsulated and easier to maintain. We plan on not exposing the ContentViewCore anymore. If that happens, we would have to change all tests that were checking things related to the ContentViewCore.
On 2015/10/26 23:49:58, pedrosimonetti wrote: > mailto:thestig@chromium.org: please take a look at changes in tap_test.html You don't need me. See chrome/test/data/OWNERS, but consider sticking to 80 chars / column in tap_test.html.
pedrosimonetti@chromium.org changed reviewers: - thestig@chromium.org
On 2015/10/30 05:35:54, Lei Zhang wrote: > On 2015/10/26 23:49:58, pedrosimonetti wrote: > > mailto:thestig@chromium.org: please take a look at changes in tap_test.html > > You don't need me. See chrome/test/data/OWNERS, but consider sticking to 80 > chars / column in tap_test.html. thestig@: Thanks for the input. Removed you from the reviewers list.
https://codereview.chromium.org/1426683005/diff/30006/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1426683005/diff/30006/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:340: private void assertContentViewCoreCreated() { On 2015/10/30 at 01:23:28, pedrosimonetti wrote: > On 2015/10/29 23:16:06, aurimas wrote: > > I don't really like these one line methods. Can we just use > > assertNotNull(getPanelContentViewCore()) inline? Same with methods below. > > Could you elaborate on why you don't like them? > > I think they provide a good readability value, making tests easier to read and write (specially on code editors that support autocompletion). I would claim that assertContentViewCoreCreated() is just as readable as assertNotNull(getPanelContentViewCore()) or ssertContentViewCoreVisible() vs assertTrue(isContentViewCoreVisible()) or assertNoContentViewCore() vs assertNull(getPanelContentViewCore()). > We plan on not exposing the ContentViewCore anymore. We can change the checks when we get there. You can land this change as I already gave my LGTM, I just wanted to point out that these one line methods with really simple checks seem unnecessary.
dfalcantara@chromium.org changed reviewers: + dfalcantara@chromium.org
lgtm for //chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
dtrainor@: I handed this over to dfalcantara@ since I only needed stamp on one file. dfalcantara@: Thanks for the stamp! aurimas@: Okay. I just wanted to confirm how strongly you felt against it. I wouldn't mind changing the code, it would be a pretty simple search & replace. But I just thought there was a good value having those simple methods.
The CQ bit was checked by pedrosimonetti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org Link to the patchset: https://codereview.chromium.org/1426683005/#ps50001 (title: "Sync & rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1426683005/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1426683005/50001
Message was sent while issue was closed.
Committed patchset #4 (id:50001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7e0c3b88ed5970327b2aa7962a1bc336b71bc427 Cr-Commit-Position: refs/heads/master@{#357143}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:50001) has been created in https://codereview.chromium.org/1413383007/ by jam@chromium.org. The reason for reverting is: Adds a very flaky test. BUG=549805. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
