New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 793470 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Add UKM browsertest for history deletions.

Project Member Reported by holte@chromium.org, Dec 8 2017

Issue description

We should have a browsertest (and EG test for iOS) which makes sure history deletions are handled properly by UKM.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a7fbdc52318f6169ae27ef251c52d91b5480aa9

commit 2a7fbdc52318f6169ae27ef251c52d91b5480aa9
Author: Steven Holte <holte@google.com>
Date: Mon Dec 11 23:47:19 2017

Add test coverage of UKM History delete handling.

Coverage on iOS left for a follow up.

Bug:  793470 
Change-Id: I4844e667c421f7df3c3412362d50ad74078e8223
Reviewed-on: https://chromium-review.googlesource.com/818202
Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523256}
[modify] https://crrev.com/2a7fbdc52318f6169ae27ef251c52d91b5480aa9/chrome/browser/metrics/ukm_browsertest.cc

Note that, on iOS, we have the testNoSync test, which asserts that the client id is not reset when signing out of sync. I assume this is correct but we should just make sure that the behavior is intended, and consider adding an equivalent non-iOS test so we verify that behaviors match across platforms.
It seems like the non-iOS test where we could match this behavior is UkmBrowserTest.SyncSignoutCheck. There's no assertion that the client id does not change in this test currently. We should consider adding it to match the iOS test.
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fdf4d5eb8e20a0a66bd659400bd2729f3ca2072c

commit fdf4d5eb8e20a0a66bd659400bd2729f3ca2072c
Author: Reid Kleckner <rnk@google.com>
Date: Thu Dec 21 20:43:07 2017

Revert "Add test coverage of UKM History delete handling."

This reverts commit 2a7fbdc52318f6169ae27ef251c52d91b5480aa9.

Reason for revert: This new test is flaky on Windows.

Original change's description:
> Add test coverage of UKM History delete handling.
> 
> Coverage on iOS left for a follow up.
> 
> Bug:  793470 
> Change-Id: I4844e667c421f7df3c3412362d50ad74078e8223
> Reviewed-on: https://chromium-review.googlesource.com/818202
> Reviewed-by: Brian White <bcwhite@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Commit-Queue: Steven Holte <holte@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#523256}

TBR=asvitkine@chromium.org,bcwhite@chromium.org,holte@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  793470 , 796236 
Change-Id: I43356b1e4f5bea9b71b206ffe7ef1496d9d41b7d
Reviewed-on: https://chromium-review.googlesource.com/840862
Commit-Queue: Reid Kleckner <rnk@chromium.org>
Reviewed-by: Reid Kleckner <rnk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525795}
[modify] https://crrev.com/fdf4d5eb8e20a0a66bd659400bd2729f3ca2072c/chrome/browser/metrics/ukm_browsertest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b4830034233ff787d2d54a093cbce229fed197b1

commit b4830034233ff787d2d54a093cbce229fed197b1
Author: Steven Holte <holte@chromium.org>
Date: Wed Jan 17 20:44:50 2018

UKM History Deletion test for iOS.

BUG= 793470 

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ib729e61bddbfaf65a3f9237dc2ac70db559f7763
Reviewed-on: https://chromium-review.googlesource.com/869121
Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Commit-Queue: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529879}
[modify] https://crrev.com/b4830034233ff787d2d54a093cbce229fed197b1/ios/chrome/browser/metrics/ukm_egtest.mm

Is this complete?

Comment 7 by holte@chromium.org, Jan 22 2018

I think we need some coverage of this on Android as well, since neither browsertests nor egtests run on Android.  So I think we need something similar to https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromium/chrome/browser/metrics/UkmIncognitoTest.java

Comment 8 by holte@chromium.org, Jan 22 2018

Status: Fixed (was: Assigned)
Filed https://bugs.chromium.org/p/chromium/issues/detail?id=804439
for Android follow up work, marking this one as fixed.

Sign in to add a comment