From 11074780f5458df5d5347a7e388a05f7b11c408f Mon Sep 17 00:00:00 2001 From: Neil Fuller Date: Fri, 8 Apr 2022 14:11:17 +0100 Subject: [PATCH] Fix metrics test flakiness This commit makes two changes: 1) The shell commands for accessing config went directly to the ServiceConfigAccessor, while metrics used a cached copy of config. This means they may disagree under some circumstances like automated tests. This change tunnels shell command queries to the strategy object, which generates the metrics, ensuring a higher chance of consistency. 2) Some types of configuration change (used during tests) were not causing the copy of ConfigurationInternal held by TimeZoneDetectorStrategyImpl to be updated. This corrects that. This should fix automated tests that were receiving constradictory information about whether geo detection was supported on a device from shell command Vs metrics reporting. It is likely this is the result of a previous test changing the supported status (probably by changing the config of location time zone providers) and the issues fixed by points 1 and 2 (mostly 2) above causing the contradiction. Bug: 228457046 Test: Treehugger Change-Id: I95e79575ec102662bcbfbbab7c99c07437e3a633 --- .../ServiceConfigAccessorImpl.java | 12 ++++++++++++ .../timezonedetector/TimeZoneDetectorService.java | 4 ++-- .../timezonedetector/TimeZoneDetectorStrategy.java | 6 ++++++ .../TimeZoneDetectorStrategyImpl.java | 14 ++++++++++++++ .../FakeTimeZoneDetectorStrategy.java | 10 ++++++++++ 5 files changed, 44 insertions(+), 2 deletions(-) diff --git a/services/core/java/com/android/server/timezonedetector/ServiceConfigAccessorImpl.java b/services/core/java/com/android/server/timezonedetector/ServiceConfigAccessorImpl.java index ae52912d4821a..7173f60081775 100644 --- a/services/core/java/com/android/server/timezonedetector/ServiceConfigAccessorImpl.java +++ b/services/core/java/com/android/server/timezonedetector/ServiceConfigAccessorImpl.java @@ -59,6 +59,8 @@ public final class ServiceConfigAccessorImpl implements ServiceConfigAccessor { */ private static final Set CONFIGURATION_INTERNAL_SERVER_FLAGS_KEYS_TO_WATCH = Set.of( ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_FEATURE_SUPPORTED, + ServerFlags.KEY_PRIMARY_LTZP_MODE_OVERRIDE, + ServerFlags.KEY_SECONDARY_LTZP_MODE_OVERRIDE, ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_RUN_IN_BACKGROUND_ENABLED, ServerFlags.KEY_ENHANCED_METRICS_COLLECTION_ENABLED, ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_SETTING_ENABLED_DEFAULT, @@ -443,6 +445,9 @@ public final class ServiceConfigAccessorImpl implements ServiceConfigAccessor { mTestPrimaryLocationTimeZoneProviderMode = mTestPrimaryLocationTimeZoneProviderPackageName == null ? PROVIDER_MODE_DISABLED : PROVIDER_MODE_ENABLED; + // Changing this state can affect the content of ConfigurationInternal, so listeners need to + // be informed. + mContext.getMainThreadHandler().post(this::handleConfigurationInternalChangeOnMainThread); } @Override @@ -469,6 +474,9 @@ public final class ServiceConfigAccessorImpl implements ServiceConfigAccessor { mTestSecondaryLocationTimeZoneProviderMode = mTestSecondaryLocationTimeZoneProviderPackageName == null ? PROVIDER_MODE_DISABLED : PROVIDER_MODE_ENABLED; + // Changing this state can affect the content of ConfigurationInternal, so listeners need to + // be informed. + mContext.getMainThreadHandler().post(this::handleConfigurationInternalChangeOnMainThread); } @Override @@ -573,6 +581,10 @@ public final class ServiceConfigAccessorImpl implements ServiceConfigAccessor { mTestSecondaryLocationTimeZoneProviderPackageName = null; mTestSecondaryLocationTimeZoneProviderMode = null; mRecordStateChangesForTests = false; + + // Changing LTZP config can affect the content of ConfigurationInternal, so listeners + // need to be informed. + mContext.getMainThreadHandler().post(this::handleConfigurationInternalChangeOnMainThread); } private boolean isTelephonyFallbackSupported() { diff --git a/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorService.java b/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorService.java index f75608e145395..898d02e212f46 100644 --- a/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorService.java +++ b/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorService.java @@ -336,13 +336,13 @@ public final class TimeZoneDetectorService extends ITimeZoneDetectorService.Stub boolean isTelephonyTimeZoneDetectionSupported() { enforceManageTimeZoneDetectorPermission(); - return mServiceConfigAccessor.isTelephonyTimeZoneDetectionFeatureSupported(); + return mTimeZoneDetectorStrategy.isTelephonyTimeZoneDetectionSupported(); } boolean isGeoTimeZoneDetectionSupported() { enforceManageTimeZoneDetectorPermission(); - return mServiceConfigAccessor.isGeoTimeZoneDetectionFeatureSupported(); + return mTimeZoneDetectorStrategy.isGeoTimeZoneDetectionSupported(); } /** diff --git a/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorStrategy.java b/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorStrategy.java index 6b04adf7de618..95ebd6803cd0a 100644 --- a/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorStrategy.java +++ b/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorStrategy.java @@ -124,4 +124,10 @@ public interface TimeZoneDetectorStrategy extends Dumpable { /** Generates a state snapshot for metrics. */ @NonNull MetricsTimeZoneDetectorState generateMetricsState(); + + /** Returns {@code true} if the device supports telephony time zone detection. */ + boolean isTelephonyTimeZoneDetectionSupported(); + + /** Returns {@code true} if the device supports geolocation time zone detection. */ + boolean isGeoTimeZoneDetectionSupported(); } diff --git a/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorStrategyImpl.java b/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorStrategyImpl.java index e21d0e45ef0e0..66c23f5d01a4e 100644 --- a/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorStrategyImpl.java +++ b/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorStrategyImpl.java @@ -396,6 +396,20 @@ public final class TimeZoneDetectorStrategyImpl implements TimeZoneDetectorStrat getLatestGeolocationSuggestion()); } + @Override + public boolean isTelephonyTimeZoneDetectionSupported() { + synchronized (this) { + return mCurrentConfigurationInternal.isTelephonyDetectionSupported(); + } + } + + @Override + public boolean isGeoTimeZoneDetectionSupported() { + synchronized (this) { + return mCurrentConfigurationInternal.isGeoDetectionSupported(); + } + } + private static int scoreTelephonySuggestion(@NonNull TelephonyTimeZoneSuggestion suggestion) { int score; if (suggestion.getZoneId() == null) { diff --git a/services/tests/servicestests/src/com/android/server/timezonedetector/FakeTimeZoneDetectorStrategy.java b/services/tests/servicestests/src/com/android/server/timezonedetector/FakeTimeZoneDetectorStrategy.java index 2d0dca25465b9..c9fc033e90867 100644 --- a/services/tests/servicestests/src/com/android/server/timezonedetector/FakeTimeZoneDetectorStrategy.java +++ b/services/tests/servicestests/src/com/android/server/timezonedetector/FakeTimeZoneDetectorStrategy.java @@ -60,6 +60,16 @@ class FakeTimeZoneDetectorStrategy implements TimeZoneDetectorStrategy { throw new UnsupportedOperationException(); } + @Override + public boolean isTelephonyTimeZoneDetectionSupported() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isGeoTimeZoneDetectionSupported() { + throw new UnsupportedOperationException(); + } + @Override public void dump(IndentingPrintWriter pw, String[] args) { mDumpCalled = true;