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

Issue 727916 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Traveling - Back 2/6
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug


Show other hotlists

Hotlists containing this issue:
In-Product-Help


Sign in to add a comment

Unit test for SystemTimeProvider for In-Product Help requires UTC

Project Member Reported by nyquist@chromium.org, May 30 2017

Issue description

Chrome Version: 8afbdf7d2bfa9779d548d9d131ae0415813453fb
OS: Android

What steps will reproduce the problem?
(1) Change timezone away from UTC / GMT+0
(2) Run components_unittests.

What is the expected result?
They work

What happens instead?
A test fails.

See https://chromium-review.googlesource.com/c/501469/12/components/feature_engagement_tracker/internal/system_time_provider_unittest.cc#92 for the report.


 
Owner: nyquist@chromium.org
Status: Started (was: Untriaged)
CL in review: https://chromium-review.googlesource.com/c/517870/
Project Member

Comment 3 by bugdroid1@chromium.org, May 31 2017

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

commit 867e554db2e9d5b2cd58d684544b81c8d170144b
Author: Tommy Nyquist <nyquist@chromium.org>
Date: Wed May 31 01:50:53 2017

Unit test for feature engagement tracker time provider requiest UTC

The unit test for SystemTimeProvider uses
base::Time::FromLocalExploded() to create a hard coded time. However,
this has the unfortunate side effect of creating a different number
based on the local timezone.

This CL changes the test to instead use base::Time::FromUTCExploded()
which forces the timezone to be UTC, which fixes test errors.

BUG= 727916 

Change-Id: I027ef0c5e100cb04ddad7472e8a8eaa7a3521a7f
Reviewed-on: https://chromium-review.googlesource.com/517870
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#475734}
[modify] https://crrev.com/867e554db2e9d5b2cd58d684544b81c8d170144b/components/feature_engagement_tracker/internal/system_time_provider_unittest.cc

Status: Fixed (was: Started)
Labels: -M-61 M-60 Merge-Request-60
Since this will lead to issues with components_unittests on the M60-branch, requesting merge.
Project Member

Comment 6 by sheriffbot@chromium.org, May 31 2017

Labels: -Merge-Request-60 Hotlist-Merge-Approved Merge-Approved-60
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 7 by bugdroid1@chromium.org, May 31 2017

Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86a78e68cf0b17f16213dd48cefae153212d6640

commit 86a78e68cf0b17f16213dd48cefae153212d6640
Author: Tommy Nyquist <nyquist@chromium.org>
Date: Wed May 31 04:45:51 2017

Unit test for feature engagement tracker time provider requiest UTC

The unit test for SystemTimeProvider uses
base::Time::FromLocalExploded() to create a hard coded time. However,
this has the unfortunate side effect of creating a different number
based on the local timezone.

This CL changes the test to instead use base::Time::FromUTCExploded()
which forces the timezone to be UTC, which fixes test errors.

BUG= 727916 
TBR=nyquist@chromium.org

(cherry picked from commit 867e554db2e9d5b2cd58d684544b81c8d170144b)

Change-Id: I027ef0c5e100cb04ddad7472e8a8eaa7a3521a7f
Reviewed-on: https://chromium-review.googlesource.com/517870
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Tommy Nyquist <nyquist@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#475734}
Reviewed-on: https://chromium-review.googlesource.com/518584
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/branch-heads/3112@{#48}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
[modify] https://crrev.com/86a78e68cf0b17f16213dd48cefae153212d6640/components/feature_engagement_tracker/internal/system_time_provider_unittest.cc

Components: Internals>FeatureEngagementTracker

Sign in to add a comment