Improve update checking (#18193)

* Improve update checking

This change emables pinging the server to check for updates regardless of whether the local version is "out of date". The server code already has a 7-day cache so the result is that we can now ping the server once every 7 days instead of waiting for the local install to be 4 weeks out of date before pinging.

The original 4 week period is still used for when we'll start warning the user they're out of date if we could not confirm with the server whether there's a new version.

* Improve message when we know there's a new version available

* Fix bnullable bool checks

* Switch nullable bool to enum

* Fix casing of enum values

* Remove stale comment

The names are now descriptive so doesn't need additional explanation.

* Improve name of function

* Remove note:

* Rename kPauseToLetUserReadTheMessage -> timeToPauseToLetUserReadTheMessage

* Change kVersionAgeConsideredUpToDate to 5 weeks from 4

* Inline the isNewerFrameworkVersionAvailable check

* Fix indenting (?)

* Fix more indenting

* Rename function to be clearer it's getting the commit date

* Formating tweaks

* Update stamp when connection failed, and reduce time before we'll try again

Previously we would hit the server on every command if we thought we might be out of date and we never successfully connected (eg. if you're offline). This makes the stamp update even when there's a conneciton failure so that this won't happen, but reduces the time till we check again from 7 days to 3 days to compensate a little in case it was a one-off.

https://github.com/flutter/flutter/pull/18193#issuecomment-399222269

* Fix comment

* Don't perform update checks if not on an official channel

packages/flutter_tools/lib/src/version.dart

@@ -206,12 +206,14 @@ class FlutterVersion {
   /// The amount of time we wait before pinging the server to check for the
   /// availability of a newer version of Flutter.
   @visibleForTesting
-  static const Duration kCheckAgeConsideredUpToDate = const Duration(days: 7);
+  static const Duration kCheckAgeConsideredUpToDate = const Duration(days: 3);
 
   /// We warn the user if the age of their Flutter installation is greater than
   /// this duration.
+  ///
+  /// This is set to 5 weeks because releases are currently around every 4 weeks.
   @visibleForTesting
-  static final Duration kVersionAgeConsideredUpToDate = kCheckAgeConsideredUpToDate * 4;
+  static const Duration kVersionAgeConsideredUpToDate = const Duration(days: 35);
 
   /// The amount of time we wait between issuing a warning.
   ///
@@ -224,7 +226,7 @@ class FlutterVersion {
   ///
   /// This can be customized in tests to speed them up.
   @visibleForTesting
-  static Duration kPauseToLetUserReadTheMessage = const Duration(seconds: 2);
+  static Duration timeToPauseToLetUserReadTheMessage = const Duration(seconds: 2);
 
   /// Checks if the currently installed version of Flutter is up-to-date, and
   /// warns the user if it isn't.
@@ -232,30 +234,49 @@ class FlutterVersion {
   /// This function must run while [Cache.lock] is acquired because it reads and
   /// writes shared cache files.
   Future<Null> checkFlutterVersionFreshness() async {
+    // Don't perform update checks if we're not on an official channel.
+    if (!officialChannels.contains(_channel)) {
+      return;
+    }
+
     final DateTime localFrameworkCommitDate = DateTime.parse(frameworkCommitDate);
     final Duration frameworkAge = _clock.now().difference(localFrameworkCommitDate);
     final bool installationSeemsOutdated = frameworkAge > kVersionAgeConsideredUpToDate;
 
-    Future<bool> newerFrameworkVersionAvailable() async {
-      final DateTime latestFlutterCommitDate = await _getLatestAvailableFlutterVersion();
-
-      if (latestFlutterCommitDate == null)
-        return false;
-
-      return latestFlutterCommitDate.isAfter(localFrameworkCommitDate);
-    }
-
+    // Get whether there's a newer version on the remote. This only goes
+    // to the server if we haven't checked recently so won't happen on every
+    // command.
+    final DateTime latestFlutterCommitDate = await _getLatestAvailableFlutterDate();
+    final VersionCheckResult remoteVersionStatus =
+        latestFlutterCommitDate == null
+            ? VersionCheckResult.unknown
+            : latestFlutterCommitDate.isAfter(localFrameworkCommitDate)
+                ? VersionCheckResult.newVersionAvailable
+                : VersionCheckResult.versionIsCurrent;
+
+    // Do not load the stamp before the above server check as it may modify the stamp file.
     final VersionCheckStamp stamp = await VersionCheckStamp.load();
     final DateTime lastTimeWarningWasPrinted = stamp.lastTimeWarningWasPrinted ?? _clock.agoBy(kMaxTimeSinceLastWarning * 2);
     final bool beenAWhileSinceWarningWasPrinted = _clock.now().difference(lastTimeWarningWasPrinted) > kMaxTimeSinceLastWarning;
 
-    if (beenAWhileSinceWarningWasPrinted && installationSeemsOutdated && await newerFrameworkVersionAvailable()) {
-      printStatus(versionOutOfDateMessage(frameworkAge), emphasis: true);
+    // We show a warning if either we know there is a new remote version, or we couldn't tell but the local
+    // version is outdated.
+    final bool canShowWarning =
+        remoteVersionStatus == VersionCheckResult.newVersionAvailable ||
+            (remoteVersionStatus == VersionCheckResult.unknown &&
+                installationSeemsOutdated);
+
+    if (beenAWhileSinceWarningWasPrinted && canShowWarning) {
+      final String updateMessage =
+          remoteVersionStatus == VersionCheckResult.newVersionAvailable
+              ? newVersionAvailableMessage()
+              : versionOutOfDateMessage(frameworkAge);
+      printStatus(updateMessage, emphasis: true);
       await Future.wait<Null>(<Future<Null>>[
         stamp.store(
           newTimeWarningWasPrinted: _clock.now(),
         ),
-        new Future<Null>.delayed(kPauseToLetUserReadTheMessage),
+        new Future<Null>.delayed(timeToPauseToLetUserReadTheMessage),
       ]);
     }
   }
@@ -275,6 +296,17 @@ class FlutterVersion {
 ''';
   }
 
+  @visibleForTesting
+  static String newVersionAvailableMessage() {
+    return '''
+  ╔════════════════════════════════════════════════════════════════════════════╗
+  ║ A new version of Flutter is available!                                     ║
+  ║                                                                            ║
+  ║ To update to the latest version, run "flutter upgrade".                    ║
+  ╚════════════════════════════════════════════════════════════════════════════╝
+''';
+  }
+
   /// Gets the release date of the latest available Flutter version.
   ///
   /// This method sends a server request if it's been more than
@@ -282,7 +314,7 @@ class FlutterVersion {
   ///
   /// Returns null if the cached version is out-of-date or missing, and we are
   /// unable to reach the server to get the latest version.
-  Future<DateTime> _getLatestAvailableFlutterVersion() async {
+  Future<DateTime> _getLatestAvailableFlutterDate() async {
     Cache.checkLockAcquired();
     final VersionCheckStamp versionCheckStamp = await VersionCheckStamp.load();
 
@@ -296,8 +328,7 @@ class FlutterVersion {
 
     // Cache is empty or it's been a while since the last server ping. Ping the server.
     try {
-      final String branch = officialChannels.contains(_channel) ? _channel : 'master';
-      final DateTime remoteFrameworkCommitDate = DateTime.parse(await FlutterVersion.fetchRemoteFrameworkCommitDate(branch));
+      final DateTime remoteFrameworkCommitDate = DateTime.parse(await FlutterVersion.fetchRemoteFrameworkCommitDate(_channel));
       await versionCheckStamp.store(
         newTimeVersionWasChecked: _clock.now(),
         newKnownRemoteVersion: remoteFrameworkCommitDate,
@@ -308,6 +339,11 @@ class FlutterVersion {
       // there's no Internet connectivity. Remote version check is best effort
       // only. We do not prevent the command from running when it fails.
       printTrace('Failed to check Flutter version in the remote repository: $error');
+      // Still update the timestamp to avoid us hitting the server on every single
+      // command if for some reason we cannot connect (eg. we may be offline).
+      await versionCheckStamp.store(
+        newTimeVersionWasChecked: _clock.now(),
+      );
       return null;
     }
   }
@@ -355,8 +391,8 @@ class VersionCheckStamp {
   static VersionCheckStamp fromJson(Map<String, String> jsonObject) {
     DateTime readDateTime(String property) {
       return jsonObject.containsKey(property)
-        ? DateTime.parse(jsonObject[property])
-        : null;
+          ? DateTime.parse(jsonObject[property])
+          : null;
     }
 
     return new VersionCheckStamp(
@@ -513,3 +549,13 @@ class GitTagVersion {
     return '$x.$y.${z + 1}-pre.$commits';
   }
 }
+
+enum VersionCheckResult {
+  /// Unable to check whether a new version is available, possibly due to
+  /// a connectivity issue.
+  unknown,
+  /// The current version is up to date.
+  versionIsCurrent,
+  /// A newer version is available.
+  newVersionAvailable,
+}

packages/flutter_tools/test/version_test.dart

@@ -36,7 +36,7 @@ void main() {
       <String>['git', 'rev-parse', '--abbrev-ref', '--symbolic', '@{u}'],
       workingDirectory: anyNamed('workingDirectory'),
       environment: anyNamed('environment'),
-    )).thenReturn(new ProcessResult(101, 0, 'channel', ''));
+    )).thenReturn(new ProcessResult(101, 0, 'master', ''));
     when(mockProcessManager.runSync(
       <String>['git', 'rev-parse', '--abbrev-ref', 'HEAD'],
       workingDirectory: anyNamed('workingDirectory'),
@@ -62,11 +62,18 @@ void main() {
   group('$FlutterVersion', () {
     setUpAll(() {
       Cache.disableLocking();
-      FlutterVersion.kPauseToLetUserReadTheMessage = Duration.zero;
+      FlutterVersion.timeToPauseToLetUserReadTheMessage = Duration.zero;
     });
 
     testUsingContext('prints nothing when Flutter installation looks fresh', () async {
-      fakeData(mockProcessManager, mockCache, localCommitDate: _upToDateVersion);
+      fakeData(
+        mockProcessManager,
+        mockCache,
+        localCommitDate: _upToDateVersion,
+        // Server will be pinged because we haven't pinged within last x days
+        expectServerPing: true,
+        remoteCommitDate: _outOfDateVersion,
+        expectSetStamp: true);
       await FlutterVersion.instance.checkFlutterVersionFreshness();
       _expectVersionMessage('');
     }, overrides: <Type, Generator>{
@@ -114,7 +121,7 @@ void main() {
       );
 
       await version.checkFlutterVersionFreshness();
-      _expectVersionMessage(FlutterVersion.versionOutOfDateMessage(_testClock.now().difference(_outOfDateVersion)));
+      _expectVersionMessage(FlutterVersion.newVersionAvailableMessage());
     }, overrides: <Type, Generator>{
       FlutterVersion: () => new FlutterVersion(_testClock),
       ProcessManager: () => mockProcessManager,
@@ -136,7 +143,7 @@ void main() {
       );
 
       await version.checkFlutterVersionFreshness();
-      _expectVersionMessage(FlutterVersion.versionOutOfDateMessage(_testClock.now().difference(_outOfDateVersion)));
+      _expectVersionMessage(FlutterVersion.newVersionAvailableMessage());
       expect((await VersionCheckStamp.load()).lastTimeWarningWasPrinted, _testClock.now());
 
       await version.checkFlutterVersionFreshness();
@@ -160,7 +167,7 @@ void main() {
       );
 
       await version.checkFlutterVersionFreshness();
-      _expectVersionMessage(FlutterVersion.versionOutOfDateMessage(_testClock.now().difference(_outOfDateVersion)));
+      _expectVersionMessage(FlutterVersion.newVersionAvailableMessage());
 
       // Immediate subsequent check is not expected to ping the server.
       fakeData(
@@ -194,22 +201,23 @@ void main() {
       );
 
       await version.checkFlutterVersionFreshness();
-      _expectVersionMessage(FlutterVersion.versionOutOfDateMessage(_testClock.now().difference(_outOfDateVersion)));
+      _expectVersionMessage(FlutterVersion.newVersionAvailableMessage());
     }, overrides: <Type, Generator>{
       FlutterVersion: () => new FlutterVersion(_testClock),
       ProcessManager: () => mockProcessManager,
       Cache: () => mockCache,
     });
 
-    testUsingContext('ignores network issues', () async {
+    testUsingContext('does not print warning when unable to connect to server if not out of date', () async {
       final FlutterVersion version = FlutterVersion.instance;
 
       fakeData(
         mockProcessManager,
         mockCache,
-        localCommitDate: _outOfDateVersion,
+        localCommitDate: _upToDateVersion,
         errorOnFetch: true,
         expectServerPing: true,
+        expectSetStamp: true,
       );
 
       await version.checkFlutterVersionFreshness();
@@ -220,6 +228,26 @@ void main() {
       Cache: () => mockCache,
     });
 
+    testUsingContext('prints warning when unable to connect to server if really out of date', () async {
+      final FlutterVersion version = FlutterVersion.instance;
+
+      fakeData(
+        mockProcessManager,
+        mockCache,
+        localCommitDate: _outOfDateVersion,
+        errorOnFetch: true,
+        expectServerPing: true,
+        expectSetStamp: true
+      );
+
+      await version.checkFlutterVersionFreshness();
+      _expectVersionMessage(FlutterVersion.versionOutOfDateMessage(_testClock.now().difference(_outOfDateVersion)));
+    }, overrides: <Type, Generator>{
+      FlutterVersion: () => new FlutterVersion(_testClock),
+      ProcessManager: () => mockProcessManager,
+      Cache: () => mockCache,
+    });
+
     testUsingContext('versions comparison', () async {
       when(mockProcessManager.runSync(
         <String>['git', 'merge-base', '--is-ancestor', 'abcdef', '123456'],