[Contextual Search] Reset all counters to fix flaky tests.

[Contextual Search] Reset all counters to fix a flaky test.

At least one test that relies on counters was flaky because counters were not being consistently reset for testing.
The counter resetting code was inexplicably calling code to update counters as if an open action had occurred (removed). This change may fix other flaky tests.
Update the test code to just reset all preferences instead of trying to remember to add reset code when adding a new counter.
Fixes testLongPressShowsPeekPromo.

BUG=540820, 569123
Committed: https://crrev.com/0eefcd63e6c76de96bf16b9297414f1756e7f649
Cr-Commit-Position: refs/heads/master@{#371353}

[Donn Denman, 2015-12-15 19:23:54]

Trivial (no longer dependent on other patches).

[Donn Denman, 2015-12-15 19:38:44]

Added required import

[Donn Denman, 2015-12-15 19:41:49]

donnd@chromium.org changed reviewers:
+ twellington@chromium.org

[Donn Denman, 2015-12-15 19:41:49]

Theresa, PTAL.

[Theresa, 2015-12-15 20:28:10]

https://chromiumcodereview.appspot.com/1517883003/diff/40001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://chromiumcodereview.appspot.com/1517883003/diff/40001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2185:
assertEquals(0, mPolicy.getPromoOpenCount());
Is it possible that this test is suffering the same one as the other test and
getPromoOpenCount isn't 0? Maybe we should call
ContextualSearchPolicy#resetCounters()? The one example failure was on the
waitForPanelToExpand() assert below, so maybe not.

Were you able to to run this w/ repeat=50 to see if it flaked locally ever?

[Donn Denman, 2016-01-06 23:41:59]

Description was changed from

==========
[Contextual Search] Reenable testLongPressShowsPeekPromo.

Reenabling this important test.  Not sure why it was
marked flaky -- and it may still be flaky.  Please
update the bug referenced below if this turns out
to be flaky.

BUG=540820
==========

to

==========
[Contextual Search] Reenable testLongPressShowsPeekPromo.

Reenabling this important test.  Not sure why it was
marked flaky -- and it may still be flaky.  Please
update the bug referenced below if this turns out
to be flaky.

BUG=540820
==========

[Donn Denman, 2016-01-06 23:41:59]

donnd@chromium.org changed reviewers:
+ mdjones@chromium.org

[Donn Denman, 2016-01-21 23:48:36]

Description was changed from

==========
[Contextual Search] Reenable testLongPressShowsPeekPromo.

Reenabling this important test.  Not sure why it was
marked flaky -- and it may still be flaky.  Please
update the bug referenced below if this turns out
to be flaky.

BUG=540820
==========

to

==========
[Contextual Search] Reset all counters to fix flaky tests.

Two tests that rely on counters were flaky because their counters were not being
reset for testing.
Now that we reset these counters, these tests should be fine.
Tests: testTapCount, testLongPressShowsPeekPromo.

BUG=540820,496737


BUG=540820
==========

[Donn Denman, 2016-01-21 23:51:48]

On 2015/12/15 20:28:10, Theresa Wellington wrote:
>
https://chromiumcodereview.appspot.com/1517883003/diff/40001/chrome/android/j...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
> (right):
> 
>
https://chromiumcodereview.appspot.com/1517883003/diff/40001/chrome/android/j...
>
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2185:
> assertEquals(0, mPolicy.getPromoOpenCount());
> Is it possible that this test is suffering the same one as the other test and
> getPromoOpenCount isn't 0? Maybe we should call
> ContextualSearchPolicy#resetCounters()? The one example failure was on the
> waitForPanelToExpand() assert below, so maybe not.
> 
> Were you able to to run this w/ repeat=50 to see if it flaked locally ever?

I think you were on the right track here: resetCounters was not resetting all
counters!

[Donn Denman, 2016-01-21 23:52:54]

donnd@chromium.org changed reviewers:
+ newt@chromium.org

[Donn Denman, 2016-01-21 23:52:54]

Theresa, PTAL.

Newt, PTAL at the comment added to ChromePreferenceManager (and let me know if
you can think of a way to enforce this).

[Donn Denman, 2016-01-21 23:57:55]

donnd@chromium.org changed reviewers:
- mdjones@chromium.org, newt@chromium.org, twellington@chromium.org

[Donn Denman, 2016-01-21 23:57:55]

Not ready yet: I want to test this some more.  Will ping you again when ready!

[Donn Denman, 2016-01-22 22:00:19]

Description was changed from

==========
[Contextual Search] Reset all counters to fix flaky tests.

Two tests that rely on counters were flaky because their counters were not being
reset for testing.
Now that we reset these counters, these tests should be fine.
Tests: testTapCount, testLongPressShowsPeekPromo.

BUG=540820,496737


BUG=540820
==========

to

==========
[Contextual Search] Reset all counters to fix a flaky test.

At least one test that relies on counters was flaky because counters were not
being consistently reset for testing.
The counter resetting code was inexplicably calling code to update counters as
if an open action had occurred (removed). This change may fix other flaky tests.
Update the counter resetting code to make it easier to remember to add reset
code when adding a new counter.
Fixes testLongPressShowsPeekPromo.

BUG=540820,569123
==========

[Donn Denman, 2016-01-22 22:01:15]

donnd@chromium.org changed reviewers:
+ newt@chromium.org, twellington@chromium.org

[Donn Denman, 2016-01-22 22:01:15]

Theresa, PTAL.

Newt, PTAL at the comment added to ChromePreferenceManager (and let me know if
you can think of a way to enforce this).

[newt (away), 2016-01-22 23:06:54]

https://codereview.chromium.org/1517883003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/1517883003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:114:
resetCounters();
Why do you need to call this? ChromeActivityTestCaseBase.setUp() calls
ApplicationTestUtils.setUp(), which clears the app data and hence resets any
shared prefs.

https://codereview.chromium.org/1517883003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:900:
CriteriaHelper.pollForCriteria(new Criteria() {
Suggestion: change ThreadUtils.runOnUiThread() to
ThreadUtils.runOnUiThreadBlocking(). Then you can delete the didResetCounters()
method, simplify the code, and make things faster in one fell swoop :)

[Donn Denman, 2016-01-23 01:07:43]

Newt, PTAL at your convenience.  And thanks for the suggestion and raising the
clear-data issue (still a mystery)!

https://codereview.chromium.org/1517883003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/1517883003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:114:
resetCounters();
On 2016/01/22 23:06:54, newt wrote:
> Why do you need to call this? ChromeActivityTestCaseBase.setUp() calls
> ApplicationTestUtils.setUp(), which clears the app data and hence resets any
> shared prefs.

Looks like those prefs are not getting reset for some reason.  Are you sure that
clearing app data is supposed to also clear preference data?  I still see
failure when I don't do this reset, and the ApplicationData.clearAppData is
being called.

https://codereview.chromium.org/1517883003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:900:
CriteriaHelper.pollForCriteria(new Criteria() {
On 2016/01/22 23:06:54, newt wrote:
> Suggestion: change ThreadUtils.runOnUiThread() to
> ThreadUtils.runOnUiThreadBlocking(). Then you can delete the
didResetCounters()
> method, simplify the code, and make things faster in one fell swoop :)

Done.  Thanks for the suggestion!

[newt (away), 2016-01-23 01:19:44]

Looks better :)

Instead of clearing individual prefs, how about clearing all shared prefs? That
would be more robust for testing.

PreferenceManager.getDefaultSharedPreferences(context).edit().clear().apply();

https://codereview.chromium.org/1517883003/diff/120001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/1517883003/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1371:
// TODO(donnd): consider using to runOnUiThreadBlocking, won't need to
waitForIdleSync?
Yes, you should almost always use runOnUiThreadBlocking() instead of
runOnUiThread().

waitForIdleSync() will wait for TabModelUtils.setIndex() to run, and will wait
for any subsequently posted tasks to run. If you need to wait on later posted
tasks, then you'll need to keep waitForIdleSync(). If not, you can remove it.

[Donn Denman, 2016-01-25 19:31:29]

Newt, PTAL, and thanks for the suggestions!  I'm going in send more CLs your way
for review!

Resetting all preferences seems to work fine.

https://chromiumcodereview.appspot.com/1517883003/diff/120001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://chromiumcodereview.appspot.com/1517883003/diff/120001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1371:
// TODO(donnd): consider using to runOnUiThreadBlocking, won't need to
waitForIdleSync?
On 2016/01/23 01:19:44, newt wrote:
> Yes, you should almost always use runOnUiThreadBlocking() instead of
> runOnUiThread().
> 
> waitForIdleSync() will wait for TabModelUtils.setIndex() to run, and will wait
> for any subsequently posted tasks to run. If you need to wait on later posted
> tasks, then you'll need to keep waitForIdleSync(). If not, you can remove it.

I think I'll clean this up in a separate CL.  Thanks for the explanation!

[Donn Denman, 2016-01-25 19:33:47]

Description was changed from

==========
[Contextual Search] Reset all counters to fix a flaky test.

At least one test that relies on counters was flaky because counters were not
being consistently reset for testing.
The counter resetting code was inexplicably calling code to update counters as
if an open action had occurred (removed). This change may fix other flaky tests.
Update the counter resetting code to make it easier to remember to add reset
code when adding a new counter.
Fixes testLongPressShowsPeekPromo.

BUG=540820,569123
==========

to

==========
[Contextual Search] Reset all counters to fix a flaky test.

At least one test that relies on counters was flaky because counters were not
being consistently reset for testing.
The counter resetting code was inexplicably calling code to update counters as
if an open action had occurred (removed). This change may fix other flaky tests.
Update the test code to just reset all preferences instead of trying to remember
to add reset code when adding a new counter.
Fixes testLongPressShowsPeekPromo.

BUG=540820,569123
==========

[Donn Denman, 2016-01-25 19:33:47]

donnd@chromium.org changed reviewers:
- newt@chromium.org

[Donn Denman, 2016-01-25 19:36:37]

donnd@chromium.org changed reviewers:
+ newt@chromium.org

[Donn Denman, 2016-01-25 19:36:37]

Newt, All we have left for you is a white-space cleaup.

[Theresa, 2016-01-25 19:41:07]

lgtm

https://codereview.chromium.org/1517883003/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/1517883003/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1366:
// TODO(donnd): consider using to runOnUiThreadBlocking, won't need to
waitForIdleSync?
nit: "consider changing to..."? or "consider using runOn..."

[Donn Denman, 2016-01-25 20:31:39]

Thanks!

https://codereview.chromium.org/1517883003/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/1517883003/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1366:
// TODO(donnd): consider using to runOnUiThreadBlocking, won't need to
waitForIdleSync?
On 2016/01/25 19:41:07, Theresa Wellington wrote:
> nit: "consider changing to..."? or "consider using runOn..."

Done.

[Donn Denman, 2016-01-25 20:31:55]

The CQ bit was checked by donnd@chromium.org

[Donn Denman, 2016-01-25 20:31:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org
Link to the patchset: https://codereview.chromium.org/1517883003/#ps180001
(title: "Fixed a nit comment.")

[commit-bot: I haz the power, 2016-01-25 20:33:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1517883003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1517883003/180001

[commit-bot: I haz the power, 2016-01-25 20:46:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-25 20:46:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Donn Denman, 2016-01-25 20:54:55]

Newt, still need your LGTM for a whitespace change to
ChromePreferenceManager.java

[newt (away), 2016-01-25 23:01:40]

lgtm

[Donn Denman, 2016-01-25 23:16:18]

The CQ bit was checked by donnd@chromium.org

[commit-bot: I haz the power, 2016-01-25 23:16:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1517883003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1517883003/180001

[commit-bot: I haz the power, 2016-01-25 23:58:51]

Description was changed from

==========
[Contextual Search] Reset all counters to fix a flaky test.

At least one test that relies on counters was flaky because counters were not
being consistently reset for testing.
The counter resetting code was inexplicably calling code to update counters as
if an open action had occurred (removed). This change may fix other flaky tests.
Update the test code to just reset all preferences instead of trying to remember
to add reset code when adding a new counter.
Fixes testLongPressShowsPeekPromo.

BUG=540820,569123
==========

to

==========
[Contextual Search] Reset all counters to fix a flaky test.

At least one test that relies on counters was flaky because counters were not
being consistently reset for testing.
The counter resetting code was inexplicably calling code to update counters as
if an open action had occurred (removed). This change may fix other flaky tests.
Update the test code to just reset all preferences instead of trying to remember
to add reset code when adding a new counter.
Fixes testLongPressShowsPeekPromo.

BUG=540820,569123
==========

[commit-bot: I haz the power, 2016-01-25 23:58:52]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-01-26 00:00:58]

Description was changed from

==========
[Contextual Search] Reset all counters to fix a flaky test.

At least one test that relies on counters was flaky because counters were not
being consistently reset for testing.
The counter resetting code was inexplicably calling code to update counters as
if an open action had occurred (removed). This change may fix other flaky tests.
Update the test code to just reset all preferences instead of trying to remember
to add reset code when adding a new counter.
Fixes testLongPressShowsPeekPromo.

BUG=540820,569123
==========

to

==========
[Contextual Search] Reset all counters to fix a flaky test.

At least one test that relies on counters was flaky because counters were not
being consistently reset for testing.
The counter resetting code was inexplicably calling code to update counters as
if an open action had occurred (removed). This change may fix other flaky tests.
Update the test code to just reset all preferences instead of trying to remember
to add reset code when adding a new counter.
Fixes testLongPressShowsPeekPromo.

BUG=540820,569123

Committed: https://crrev.com/0eefcd63e6c76de96bf16b9297414f1756e7f649
Cr-Commit-Position: refs/heads/master@{#371353}
==========

[commit-bot: I haz the power, 2016-01-26 00:00:59]

Patchset 10 (id:??) landed as
https://crrev.com/0eefcd63e6c76de96bf16b9297414f1756e7f649
Cr-Commit-Position: refs/heads/master@{#371353}