From 48549f30da96f3a1a4da556b6b53cc4a7f1e17fd Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sun, 30 May 2021 02:38:25 +0200 Subject: [PATCH 01/20] Add sync engine handling pings --- lib/src/common/network/wiredash_api.dart | 6 ++ lib/src/sync/sync_engine.dart | 82 +++++++++++++++++++ lib/src/wiredash_widget.dart | 6 ++ .../pending_feedback_item_storage_test.dart | 21 ++++- test/sync/sync_engine_test.dart | 72 ++++++++++++++++ test/util/invocation_catcher.dart | 4 +- 6 files changed, 188 insertions(+), 3 deletions(-) create mode 100644 lib/src/sync/sync_engine.dart create mode 100644 test/sync/sync_engine_test.dart diff --git a/lib/src/common/network/wiredash_api.dart b/lib/src/common/network/wiredash_api.dart index 5de97aaf..eece0f32 100644 --- a/lib/src/common/network/wiredash_api.dart +++ b/lib/src/common/network/wiredash_api.dart @@ -60,6 +60,10 @@ class WiredashApi { throw WiredashApiException(response: response); } + Future ping() { + throw "TODO implement"; + } + /// Sends a [BaseRequest] after attaching HTTP headers Future _send(BaseRequest request) async { request.headers['project'] = 'Project $_projectId'; @@ -70,6 +74,8 @@ class WiredashApi { } } +class PingResponse {} + /// Generic error from the Wiredash API class WiredashApiException implements Exception { WiredashApiException({String? message, this.response}) : _message = message; diff --git a/lib/src/sync/sync_engine.dart b/lib/src/sync/sync_engine.dart new file mode 100644 index 00000000..c76a476a --- /dev/null +++ b/lib/src/sync/sync_engine.dart @@ -0,0 +1,82 @@ +import 'dart:async'; + +import 'package:flutter/cupertino.dart'; +import 'package:shared_preferences/shared_preferences.dart'; +import 'package:wiredash/src/common/network/wiredash_api.dart'; +import 'package:clock/clock.dart'; + +class SyncEngine { + final WiredashApi _api; + final Future Function() _sharedPreferences; + + SyncEngine( + WiredashApi api, Future Function() sharedPreferences) + : _api = api, + _sharedPreferences = sharedPreferences; + + void dispose() { + _initTimer?.cancel(); + } + + Timer? _initTimer; + + static const minSyncGap = Duration(hours: 1); + + static const lastSuccessfulPingKey = 'io.wiredash.last_successful_ping'; + static const lastFeedbackSubmissionKey = + 'io.wiredash.last_feedback_submission'; + + /// Called when the SDK is initialized (by wrapping the app) + /// + /// This eventually syncs with the backend + Future onWiredashInitialized() async { + assert(() { + if (_initTimer != null) { + debugPrint("called onWiredashInitialized multiple times"); + } + return true; + }()); + + final now = clock.now(); + final preferences = await _sharedPreferences(); + final lastPingInt = preferences.getInt(lastSuccessfulPingKey); + final lastPing = lastPingInt != null + ? DateTime.fromMillisecondsSinceEpoch(lastPingInt) + : null; + + if (lastPing != null && lastPing.difference(now).abs() > minSyncGap) { + _initTimer?.cancel(); + _initTimer = Timer(Duration(seconds: 2), _ping); + } + } + + /// Called when the user manually opened Wiredash + /// + /// This 100% calls the backend, forcing a sync + Future onUserOpenedWiredash() async { + await _ping(); + } + + /// Pings the backend with a very cheep call checking if anything should be synced + Future _ping() async { + try { + final response = await _api.ping(); + final preferences = await _sharedPreferences(); + final now = clock.now(); + await preferences.setInt(lastSuccessfulPingKey, now.millisecond); + } catch (e, stack) { + // TODO + print(e); + print(stack); + } + } + + /// Remembers the time (now) when the last feedback was submitted + /// + /// This information is used to trigger [_ping] on app start within [minSyncGap] periode + Future rememberFeedbackSubmission() async { + final now = clock.now(); + final preferences = await _sharedPreferences(); + await preferences.setInt(lastFeedbackSubmissionKey, now.millisecond); + } +} diff --git a/lib/src/wiredash_widget.dart b/lib/src/wiredash_widget.dart index cabc272a..ba98ad8b 100644 --- a/lib/src/wiredash_widget.dart +++ b/lib/src/wiredash_widget.dart @@ -21,6 +21,7 @@ import 'package:wiredash/src/feedback/data/direct_feedback_submitter.dart'; import 'package:wiredash/src/feedback/data/pending_feedback_item_storage.dart'; import 'package:wiredash/src/feedback/data/retrying_feedback_submitter.dart'; import 'package:wiredash/src/feedback/feedback_model.dart'; +import 'package:wiredash/src/sync/sync_engine.dart'; import 'package:wiredash/src/wiredash_controller.dart'; import 'package:wiredash/src/wiredash_provider.dart'; @@ -135,6 +136,8 @@ class WiredashState extends State { late WiredashOptionsData _options; late WiredashThemeData _theme; + late SyncEngine _syncEngine; + @override void initState() { super.initState(); @@ -153,6 +156,7 @@ class WiredashState extends State { projectId: widget.projectId, secret: widget.secret, ); + _syncEngine = SyncEngine(_api, SharedPreferences.getInstance); userManager = UserManager(); buildInfoManager = BuildInfoManager(PlatformBuildInfo()); @@ -179,6 +183,8 @@ class WiredashState extends State { WidgetsBinding.instance!.window, ), ); + + _syncEngine.onWiredashInitialized(); } @override diff --git a/test/feedback/data/pending_feedback_item_storage_test.dart b/test/feedback/data/pending_feedback_item_storage_test.dart index 22a6a542..1e2ded8d 100644 --- a/test/feedback/data/pending_feedback_item_storage_test.dart +++ b/test/feedback/data/pending_feedback_item_storage_test.dart @@ -20,7 +20,6 @@ class FakeSharedPreferences extends Fake implements SharedPreferences { final MethodInvocationCatcher setStringListInvocations = MethodInvocationCatcher('setStringList'); - @override Future setStringList(String key, List value) async { await setStringListInvocations.addMethodCall(args: [key, value]); @@ -38,6 +37,26 @@ class FakeSharedPreferences extends Fake implements SharedPreferences { } return _store[key] as List?; } + + final MethodInvocationCatcher setIntInvocations = + MethodInvocationCatcher('setInt'); + @override + Future setInt(String key, int value) async { + await setIntInvocations.addMethodCall(args: [key, value]); + _store[key] = value; + return true; + } + + final MethodInvocationCatcher getIntInvocations = + MethodInvocationCatcher('getInt'); + @override + int? getInt(String key) { + final result = getIntInvocations.addMethodCall(args: [key]); + if (result != null) { + return result as int?; + } + return _store[key] as int?; + } } class IncrementalUuidV4Generator implements UuidV4Generator { diff --git a/test/sync/sync_engine_test.dart b/test/sync/sync_engine_test.dart new file mode 100644 index 00000000..e1d28f0b --- /dev/null +++ b/test/sync/sync_engine_test.dart @@ -0,0 +1,72 @@ +import 'package:fake_async/fake_async.dart'; +import 'package:test/fake.dart'; +import 'package:test/test.dart'; +import 'package:wiredash/src/common/network/wiredash_api.dart'; +import 'package:wiredash/src/sync/sync_engine.dart'; +import 'package:clock/clock.dart'; + +import '../feedback/data/pending_feedback_item_storage_test.dart'; +import '../util/invocation_catcher.dart'; + +void main() { + group('Triggering ping', () { + late _MockWiredashApi api; + late FakeSharedPreferences prefs; + late SyncEngine syncEngine; + + setUp(() { + api = _MockWiredashApi(); + prefs = FakeSharedPreferences(); + syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); + }); + + test('Never opened Wiredash does not trigger ping', () { + fakeAsync((async) { + syncEngine.onWiredashInitialized(); + async.elapse(Duration(seconds: 10)); + expect(api.pingInvocations.count, 0); + }); + }); + + test( + 'appstart pings when the user submitted feedback/sent message ' + 'in the last 30 days (delayed by 2 seconds)', () { + fakeAsync((async) { + // Given last feedback 29 days ago + syncEngine.rememberFeedbackSubmission(); + async.elapse(Duration(days: 29)); + + // Given last sync 2 hours ago + prefs.setInt(SyncEngine.lastSuccessfulPingKey, clock.now().millisecond); + async.elapse(Duration(hours: 2)); + + syncEngine.onWiredashInitialized(); + async.elapse(Duration(milliseconds: 1990)); + expect(api.pingInvocations.count, 0); + async.elapse(Duration(milliseconds: 100)); + expect(api.pingInvocations.count, 1); + }); + }); + + test('opening wiredash triggers ping immediately', () { + fakeAsync((async) { + expect(api.pingInvocations.count, 0); + syncEngine.onUserOpenedWiredash(); + async.elapse(Duration(milliseconds: 1)); + expect(api.pingInvocations.count, 1); + }); + }); + }); +} + +class _MockWiredashApi extends Fake implements WiredashApi { + final MethodInvocationCatcher pingInvocations = + MethodInvocationCatcher('ping'); + + @override + Future ping() async { + await pingInvocations.addMethodCall(); + throw "Not implemented"; + } +} diff --git a/test/util/invocation_catcher.dart b/test/util/invocation_catcher.dart index ae2eb4a4..2f1ed161 100644 --- a/test/util/invocation_catcher.dart +++ b/test/util/invocation_catcher.dart @@ -38,6 +38,7 @@ class MethodInvocationCatcher { return null; } + /// Add an interceptor to get a callback when a method is called or return mock data to the caller dynamic Function(Invocation invocation)? interceptor; void verifyInvocationCount(int n) { @@ -56,6 +57,7 @@ class MethodInvocationCatcher { } } +/// A invocation which can be used to assert specific values class AssertableInvocation { AssertableInvocation(this.original); @@ -87,5 +89,3 @@ class AssertableInvocation { return "${original.memberName}(named: ${original.namedArguments}, positional: ${original.positionalArguments})"; } } - -class WithArgument {} From 5b22348e1a05a89d7efc4b65a735560bb2011608 Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sun, 30 May 2021 02:48:04 +0200 Subject: [PATCH 02/20] Fix analysis --- lib/src/common/network/wiredash_api.dart | 8 +++++++- lib/src/sync/sync_engine.dart | 9 +++++---- test/sync/sync_engine_test.dart | 14 +++++++------- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/lib/src/common/network/wiredash_api.dart b/lib/src/common/network/wiredash_api.dart index eece0f32..7a354152 100644 --- a/lib/src/common/network/wiredash_api.dart +++ b/lib/src/common/network/wiredash_api.dart @@ -74,7 +74,13 @@ class WiredashApi { } } -class PingResponse {} +class PingResponse { + bool hasNewMessages; + + PingResponse({ + required this.hasNewMessages, + }); +} /// Generic error from the Wiredash API class WiredashApiException implements Exception { diff --git a/lib/src/sync/sync_engine.dart b/lib/src/sync/sync_engine.dart index c76a476a..64cae0a1 100644 --- a/lib/src/sync/sync_engine.dart +++ b/lib/src/sync/sync_engine.dart @@ -1,9 +1,9 @@ import 'dart:async'; +import 'package:clock/clock.dart'; import 'package:flutter/cupertino.dart'; import 'package:shared_preferences/shared_preferences.dart'; import 'package:wiredash/src/common/network/wiredash_api.dart'; -import 'package:clock/clock.dart'; class SyncEngine { final WiredashApi _api; @@ -46,7 +46,7 @@ class SyncEngine { if (lastPing != null && lastPing.difference(now).abs() > minSyncGap) { _initTimer?.cancel(); - _initTimer = Timer(Duration(seconds: 2), _ping); + _initTimer = Timer(const Duration(seconds: 2), _ping); } } @@ -61,13 +61,14 @@ class SyncEngine { Future _ping() async { try { final response = await _api.ping(); + assert(!response.hasNewMessages); final preferences = await _sharedPreferences(); final now = clock.now(); await preferences.setInt(lastSuccessfulPingKey, now.millisecond); } catch (e, stack) { // TODO - print(e); - print(stack); + debugPrint(e.toString()); + debugPrint(stack.toString()); } } diff --git a/test/sync/sync_engine_test.dart b/test/sync/sync_engine_test.dart index e1d28f0b..e6ef787e 100644 --- a/test/sync/sync_engine_test.dart +++ b/test/sync/sync_engine_test.dart @@ -1,9 +1,9 @@ +import 'package:clock/clock.dart'; import 'package:fake_async/fake_async.dart'; import 'package:test/fake.dart'; import 'package:test/test.dart'; import 'package:wiredash/src/common/network/wiredash_api.dart'; import 'package:wiredash/src/sync/sync_engine.dart'; -import 'package:clock/clock.dart'; import '../feedback/data/pending_feedback_item_storage_test.dart'; import '../util/invocation_catcher.dart'; @@ -24,7 +24,7 @@ void main() { test('Never opened Wiredash does not trigger ping', () { fakeAsync((async) { syncEngine.onWiredashInitialized(); - async.elapse(Duration(seconds: 10)); + async.elapse(const Duration(seconds: 10)); expect(api.pingInvocations.count, 0); }); }); @@ -35,16 +35,16 @@ void main() { fakeAsync((async) { // Given last feedback 29 days ago syncEngine.rememberFeedbackSubmission(); - async.elapse(Duration(days: 29)); + async.elapse(const Duration(days: 29)); // Given last sync 2 hours ago prefs.setInt(SyncEngine.lastSuccessfulPingKey, clock.now().millisecond); - async.elapse(Duration(hours: 2)); + async.elapse(const Duration(hours: 2)); syncEngine.onWiredashInitialized(); - async.elapse(Duration(milliseconds: 1990)); + async.elapse(const Duration(milliseconds: 1990)); expect(api.pingInvocations.count, 0); - async.elapse(Duration(milliseconds: 100)); + async.elapse(const Duration(milliseconds: 100)); expect(api.pingInvocations.count, 1); }); }); @@ -53,7 +53,7 @@ void main() { fakeAsync((async) { expect(api.pingInvocations.count, 0); syncEngine.onUserOpenedWiredash(); - async.elapse(Duration(milliseconds: 1)); + async.elapse(const Duration(milliseconds: 1)); expect(api.pingInvocations.count, 1); }); }); From 15e51e9d84229b33da89a50cc4f50c946c9a7933 Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sun, 30 May 2021 02:53:14 +0200 Subject: [PATCH 03/20] Don't use clock in production code --- lib/src/sync/sync_engine.dart | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/src/sync/sync_engine.dart b/lib/src/sync/sync_engine.dart index 64cae0a1..ae2f2bf8 100644 --- a/lib/src/sync/sync_engine.dart +++ b/lib/src/sync/sync_engine.dart @@ -1,6 +1,5 @@ import 'dart:async'; -import 'package:clock/clock.dart'; import 'package:flutter/cupertino.dart'; import 'package:shared_preferences/shared_preferences.dart'; import 'package:wiredash/src/common/network/wiredash_api.dart'; @@ -37,7 +36,7 @@ class SyncEngine { return true; }()); - final now = clock.now(); + final now = DateTime.now(); final preferences = await _sharedPreferences(); final lastPingInt = preferences.getInt(lastSuccessfulPingKey); final lastPing = lastPingInt != null @@ -63,7 +62,7 @@ class SyncEngine { final response = await _api.ping(); assert(!response.hasNewMessages); final preferences = await _sharedPreferences(); - final now = clock.now(); + final now = DateTime.now(); await preferences.setInt(lastSuccessfulPingKey, now.millisecond); } catch (e, stack) { // TODO @@ -76,7 +75,7 @@ class SyncEngine { /// /// This information is used to trigger [_ping] on app start within [minSyncGap] periode Future rememberFeedbackSubmission() async { - final now = clock.now(); + final now = DateTime.now(); final preferences = await _sharedPreferences(); await preferences.setInt(lastFeedbackSubmissionKey, now.millisecond); } From f54b810eafcb0cbb5684169bf5acc8e0fd342a7d Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sun, 30 May 2021 12:48:34 +0200 Subject: [PATCH 04/20] Increase minSyncGap to 3h --- lib/src/sync/sync_engine.dart | 2 +- test/sync/sync_engine_test.dart | 30 ++++++++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/lib/src/sync/sync_engine.dart b/lib/src/sync/sync_engine.dart index ae2f2bf8..f5e042ec 100644 --- a/lib/src/sync/sync_engine.dart +++ b/lib/src/sync/sync_engine.dart @@ -19,7 +19,7 @@ class SyncEngine { Timer? _initTimer; - static const minSyncGap = Duration(hours: 1); + static const minSyncGap = Duration(hours: 3); static const lastSuccessfulPingKey = 'io.wiredash.last_successful_ping'; static const lastFeedbackSubmissionKey = diff --git a/test/sync/sync_engine_test.dart b/test/sync/sync_engine_test.dart index e6ef787e..dbb4158c 100644 --- a/test/sync/sync_engine_test.dart +++ b/test/sync/sync_engine_test.dart @@ -37,9 +37,9 @@ void main() { syncEngine.rememberFeedbackSubmission(); async.elapse(const Duration(days: 29)); - // Given last sync 2 hours ago + // Given last sync 6 hours ago prefs.setInt(SyncEngine.lastSuccessfulPingKey, clock.now().millisecond); - async.elapse(const Duration(hours: 2)); + async.elapse(const Duration(hours: 6)); syncEngine.onWiredashInitialized(); async.elapse(const Duration(milliseconds: 1990)); @@ -57,6 +57,32 @@ void main() { expect(api.pingInvocations.count, 1); }); }); + + test('opening the app twice within 3h gap does nothing', () { + fakeAsync((async) { + // Given last ping was almost 3h ago + prefs.setInt(SyncEngine.lastSuccessfulPingKey, clock.now().millisecond); + async.elapse(const Duration(hours: 2, minutes: 59)); + + expect(api.pingInvocations.count, 0); + syncEngine.onWiredashInitialized(); + async.elapse(const Duration(milliseconds: 1)); + expect(api.pingInvocations.count, 0); + }); + }); + + test('opening wiredash within 3h gap triggers ping', () { + fakeAsync((async) { + // Given last ping was almost 3h ago + prefs.setInt(SyncEngine.lastSuccessfulPingKey, clock.now().millisecond); + async.elapse(const Duration(hours: 2, minutes: 59)); + + expect(api.pingInvocations.count, 0); + syncEngine.onUserOpenedWiredash(); + async.elapse(const Duration(milliseconds: 1)); + expect(api.pingInvocations.count, 1); + }); + }); }); } From 9858f56d93191287b20b6a66ada9f4218fc3a969 Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Wed, 7 Jul 2021 00:20:41 +0200 Subject: [PATCH 05/20] Define KillSwitchException Improve mocking for better Future support --- lib/src/common/network/wiredash_api.dart | 22 ++++++++++++++-- lib/src/sync/sync_engine.dart | 11 +++++--- .../pending_feedback_item_storage_test.dart | 25 +++++++++++++------ .../retrying_feedback_submitter_test.dart | 4 +-- test/feedback/feedback_model_test.dart | 7 ++++-- test/sync/sync_engine_test.dart | 5 +++- test/util/invocation_catcher.dart | 25 +++++++++++++++++-- test/wiredash_widget_test.dart | 4 +-- 8 files changed, 80 insertions(+), 23 deletions(-) diff --git a/lib/src/common/network/wiredash_api.dart b/lib/src/common/network/wiredash_api.dart index 7a354152..ea59006e 100644 --- a/lib/src/common/network/wiredash_api.dart +++ b/lib/src/common/network/wiredash_api.dart @@ -62,6 +62,7 @@ class WiredashApi { Future ping() { throw "TODO implement"; + // TODO throw KillSwitchException(); } /// Sends a [BaseRequest] after attaching HTTP headers @@ -75,13 +76,30 @@ class WiredashApi { } class PingResponse { - bool hasNewMessages; + /// The id of the last message from dev or user + /// + /// Used to detecht if there are new messages + final String latestMessageId; PingResponse({ - required this.hasNewMessages, + required this.latestMessageId, }); } +/// Backend returns an error which silences the SDK (preventing automatic pings) +/// until [silentUntil] +class KillSwitchException extends WiredashApiException { + KillSwitchException(this.silentUntil, {Response? response}) + : super(response: response); + + final DateTime silentUntil; + + @override + String toString() { + return 'KillSwitchException{${response?.statusCode}, silentUntil: $silentUntil, body: ${response?.body}}'; + } +} + /// Generic error from the Wiredash API class WiredashApiException implements Exception { WiredashApiException({String? message, this.response}) : _message = message; diff --git a/lib/src/sync/sync_engine.dart b/lib/src/sync/sync_engine.dart index f5e042ec..0982932b 100644 --- a/lib/src/sync/sync_engine.dart +++ b/lib/src/sync/sync_engine.dart @@ -60,14 +60,17 @@ class SyncEngine { Future _ping() async { try { final response = await _api.ping(); - assert(!response.hasNewMessages); + // TODO save to prefs + assert(response.latestMessageId.isNotEmpty); + final preferences = await _sharedPreferences(); final now = DateTime.now(); await preferences.setInt(lastSuccessfulPingKey, now.millisecond); } catch (e, stack) { - // TODO - debugPrint(e.toString()); - debugPrint(stack.toString()); + // TODO track number of conseccutive errors to prevent pings at all + // debugPrint(e.toString()); + // debugPrint(stack.toString()); + assert(stack != null); } } diff --git a/test/feedback/data/pending_feedback_item_storage_test.dart b/test/feedback/data/pending_feedback_item_storage_test.dart index 1e2ded8d..ceef03a7 100644 --- a/test/feedback/data/pending_feedback_item_storage_test.dart +++ b/test/feedback/data/pending_feedback_item_storage_test.dart @@ -22,7 +22,11 @@ class FakeSharedPreferences extends Fake implements SharedPreferences { MethodInvocationCatcher('setStringList'); @override Future setStringList(String key, List value) async { - await setStringListInvocations.addMethodCall(args: [key, value]); + final mockedReturnValue = + setStringListInvocations.addAsyncMethodCall(args: [key, value]); + if (mockedReturnValue != null) { + return await mockedReturnValue.value as bool; + } _store[key] = value; return true; } @@ -31,9 +35,10 @@ class FakeSharedPreferences extends Fake implements SharedPreferences { MethodInvocationCatcher('getStringList'); @override List? getStringList(String key) { - final result = getStringListInvocations.addMethodCall(args: [key]); - if (result != null) { - return result as List?; + final mockedReturnValue = + getStringListInvocations.addMethodCall(args: [key]); + if (mockedReturnValue != null) { + return mockedReturnValue.value as List?; } return _store[key] as List?; } @@ -42,7 +47,11 @@ class FakeSharedPreferences extends Fake implements SharedPreferences { MethodInvocationCatcher('setInt'); @override Future setInt(String key, int value) async { - await setIntInvocations.addMethodCall(args: [key, value]); + final mockedReturnValue = + setIntInvocations.addMethodCall(args: [key, value]); + if (mockedReturnValue != null) { + return await mockedReturnValue.value as bool; + } _store[key] = value; return true; } @@ -51,9 +60,9 @@ class FakeSharedPreferences extends Fake implements SharedPreferences { MethodInvocationCatcher('getInt'); @override int? getInt(String key) { - final result = getIntInvocations.addMethodCall(args: [key]); - if (result != null) { - return result as int?; + final mockedReturnValue = getIntInvocations.addMethodCall(args: [key]); + if (mockedReturnValue != null) { + return mockedReturnValue.value as int?; } return _store[key] as int?; } diff --git a/test/feedback/data/retrying_feedback_submitter_test.dart b/test/feedback/data/retrying_feedback_submitter_test.dart index 00cb079f..8b5f6afc 100644 --- a/test/feedback/data/retrying_feedback_submitter_test.dart +++ b/test/feedback/data/retrying_feedback_submitter_test.dart @@ -26,8 +26,8 @@ class MockNetworkManager extends Fake implements WiredashApi { @override Future sendFeedback( {required FeedbackItem feedback, Uint8List? screenshot}) async { - await sendFeedbackInvocations.addMethodCall( - namedArgs: {'feedback': feedback, 'screenshot': screenshot}); + await sendFeedbackInvocations.addAsyncMethodCall( + namedArgs: {'feedback': feedback, 'screenshot': screenshot})?.value; } } diff --git a/test/feedback/feedback_model_test.dart b/test/feedback/feedback_model_test.dart index d8fb29c7..aed9bd18 100644 --- a/test/feedback/feedback_model_test.dart +++ b/test/feedback/feedback_model_test.dart @@ -36,8 +36,11 @@ class MockRetryingFeedbackSubmitter extends Fake MethodInvocationCatcher('submit'); @override Future submit(FeedbackItem item, Uint8List? screenshot) async { - submitInvocations - .addMethodCall(namedArgs: {'item': item, 'screenshot': screenshot}); + final mockedReturnValue = submitInvocations.addAsyncMethodCall( + namedArgs: {'item': item, 'screenshot': screenshot}); + if (mockedReturnValue != null) { + await mockedReturnValue.value; + } } } diff --git a/test/sync/sync_engine_test.dart b/test/sync/sync_engine_test.dart index dbb4158c..69ea8a42 100644 --- a/test/sync/sync_engine_test.dart +++ b/test/sync/sync_engine_test.dart @@ -92,7 +92,10 @@ class _MockWiredashApi extends Fake implements WiredashApi { @override Future ping() async { - await pingInvocations.addMethodCall(); + final mockedReturnValue = pingInvocations.addAsyncMethodCall(); + if (mockedReturnValue != null) { + return await mockedReturnValue.value as PingResponse; + } throw "Not implemented"; } } diff --git a/test/util/invocation_catcher.dart b/test/util/invocation_catcher.dart index 2f1ed161..d0746d98 100644 --- a/test/util/invocation_catcher.dart +++ b/test/util/invocation_catcher.dart @@ -22,7 +22,7 @@ class MethodInvocationCatcher { int get count => _invocations.length; - dynamic addMethodCall({ + MockedReturnValue? addMethodCall({ Map? namedArgs, List? args, }) { @@ -33,7 +33,23 @@ class MethodInvocationCatcher { ); _invocations.add(AssertableInvocation(iv)); if (interceptor != null) { - return interceptor!.call(iv); + return MockedReturnValue(interceptor!.call(iv)); + } + return null; + } + + MockedReturnValue>? addAsyncMethodCall({ + Map? namedArgs, + List? args, + }) { + final iv = Invocation.method( + Symbol(methodName), + args, + namedArgs?.map((key, value) => MapEntry(Symbol(key), value)), + ); + _invocations.add(AssertableInvocation(iv)); + if (interceptor != null) { + return MockedReturnValue(interceptor!.call(iv) as Future); } return null; } @@ -57,6 +73,11 @@ class MethodInvocationCatcher { } } +class MockedReturnValue { + MockedReturnValue(this.value); + final T value; +} + /// A invocation which can be used to assert specific values class AssertableInvocation { AssertableInvocation(this.original); diff --git a/test/wiredash_widget_test.dart b/test/wiredash_widget_test.dart index 3ddda440..b7d55c93 100644 --- a/test/wiredash_widget_test.dart +++ b/test/wiredash_widget_test.dart @@ -161,7 +161,7 @@ class _MockProjectCredentialValidator extends Fake @override Future validate( {required String projectId, required String secret}) async { - validateInvocations - .addMethodCall(namedArgs: {'projectId': projectId, 'secret': secret}); + await validateInvocations.addAsyncMethodCall( + namedArgs: {'projectId': projectId, 'secret': secret})?.value; } } From ae51915baf07d778c9d282b80e92873cebd515c0 Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Wed, 7 Jul 2021 01:43:37 +0200 Subject: [PATCH 06/20] Implement Kill Switch (silenceUntil) --- lib/src/sync/sync_engine.dart | 67 ++++++++++++++++--- test/sync/sync_engine_test.dart | 112 ++++++++++++++++++++++++++++---- 2 files changed, 160 insertions(+), 19 deletions(-) diff --git a/lib/src/sync/sync_engine.dart b/lib/src/sync/sync_engine.dart index 0982932b..aa683832 100644 --- a/lib/src/sync/sync_engine.dart +++ b/lib/src/sync/sync_engine.dart @@ -3,6 +3,9 @@ import 'dart:async'; import 'package:flutter/cupertino.dart'; import 'package:shared_preferences/shared_preferences.dart'; import 'package:wiredash/src/common/network/wiredash_api.dart'; +import 'package:clock/clock.dart'; + +const _debugPrint = false; class SyncEngine { final WiredashApi _api; @@ -24,6 +27,7 @@ class SyncEngine { static const lastSuccessfulPingKey = 'io.wiredash.last_successful_ping'; static const lastFeedbackSubmissionKey = 'io.wiredash.last_feedback_submission'; + static const silenceUntilKey = 'io.wiredash.silence_until'; /// Called when the SDK is initialized (by wrapping the app) /// @@ -36,23 +40,44 @@ class SyncEngine { return true; }()); - final now = DateTime.now(); + final now = clock.now(); final preferences = await _sharedPreferences(); final lastPingInt = preferences.getInt(lastSuccessfulPingKey); final lastPing = lastPingInt != null ? DateTime.fromMillisecondsSinceEpoch(lastPingInt) : null; - if (lastPing != null && lastPing.difference(now).abs() > minSyncGap) { - _initTimer?.cancel(); - _initTimer = Timer(const Duration(seconds: 2), _ping); + if (lastPing == null) { + // never opened wiredash, don't ping automatically on appstart + if (_debugPrint) debugPrint('Never opened wiredash, preventing ping'); + return; } + + if (now.difference(lastPing) <= minSyncGap) { + if (_debugPrint) { + debugPrint('Not syncing because within minSyncGapWindow\n' + 'now: $now lastPing:$lastPing\n' + 'diff (${now.difference(lastPing)}) <= minSyncGap ($minSyncGap)'); + } + // don't ping too often on appstart, only once every minSyncGap + return; + } + + if (await _isSilenced()) { + if (_debugPrint) debugPrint('Sdk silenced, preventing ping'); + // Received kill switch message, don't automatically ping + return; + } + + _initTimer?.cancel(); + _initTimer = Timer(const Duration(seconds: 2), _ping); } /// Called when the user manually opened Wiredash /// /// This 100% calls the backend, forcing a sync Future onUserOpenedWiredash() async { + // always ping on manual open, ignore silencing await _ping(); } @@ -64,21 +89,47 @@ class SyncEngine { assert(response.latestMessageId.isNotEmpty); final preferences = await _sharedPreferences(); - final now = DateTime.now(); + final now = clock.now(); await preferences.setInt(lastSuccessfulPingKey, now.millisecond); - } catch (e, stack) { + } on KillSwitchException catch (e) { + // sdk receives too much load, prevents further automatic pings + await _silenceUntil(e.silentUntil); + } catch (e, _) { // TODO track number of conseccutive errors to prevent pings at all // debugPrint(e.toString()); // debugPrint(stack.toString()); - assert(stack != null); } } + /// Silences the sdk, prevents automatic pings on app startup until the time is over + Future _silenceUntil(DateTime dateTime) async { + final preferences = await _sharedPreferences(); + preferences.setInt(silenceUntilKey, dateTime.millisecondsSinceEpoch); + debugPrint('Silenced Wiredash until $dateTime'); + } + + /// `true` when automatic pings should be prevented + Future _isSilenced() async { + final now = clock.now(); + final preferences = await _sharedPreferences(); + + final int? millis = preferences.getInt(silenceUntilKey); + if (millis == null) { + return false; + } + final silencedUntil = DateTime.fromMillisecondsSinceEpoch(millis); + final silenced = silencedUntil.isAfter(now); + if (_debugPrint && silenced) { + debugPrint("Sdk is silenced until $silencedUntil (now $now)"); + } + return silenced; + } + /// Remembers the time (now) when the last feedback was submitted /// /// This information is used to trigger [_ping] on app start within [minSyncGap] periode Future rememberFeedbackSubmission() async { - final now = DateTime.now(); + final now = clock.now(); final preferences = await _sharedPreferences(); await preferences.setInt(lastFeedbackSubmissionKey, now.millisecond); } diff --git a/test/sync/sync_engine_test.dart b/test/sync/sync_engine_test.dart index 69ea8a42..3b429080 100644 --- a/test/sync/sync_engine_test.dart +++ b/test/sync/sync_engine_test.dart @@ -12,17 +12,16 @@ void main() { group('Triggering ping', () { late _MockWiredashApi api; late FakeSharedPreferences prefs; - late SyncEngine syncEngine; setUp(() { api = _MockWiredashApi(); prefs = FakeSharedPreferences(); - syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); }); test('Never opened Wiredash does not trigger ping', () { fakeAsync((async) { + final syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); syncEngine.onWiredashInitialized(); async.elapse(const Duration(seconds: 10)); expect(api.pingInvocations.count, 0); @@ -33,12 +32,16 @@ void main() { 'appstart pings when the user submitted feedback/sent message ' 'in the last 30 days (delayed by 2 seconds)', () { fakeAsync((async) { + final syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); + // Given last feedback 29 days ago syncEngine.rememberFeedbackSubmission(); async.elapse(const Duration(days: 29)); // Given last sync 6 hours ago - prefs.setInt(SyncEngine.lastSuccessfulPingKey, clock.now().millisecond); + prefs.setInt(SyncEngine.lastSuccessfulPingKey, + clock.now().millisecondsSinceEpoch); async.elapse(const Duration(hours: 6)); syncEngine.onWiredashInitialized(); @@ -52,8 +55,10 @@ void main() { test('opening wiredash triggers ping immediately', () { fakeAsync((async) { expect(api.pingInvocations.count, 0); + final syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); syncEngine.onUserOpenedWiredash(); - async.elapse(const Duration(milliseconds: 1)); + async.flushTimers(); expect(api.pingInvocations.count, 1); }); }); @@ -61,12 +66,15 @@ void main() { test('opening the app twice within 3h gap does nothing', () { fakeAsync((async) { // Given last ping was almost 3h ago - prefs.setInt(SyncEngine.lastSuccessfulPingKey, clock.now().millisecond); + prefs.setInt(SyncEngine.lastSuccessfulPingKey, + clock.now().millisecondsSinceEpoch); async.elapse(const Duration(hours: 2, minutes: 59)); - expect(api.pingInvocations.count, 0); + + final syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); syncEngine.onWiredashInitialized(); - async.elapse(const Duration(milliseconds: 1)); + async.flushTimers(); expect(api.pingInvocations.count, 0); }); }); @@ -74,15 +82,97 @@ void main() { test('opening wiredash within 3h gap triggers ping', () { fakeAsync((async) { // Given last ping was almost 3h ago - prefs.setInt(SyncEngine.lastSuccessfulPingKey, clock.now().millisecond); + prefs.setInt(SyncEngine.lastSuccessfulPingKey, + clock.now().millisecondsSinceEpoch); async.elapse(const Duration(hours: 2, minutes: 59)); - expect(api.pingInvocations.count, 0); + + final syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); syncEngine.onUserOpenedWiredash(); - async.elapse(const Duration(milliseconds: 1)); + async.flushTimers(); expect(api.pingInvocations.count, 1); }); }); + + group('Kill Switch', () { + test('will silence ping on wiredash initialize', () { + // We really, really, really don't want million of wiredash users + // to kill our backend when something hits the fan + fakeAsync((async) { + // user opened app before + prefs.setInt(SyncEngine.lastSuccessfulPingKey, + clock.now().millisecondsSinceEpoch); + async.elapse(const Duration(days: 1)); + + // Silence SDK for two days + api.pingInvocations.interceptor = (_) async { + throw KillSwitchException(clock.now().add(const Duration(days: 2))); + }; + + var syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); + + // When SDK receives `silentUntil`, the sdk stops pinging automatically + syncEngine.onWiredashInitialized(); + async.flushTimers(); + expect(api.pingInvocations.count, 1); + + // doesn't ping within 2 day periode + async.elapse(const Duration(days: 1)); + syncEngine.dispose(); + syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); + syncEngine.onWiredashInitialized(); + async.flushTimers(); + expect(api.pingInvocations.count, 1); + + // When the silent duration is over (day 3) + // the sdk pings again on appstart + async.elapse(const Duration(days: 2)); + syncEngine.dispose(); + syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); + syncEngine.onWiredashInitialized(); + async.flushTimers(); + expect(api.pingInvocations.count, 2); + }); + }); + + test('Not silent when manually open wiredash', () { + fakeAsync((async) { + // user opened app before + prefs.setInt(SyncEngine.lastSuccessfulPingKey, + clock.now().millisecondsSinceEpoch); + async.elapse(const Duration(days: 1)); + + // Silence SDK for two days + api.pingInvocations.interceptor = (_) async { + throw KillSwitchException(clock.now().add(const Duration(days: 2))); + }; + + // When SDK receives `silentUntil`, the sdk stops pinging + var syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); + syncEngine.onWiredashInitialized(); + async.flushTimers(); + expect(api.pingInvocations.count, 1); + + // app start, silenced, no ping + syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); + syncEngine.onWiredashInitialized(); + async.flushTimers(); + expect(api.pingInvocations.count, 1); + + // manual open, pings + syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); + syncEngine.onUserOpenedWiredash(); + expect(api.pingInvocations.count, 2); + }); + }); + }); }); } From 8c306ff2dbaae4b6db9f99023a1125c9d8971555 Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Wed, 7 Jul 2021 15:17:27 +0200 Subject: [PATCH 07/20] Ignore avoid_web_libraries_in_flutter because it is a web only imported file --- lib/src/common/renderer/dart_html_renderer.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/src/common/renderer/dart_html_renderer.dart b/lib/src/common/renderer/dart_html_renderer.dart index 355e77a0..7b51034f 100644 --- a/lib/src/common/renderer/dart_html_renderer.dart +++ b/lib/src/common/renderer/dart_html_renderer.dart @@ -1,3 +1,4 @@ +// ignore: avoid_web_libraries_in_flutter import 'dart:js' as js; import 'package:wiredash/src/common/renderer/renderer.dart'; From 570cda061a03a4e59cfc91e9e5c5e990973099ff Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Wed, 7 Jul 2021 15:36:06 +0200 Subject: [PATCH 08/20] Test that the last successful ping date is saved --- test/sync/sync_engine_test.dart | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/sync/sync_engine_test.dart b/test/sync/sync_engine_test.dart index 3b429080..515f82dc 100644 --- a/test/sync/sync_engine_test.dart +++ b/test/sync/sync_engine_test.dart @@ -95,6 +95,19 @@ void main() { }); }); + test('last successful ping date is saved', () async { + // Silence SDK for two days + api.pingInvocations.interceptor = (_) async { + return PingResponse(latestMessageId: 'asdf'); + }; + + expect(prefs.getInt(SyncEngine.lastSuccessfulPingKey), isNull); + final syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); + await syncEngine.onUserOpenedWiredash(); + expect(prefs.getInt(SyncEngine.lastSuccessfulPingKey), isNotNull); + }); + group('Kill Switch', () { test('will silence ping on wiredash initialize', () { // We really, really, really don't want million of wiredash users From d8c21d093020d3fcdcfa2e75161f006ac7ae5e71 Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Wed, 7 Jul 2021 15:45:51 +0200 Subject: [PATCH 09/20] Fix lastSuccessfulPingKey to be saved correctly --- lib/src/sync/sync_engine.dart | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/src/sync/sync_engine.dart b/lib/src/sync/sync_engine.dart index aa683832..077d1f0c 100644 --- a/lib/src/sync/sync_engine.dart +++ b/lib/src/sync/sync_engine.dart @@ -90,7 +90,8 @@ class SyncEngine { final preferences = await _sharedPreferences(); final now = clock.now(); - await preferences.setInt(lastSuccessfulPingKey, now.millisecond); + await preferences.setInt( + lastSuccessfulPingKey, now.millisecondsSinceEpoch); } on KillSwitchException catch (e) { // sdk receives too much load, prevents further automatic pings await _silenceUntil(e.silentUntil); From 278a847a42087104bb5acb09190f9787c1586110 Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Wed, 7 Jul 2021 15:46:19 +0200 Subject: [PATCH 10/20] Test latest messageId to be saved --- lib/src/common/network/wiredash_api.dart | 5 +++- lib/src/sync/sync_engine.dart | 8 ++++--- .../pending_feedback_item_storage_test.dart | 24 +++++++++++++++++++ test/sync/sync_engine_test.dart | 13 ++++++++++ 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/lib/src/common/network/wiredash_api.dart b/lib/src/common/network/wiredash_api.dart index ea59006e..bb09812e 100644 --- a/lib/src/common/network/wiredash_api.dart +++ b/lib/src/common/network/wiredash_api.dart @@ -79,7 +79,10 @@ class PingResponse { /// The id of the last message from dev or user /// /// Used to detecht if there are new messages - final String latestMessageId; + /// + /// can be `null` when user never gave any feedback + // TODO double check with backend + final String? latestMessageId; PingResponse({ required this.latestMessageId, diff --git a/lib/src/sync/sync_engine.dart b/lib/src/sync/sync_engine.dart index 077d1f0c..f2ffd96d 100644 --- a/lib/src/sync/sync_engine.dart +++ b/lib/src/sync/sync_engine.dart @@ -85,10 +85,12 @@ class SyncEngine { Future _ping() async { try { final response = await _api.ping(); - // TODO save to prefs - assert(response.latestMessageId.isNotEmpty); - final preferences = await _sharedPreferences(); + final latestMessageId = response.latestMessageId; + if (latestMessageId != null) { + await preferences.setString(latestMessageIdKey, latestMessageId); + } + final now = clock.now(); await preferences.setInt( lastSuccessfulPingKey, now.millisecondsSinceEpoch); diff --git a/test/feedback/data/pending_feedback_item_storage_test.dart b/test/feedback/data/pending_feedback_item_storage_test.dart index ceef03a7..ec481419 100644 --- a/test/feedback/data/pending_feedback_item_storage_test.dart +++ b/test/feedback/data/pending_feedback_item_storage_test.dart @@ -66,6 +66,30 @@ class FakeSharedPreferences extends Fake implements SharedPreferences { } return _store[key] as int?; } + + final MethodInvocationCatcher setStringInvocations = + MethodInvocationCatcher('setString'); + @override + Future setString(String key, String value) async { + final mockedReturnValue = + setStringInvocations.addMethodCall(args: [key, value]); + if (mockedReturnValue != null) { + return await mockedReturnValue.value as bool; + } + _store[key] = value; + return true; + } + + final MethodInvocationCatcher getStringInvocations = + MethodInvocationCatcher('getString'); + @override + String? getString(String key) { + final mockedReturnValue = getStringInvocations.addMethodCall(args: [key]); + if (mockedReturnValue != null) { + return mockedReturnValue.value as String?; + } + return _store[key] as String?; + } } class IncrementalUuidV4Generator implements UuidV4Generator { diff --git a/test/sync/sync_engine_test.dart b/test/sync/sync_engine_test.dart index 515f82dc..ff96cc45 100644 --- a/test/sync/sync_engine_test.dart +++ b/test/sync/sync_engine_test.dart @@ -108,6 +108,19 @@ void main() { expect(prefs.getInt(SyncEngine.lastSuccessfulPingKey), isNotNull); }); + test('latest message id is saved', () async { + // Silence SDK for two days + api.pingInvocations.interceptor = (_) async { + return PingResponse(latestMessageId: 'asdf'); + }; + + expect(prefs.getString(SyncEngine.latestMessageIdKey), isNull); + final syncEngine = SyncEngine(api, () async => prefs); + addTearDown(() => syncEngine.dispose()); + await syncEngine.onUserOpenedWiredash(); + expect(prefs.getString(SyncEngine.latestMessageIdKey), 'asdf'); + }); + group('Kill Switch', () { test('will silence ping on wiredash initialize', () { // We really, really, really don't want million of wiredash users From 2ef9d83261fcd9600c83be04201bdbcede690d8b Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Wed, 7 Jul 2021 15:49:41 +0200 Subject: [PATCH 11/20] Add place for onNewMessage callback --- lib/src/sync/sync_engine.dart | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/src/sync/sync_engine.dart b/lib/src/sync/sync_engine.dart index f2ffd96d..44b3299a 100644 --- a/lib/src/sync/sync_engine.dart +++ b/lib/src/sync/sync_engine.dart @@ -88,7 +88,11 @@ class SyncEngine { final preferences = await _sharedPreferences(); final latestMessageId = response.latestMessageId; if (latestMessageId != null) { - await preferences.setString(latestMessageIdKey, latestMessageId); + final currentLatestId = preferences.getString(latestMessageIdKey); + if (currentLatestId != latestMessageId) { + await preferences.setString(latestMessageIdKey, latestMessageId); + // TODO call onNewMessage() callback + } } final now = clock.now(); From 170427b400e9857668613cd067903c4393aca7ca Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Thu, 8 Jul 2021 11:18:47 +0200 Subject: [PATCH 12/20] Add latestMessageIdKey sharedpreferences key --- lib/src/sync/sync_engine.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/src/sync/sync_engine.dart b/lib/src/sync/sync_engine.dart index 44b3299a..b6b6ebc3 100644 --- a/lib/src/sync/sync_engine.dart +++ b/lib/src/sync/sync_engine.dart @@ -28,6 +28,7 @@ class SyncEngine { static const lastFeedbackSubmissionKey = 'io.wiredash.last_feedback_submission'; static const silenceUntilKey = 'io.wiredash.silence_until'; + static const latestMessageIdKey = 'io.wiredash.latest_message_id'; /// Called when the SDK is initialized (by wrapping the app) /// From eb4a5bc792721a2c7a871aa4deb0736b619f9931 Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sun, 11 Jul 2021 19:17:00 +0200 Subject: [PATCH 13/20] Fix import --- lib/src/sync/sync_engine.dart | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/src/sync/sync_engine.dart b/lib/src/sync/sync_engine.dart index b6b6ebc3..9b265b30 100644 --- a/lib/src/sync/sync_engine.dart +++ b/lib/src/sync/sync_engine.dart @@ -1,9 +1,9 @@ import 'dart:async'; +import 'package:clock/clock.dart'; import 'package:flutter/cupertino.dart'; import 'package:shared_preferences/shared_preferences.dart'; import 'package:wiredash/src/common/network/wiredash_api.dart'; -import 'package:clock/clock.dart'; const _debugPrint = false; @@ -36,7 +36,7 @@ class SyncEngine { Future onWiredashInitialized() async { assert(() { if (_initTimer != null) { - debugPrint("called onWiredashInitialized multiple times"); + debugPrint("Warning: called onWiredashInitialized multiple times"); } return true; }()); @@ -102,10 +102,10 @@ class SyncEngine { } on KillSwitchException catch (e) { // sdk receives too much load, prevents further automatic pings await _silenceUntil(e.silentUntil); - } catch (e, _) { + } catch (e, stack) { // TODO track number of conseccutive errors to prevent pings at all - // debugPrint(e.toString()); - // debugPrint(stack.toString()); + debugPrint(e.toString()); + debugPrint(stack.toString()); } } From f314b541e7f63de34928d4e3cb1e3785de08180f Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sat, 2 Jul 2022 14:31:56 +0200 Subject: [PATCH 14/20] Use Job to queue pings --- lib/src/core/network/wiredash_api.dart | 76 ++--- lib/src/core/services/services.dart | 8 + lib/src/core/sync/ping_job.dart | 100 ++++++ lib/src/core/sync/sync_engine.dart | 175 +++++------ lib/src/core/sync/sync_feedback_job.dart | 30 ++ lib/src/core/wiredash_widget.dart | 19 +- test/sync/sync_engine_test.dart | 385 ++++++++++++----------- 7 files changed, 450 insertions(+), 343 deletions(-) create mode 100644 lib/src/core/sync/ping_job.dart create mode 100644 lib/src/core/sync/sync_feedback_job.dart diff --git a/lib/src/core/network/wiredash_api.dart b/lib/src/core/network/wiredash_api.dart index ef2e01e4..a4089183 100644 --- a/lib/src/core/network/wiredash_api.dart +++ b/lib/src/core/network/wiredash_api.dart @@ -64,19 +64,11 @@ class WiredashApi { final response = await _send(req); - if (response.statusCode == 401) { - throw UnauthenticatedWiredashApiException(response, _projectId, _secret); - } - - if (response.statusCode != 200) { - throw WiredashApiException( - message: '$type upload failed', - response: response, - ); + if (response.statusCode == 200) { + final map = jsonDecode(response.body) as Map; + return AttachmentId(map['id'] as String); } - - final map = jsonDecode(response.body) as Map; - return AttachmentId(map['id'] as String); + _parseResponseForErrors(response); } /// Reports a feedback @@ -97,13 +89,7 @@ class WiredashApi { // success 🎉 return; } - if (response.statusCode == 401) { - throw UnauthenticatedWiredashApiException(response, _projectId, _secret); - } - throw WiredashApiException( - message: 'submitting feedback failed', - response: response, - ); + _parseResponseForErrors(response); } Future sendNps(NpsRequestBody body) async { @@ -119,18 +105,17 @@ class WiredashApi { // success 🎉 return; } - if (response.statusCode == 401) { - throw UnauthenticatedWiredashApiException(response, _projectId, _secret); - } - throw WiredashApiException( - message: 'submitting nps failed', - response: response, - ); + _parseResponseForErrors(response); } Future ping() async { - throw "TODO implement"; - // TODO throw KillSwitchException(); + final uri = Uri.parse('$_host/ping'); + final Request request = Request('POST', uri); + final response = await _send(request); + if (response.statusCode == 200) { + return PingResponse(); + } + _parseResponseForErrors(response); } /// Sends a [BaseRequest] after attaching HTTP headers @@ -143,6 +128,16 @@ class WiredashApi { final streamedResponse = await _httpClient.send(request); return Response.fromStream(streamedResponse); } + + Never _parseResponseForErrors(Response response) { + if (response.statusCode == 401) { + throw UnauthenticatedWiredashApiException(response, _projectId, _secret); + } + if (response.statusCode == 403) { + throw KillSwitchException(response: response); + } + throw WiredashApiException(response: response); + } } extension UploadScreenshotApi on WiredashApi { @@ -194,6 +189,7 @@ class WiredashApiException implements Exception { String toString() { return 'WiredashApiException{' '"$message", ' + 'endpoint: ${response?.request?.url.path}, ' 'code: ${response?.statusCode}, ' 'resp: $messageFromServer' '}'; @@ -535,29 +531,15 @@ class NpsRequestBody { } class PingResponse { - /// The id of the last message from dev or user - /// - /// Used to detect if there are new messages - /// - /// can be `null` when user never gave any feedback - // TODO double check with backend - final String? latestMessageId; - - PingResponse({ - required this.latestMessageId, - }); + // Nothing in here just yet but that will change in the future + PingResponse(); } -/// Backend returns an error which silences the SDK (preventing automatic pings) -/// until [silentUntil] +/// Backend returns an error which silences the SDK for one week class KillSwitchException extends WiredashApiException { - KillSwitchException(this.silentUntil, {Response? response}) - : super(response: response); - - final DateTime silentUntil; - + KillSwitchException({Response? response}) : super(response: response); @override String toString() { - return 'KillSwitchException{${response?.statusCode}, silentUntil: $silentUntil, body: ${response?.body}}'; + return 'KillSwitchException{${response?.statusCode}, body: ${response?.body}}'; } } diff --git a/lib/src/core/services/services.dart b/lib/src/core/services/services.dart index 2318c343..1670e337 100644 --- a/lib/src/core/services/services.dart +++ b/lib/src/core/services/services.dart @@ -10,6 +10,7 @@ import 'package:shared_preferences/shared_preferences.dart'; import 'package:wiredash/src/core/network/wiredash_api.dart'; import 'package:wiredash/src/core/options/wiredash_options_data.dart'; import 'package:wiredash/src/core/services/streampod.dart'; +import 'package:wiredash/src/core/sync/sync_engine.dart'; import 'package:wiredash/src/core/widgets/backdrop/wiredash_backdrop.dart'; import 'package:wiredash/src/core/wiredash_model.dart'; import 'package:wiredash/src/core/wiredash_widget.dart'; @@ -60,6 +61,8 @@ class WiredashServices extends ChangeNotifier { WiredashApi get api => _locator.get(); + SyncEngine get syncEngine => _locator.get(); + DiscardFeedbackUseCase get discardFeedback => _locator.get(); DiscardNpsUseCase get discardNps => _locator.get(); @@ -166,6 +169,11 @@ void _setupServices(WiredashServices sl) { }, ); + sl.inject( + (locator) => SyncEngine(), + dispose: (engine) => engine.onWiredashDispose(), + ); + sl.inject((_) => DiscardFeedbackUseCase(sl)); sl.inject((_) => DiscardNpsUseCase(sl)); } diff --git a/lib/src/core/sync/ping_job.dart b/lib/src/core/sync/ping_job.dart new file mode 100644 index 00000000..40b69860 --- /dev/null +++ b/lib/src/core/sync/ping_job.dart @@ -0,0 +1,100 @@ +import 'package:clock/clock.dart'; +import 'package:shared_preferences/shared_preferences.dart'; +import 'package:wiredash/src/core/network/wiredash_api.dart'; +import 'package:wiredash/src/core/sync/sync_engine.dart'; + +class PingJob extends Job { + final WiredashApi api; + final Future Function() sharedPreferencesProvider; + + PingJob({ + required this.api, + required this.sharedPreferencesProvider, + }); + + static const lastSuccessfulPingKey = 'io.wiredash.last_successful_ping'; + static const silenceUntilKey = 'io.wiredash.silence_ping_until'; + + static const minPingGap = Duration(hours: 3); + static const killSwitchSilenceDuration = Duration(days: 7); + static const pingErrorSilenceDuration = Duration(minutes: 15); + + @override + bool shouldExecute(SdkEvent event) { + return event == SdkEvent.appStart; + } + + @override + Future execute() async { + final now = clock.now(); + + if (await _isSilenced(now)) { + // Received kill switch message, don't ping at all + syncDebugPrint('Sdk silenced, preventing ping'); + return; + } + + final lastPing = await _getLastSuccessfulPing(); + if (lastPing != null && now.difference(lastPing) <= minPingGap) { + syncDebugPrint('Not syncing because within minSyncGapWindow\n' + 'now: $now lastPing:$lastPing\n' + 'diff (${now.difference(lastPing)}) <= minSyncGap ($minPingGap)'); + // don't ping too often on app start, only once every minPingGap + return; + } + + try { + await api.ping(); + await _saveLastSuccessfulPing(now); + syncDebugPrint('ping'); + } on KillSwitchException catch (_) { + // Server explicitly asks the SDK to be silent + final earliestNextPing = now.add(killSwitchSilenceDuration); + await _silenceUntil(earliestNextPing); + syncDebugPrint('Silenced Wiredash until $earliestNextPing'); + } catch (e, stack) { + // Any other error, like network errors or server errors, silence the ping + // for a while, too + syncDebugPrint('Received an unknown error for ping'); + syncDebugPrint(e); + syncDebugPrint(stack); + } + } + + /// Silences the sdk, prevents automatic pings on app startup until the time is over + Future _silenceUntil(DateTime dateTime) async { + final preferences = await sharedPreferencesProvider(); + preferences.setInt(silenceUntilKey, dateTime.millisecondsSinceEpoch); + syncDebugPrint('Silenced Wiredash until $dateTime'); + } + + /// `true` when automatic pings should be prevented + Future _isSilenced(DateTime now) async { + final preferences = await sharedPreferencesProvider(); + + final int? millis = preferences.getInt(silenceUntilKey); + if (millis == null) { + return false; + } + final silencedUntil = DateTime.fromMillisecondsSinceEpoch(millis); + final silenced = silencedUntil.isAfter(now); + if (silenced) { + syncDebugPrint("Sdk is silenced until $silencedUntil (now $now)"); + } + return silenced; + } + + Future _getLastSuccessfulPing() async { + final preferences = await sharedPreferencesProvider(); + final lastPingInt = preferences.getInt(lastSuccessfulPingKey); + if (lastPingInt == null) { + return null; + } + return DateTime.fromMillisecondsSinceEpoch(lastPingInt); + } + + Future _saveLastSuccessfulPing(DateTime now) async { + final preferences = await sharedPreferencesProvider(); + await preferences.setInt(lastSuccessfulPingKey, now.millisecondsSinceEpoch); + } +} diff --git a/lib/src/core/sync/sync_engine.dart b/lib/src/core/sync/sync_engine.dart index a843402e..145f1b38 100644 --- a/lib/src/core/sync/sync_engine.dart +++ b/lib/src/core/sync/sync_engine.dart @@ -1,25 +1,25 @@ import 'dart:async'; -import 'package:clock/clock.dart'; import 'package:flutter/cupertino.dart'; -import 'package:shared_preferences/shared_preferences.dart'; -import 'package:wiredash/src/_wiredash_internal.dart'; -const _debugPrint = false; +const _kSyncDebugPrint = true; -class SyncEngine { - final WiredashApi _api; - final Future Function() _sharedPreferences; +void syncDebugPrint(Object? message) { + if (_kSyncDebugPrint) { + debugPrint(message?.toString()); + } +} - SyncEngine( - WiredashApi api, - Future Function() sharedPreferences, - ) : _api = api, - _sharedPreferences = sharedPreferences; +enum SdkEvent { + appStart, + openedWiredash, + submittedFeedback, + submittedNps, +} - void dispose() { - _initTimer?.cancel(); - } +/// Executes sync jobs with the network at certain times +class SyncEngine { + SyncEngine(); Timer? _initTimer; @@ -31,10 +31,27 @@ class SyncEngine { static const silenceUntilKey = 'io.wiredash.silence_until'; static const latestMessageIdKey = 'io.wiredash.latest_message_id'; + final Map _jobs = {}; + + bool get _mounted => _initTimer != null; + + /// Adds a job to be executed at the right time + void addJob( + String name, + Job job, + ) { + if (job._name != null) { + throw 'Job already has a name (${job._name}), cannot add it ($name) twice'; + } + job._name = name; + _jobs[name] = job; + syncDebugPrint('Added job $name (${job.runtimeType})'); + } + /// Called when the SDK is initialized (by wrapping the app) /// - /// This eventually syncs with the backend - Future onWiredashInitialized() async { + /// Triggers [SdkEvent.appStart] after the app settled down. + Future onWiredashInit() async { assert(() { if (_initTimer != null) { debugPrint("Warning: called onWiredashInitialized multiple times"); @@ -42,106 +59,60 @@ class SyncEngine { return true; }()); - final now = clock.now(); - final preferences = await _sharedPreferences(); - final lastPingInt = preferences.getInt(lastSuccessfulPingKey); - final lastPing = lastPingInt != null - ? DateTime.fromMillisecondsSinceEpoch(lastPingInt) - : null; - - if (lastPing == null) { - // never opened wiredash, don't ping automatically on appstart - if (_debugPrint) debugPrint('Never opened wiredash, preventing ping'); - return; - } - - if (now.difference(lastPing) <= minSyncGap) { - if (_debugPrint) { - debugPrint('Not syncing because within minSyncGapWindow\n' - 'now: $now lastPing:$lastPing\n' - 'diff (${now.difference(lastPing)}) <= minSyncGap ($minSyncGap)'); - } - // don't ping too often on appstart, only once every minSyncGap - return; - } - - if (await _isSilenced()) { - if (_debugPrint) debugPrint('Sdk silenced, preventing ping'); - // Received kill switch message, don't automatically ping - return; - } + // Delay app start a bit, so that Wiredash doesn't slow down the app start + _initTimer?.cancel(); + _initTimer = Timer(const Duration(seconds: 5), () { + _triggerEvent(SdkEvent.appStart); + }); + } + /// Shuts down the sync engine because wiredash is not part of the widget tree + /// anymore + void onWiredashDispose() { _initTimer?.cancel(); - _initTimer = Timer(const Duration(seconds: 2), _ping); + _initTimer = null; } /// Called when the user manually opened Wiredash /// /// This 100% calls the backend, forcing a sync Future onUserOpenedWiredash() async { - // always ping on manual open, ignore silencing - await _ping(); + await _triggerEvent(SdkEvent.appStart); } - /// Pings the backend with a very cheep call checking if anything should be synced - Future _ping() async { - try { - final response = await _api.ping(); - final preferences = await _sharedPreferences(); - final latestMessageId = response.latestMessageId; - if (latestMessageId != null) { - final currentLatestId = preferences.getString(latestMessageIdKey); - if (currentLatestId != latestMessageId) { - await preferences.setString(latestMessageIdKey, latestMessageId); - // TODO call onNewMessage() callback - } - } - - final now = clock.now(); - await preferences.setInt( - lastSuccessfulPingKey, - now.millisecondsSinceEpoch, - ); - } on KillSwitchException catch (e) { - // sdk receives too much load, prevents further automatic pings - await _silenceUntil(e.silentUntil); - } catch (e, stack) { - // TODO track number of consecutive errors to prevent pings at all - debugPrint(e.toString()); - debugPrint(stack.toString()); - } + Future onSubmitFeedback() async { + await _triggerEvent(SdkEvent.submittedFeedback); } - /// Silences the sdk, prevents automatic pings on app startup until the time is over - Future _silenceUntil(DateTime dateTime) async { - final preferences = await _sharedPreferences(); - preferences.setInt(silenceUntilKey, dateTime.millisecondsSinceEpoch); - debugPrint('Silenced Wiredash until $dateTime'); + Future onSubmitNPS() async { + await _triggerEvent(SdkEvent.submittedNps); } - /// `true` when automatic pings should be prevented - Future _isSilenced() async { - final now = clock.now(); - final preferences = await _sharedPreferences(); - - final int? millis = preferences.getInt(silenceUntilKey); - if (millis == null) { - return false; - } - final silencedUntil = DateTime.fromMillisecondsSinceEpoch(millis); - final silenced = silencedUntil.isAfter(now); - if (_debugPrint && silenced) { - debugPrint("Sdk is silenced until $silencedUntil (now $now)"); + /// Executes all jobs that are listening to the given event + Future _triggerEvent(SdkEvent event) async { + for (final job in _jobs.values) { + if (!_mounted) { + // stop sync operation, Wiredash was removed from the widget tree + syncDebugPrint('cancelling job execution for event $event'); + break; + } + try { + if (job.shouldExecute(event)) { + syncDebugPrint('Executing job ${job._name}'); + await job.execute(); + } + } catch (e, stack) { + debugPrint('Error executing job ${job._name}:\n$e\n$stack'); + } } - return silenced; } +} - /// Remembers the time (now) when the last feedback was submitted - /// - /// This information is used to trigger [_ping] on app start within [minSyncGap] periode - Future rememberFeedbackSubmission() async { - final now = clock.now(); - final preferences = await _sharedPreferences(); - await preferences.setInt(lastFeedbackSubmissionKey, now.millisecond); - } +abstract class Job { + String get name => _name ?? 'unnamed'; + String? _name; + + bool shouldExecute(SdkEvent event); + + Future execute(); } diff --git a/lib/src/core/sync/sync_feedback_job.dart b/lib/src/core/sync/sync_feedback_job.dart new file mode 100644 index 00000000..c762b2aa --- /dev/null +++ b/lib/src/core/sync/sync_feedback_job.dart @@ -0,0 +1,30 @@ +import 'package:flutter/foundation.dart'; +import 'package:wiredash/src/core/sync/sync_engine.dart'; +import 'package:wiredash/src/feedback/_feedback.dart'; + +class UploadPendingFeedbackJob extends Job { + final FeedbackSubmitter feedbackSubmitter; + + UploadPendingFeedbackJob({ + required this.feedbackSubmitter, + }); + + @override + bool shouldExecute(SdkEvent event) { + return [SdkEvent.appStart].contains(event); + } + + @override + Future execute() async { + if (feedbackSubmitter is! RetryingFeedbackSubmitter) { + return; + } + + final submitter = feedbackSubmitter as RetryingFeedbackSubmitter; + await submitter.submitPendingFeedbackItems(); + + if (kDebugMode) { + await submitter.deletePendingFeedbacks(); + } + } +} diff --git a/lib/src/core/wiredash_widget.dart b/lib/src/core/wiredash_widget.dart index d1bea2d4..989b7f21 100644 --- a/lib/src/core/wiredash_widget.dart +++ b/lib/src/core/wiredash_widget.dart @@ -4,6 +4,7 @@ import 'dart:ui' as ui; import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; +import 'package:shared_preferences/shared_preferences.dart'; import 'package:wiredash/src/_wiredash_internal.dart'; import 'package:wiredash/src/_wiredash_ui.dart'; import 'package:wiredash/src/core/context_cache.dart'; @@ -11,6 +12,8 @@ import 'package:wiredash/src/core/options/wiredash_options.dart'; import 'package:wiredash/src/core/project_credential_validator.dart'; import 'package:wiredash/src/core/support/back_button_interceptor.dart'; import 'package:wiredash/src/core/support/not_a_widgets_app.dart'; +import 'package:wiredash/src/core/sync/ping_job.dart'; +import 'package:wiredash/src/core/sync/sync_feedback_job.dart'; import 'package:wiredash/src/feedback/_feedback.dart'; import 'package:wiredash/src/feedback/feedback_backdrop.dart'; import 'package:wiredash/src/nps/nps_backdrop.dart'; @@ -185,8 +188,19 @@ class WiredashState extends State { _services.wiredashModel.addListener(_markNeedsBuild); _services.backdropController.addListener(_markNeedsBuild); - _submitTimer = - Timer(const Duration(seconds: 5), scheduleFeedbackSubmission); + _services.syncEngine.addJob( + 'ping', + PingJob( + api: _services.api, + sharedPreferencesProvider: SharedPreferences.getInstance), + ); + _services.syncEngine.addJob( + 'feedback', + UploadPendingFeedbackJob(feedbackSubmitter: _services.feedbackSubmitter), + ); + + _services.syncEngine.onWiredashInit(); + _backButtonDispatcher = WiredashBackButtonDispatcher()..initialize(); } @@ -214,6 +228,7 @@ class WiredashState extends State { _submitTimer = null; _services.dispose(); _backButtonDispatcher.dispose(); + _services.syncEngine.onWiredashDispose(); super.dispose(); } diff --git a/test/sync/sync_engine_test.dart b/test/sync/sync_engine_test.dart index 08dc67be..cd5c7cde 100644 --- a/test/sync/sync_engine_test.dart +++ b/test/sync/sync_engine_test.dart @@ -17,198 +17,199 @@ void main() { api = _MockWiredashApi(); prefs = InMemorySharedPreferences(); }); - - test('Never opened Wiredash does not trigger ping', () { - fakeAsync((async) { - final syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - syncEngine.onWiredashInitialized(); - async.elapse(const Duration(seconds: 10)); - expect(api.pingInvocations.count, 0); - }); - }); - - test( - 'appstart pings when the user submitted feedback/sent message ' - 'in the last 30 days (delayed by 2 seconds)', () { - fakeAsync((async) { - final syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - - // Given last feedback 29 days ago - syncEngine.rememberFeedbackSubmission(); - async.elapse(const Duration(days: 29)); - - // Given last sync 6 hours ago - prefs.setInt( - SyncEngine.lastSuccessfulPingKey, - clock.now().millisecondsSinceEpoch, - ); - async.elapse(const Duration(hours: 6)); - - syncEngine.onWiredashInitialized(); - async.elapse(const Duration(milliseconds: 1990)); - expect(api.pingInvocations.count, 0); - async.elapse(const Duration(milliseconds: 100)); - expect(api.pingInvocations.count, 1); - }); - }); - - test('opening wiredash triggers ping immediately', () { - fakeAsync((async) { - expect(api.pingInvocations.count, 0); - final syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - syncEngine.onUserOpenedWiredash(); - async.flushTimers(); - expect(api.pingInvocations.count, 1); - }); - }); - - test('opening the app twice within 3h gap does nothing', () { - fakeAsync((async) { - // Given last ping was almost 3h ago - prefs.setInt( - SyncEngine.lastSuccessfulPingKey, - clock.now().millisecondsSinceEpoch, - ); - async.elapse(const Duration(hours: 2, minutes: 59)); - expect(api.pingInvocations.count, 0); - - final syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - syncEngine.onWiredashInitialized(); - async.flushTimers(); - expect(api.pingInvocations.count, 0); - }); - }); - - test('opening wiredash within 3h gap triggers ping', () { - fakeAsync((async) { - // Given last ping was almost 3h ago - prefs.setInt( - SyncEngine.lastSuccessfulPingKey, - clock.now().millisecondsSinceEpoch, - ); - async.elapse(const Duration(hours: 2, minutes: 59)); - expect(api.pingInvocations.count, 0); - - final syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - syncEngine.onUserOpenedWiredash(); - async.flushTimers(); - expect(api.pingInvocations.count, 1); - }); - }); - - test('last successful ping date is saved', () async { - // Silence SDK for two days - api.pingInvocations.interceptor = (_) async { - return PingResponse(latestMessageId: 'asdf'); - }; - - expect(prefs.getInt(SyncEngine.lastSuccessfulPingKey), isNull); - final syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - await syncEngine.onUserOpenedWiredash(); - expect(prefs.getInt(SyncEngine.lastSuccessfulPingKey), isNotNull); - }); - - test('latest message id is saved', () async { - // Silence SDK for two days - api.pingInvocations.interceptor = (_) async { - return PingResponse(latestMessageId: 'asdf'); - }; - - expect(prefs.getString(SyncEngine.latestMessageIdKey), isNull); - final syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - await syncEngine.onUserOpenedWiredash(); - expect(prefs.getString(SyncEngine.latestMessageIdKey), 'asdf'); - }); - - group('Kill Switch', () { - test('will silence ping on wiredash initialize', () { - // We really, really, really don't want million of wiredash users - // to kill our backend when something hits the fan - fakeAsync((async) { - // user opened app before - prefs.setInt( - SyncEngine.lastSuccessfulPingKey, - clock.now().millisecondsSinceEpoch, - ); - async.elapse(const Duration(days: 1)); - - // Silence SDK for two days - api.pingInvocations.interceptor = (_) async { - throw KillSwitchException(clock.now().add(const Duration(days: 2))); - }; - - var syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - - // When SDK receives `silentUntil`, the sdk stops pinging automatically - syncEngine.onWiredashInitialized(); - async.flushTimers(); - expect(api.pingInvocations.count, 1); - - // doesn't ping within 2 day periode - async.elapse(const Duration(days: 1)); - syncEngine.dispose(); - syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - syncEngine.onWiredashInitialized(); - async.flushTimers(); - expect(api.pingInvocations.count, 1); - - // When the silent duration is over (day 3) - // the sdk pings again on appstart - async.elapse(const Duration(days: 2)); - syncEngine.dispose(); - syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - syncEngine.onWiredashInitialized(); - async.flushTimers(); - expect(api.pingInvocations.count, 2); - }); - }); - - test('Not silent when manually open wiredash', () { - fakeAsync((async) { - // user opened app before - prefs.setInt( - SyncEngine.lastSuccessfulPingKey, - clock.now().millisecondsSinceEpoch, - ); - async.elapse(const Duration(days: 1)); - - // Silence SDK for two days - api.pingInvocations.interceptor = (_) async { - throw KillSwitchException(clock.now().add(const Duration(days: 2))); - }; - - // When SDK receives `silentUntil`, the sdk stops pinging - var syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - syncEngine.onWiredashInitialized(); - async.flushTimers(); - expect(api.pingInvocations.count, 1); - - // app start, silenced, no ping - syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - syncEngine.onWiredashInitialized(); - async.flushTimers(); - expect(api.pingInvocations.count, 1); - - // manual open, pings - syncEngine = SyncEngine(api, () async => prefs); - addTearDown(() => syncEngine.dispose()); - syncEngine.onUserOpenedWiredash(); - expect(api.pingInvocations.count, 2); - }); - }); - }); + // + // // TODO not relevant anymore, we always want to ping + // test('Never opened Wiredash does not trigger ping', () { + // fakeAsync((async) { + // final syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // syncEngine.onWiredashInit(); + // async.elapse(const Duration(seconds: 10)); + // expect(api.pingInvocations.count, 0); + // }); + // }); + // + // test( + // 'appstart pings when the user submitted feedback/sent message ' + // 'in the last 30 days (delayed by 2 seconds)', () { + // fakeAsync((async) { + // final syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // + // // Given last feedback 29 days ago + // syncEngine.onSubmitFeedback(); + // async.elapse(const Duration(days: 29)); + // + // // Given last sync 6 hours ago + // prefs.setInt( + // SyncEngine.lastSuccessfulPingKey, + // clock.now().millisecondsSinceEpoch, + // ); + // async.elapse(const Duration(hours: 6)); + // + // syncEngine.onWiredashInit(); + // async.elapse(const Duration(milliseconds: 1990)); + // expect(api.pingInvocations.count, 0); + // async.elapse(const Duration(milliseconds: 100)); + // expect(api.pingInvocations.count, 1); + // }); + // }); + // + // test('opening wiredash triggers ping immediately', () { + // fakeAsync((async) { + // expect(api.pingInvocations.count, 0); + // final syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // syncEngine.onUserOpenedWiredash(); + // async.flushTimers(); + // expect(api.pingInvocations.count, 1); + // }); + // }); + // + // test('opening the app twice within 3h gap does nothing', () { + // fakeAsync((async) { + // // Given last ping was almost 3h ago + // prefs.setInt( + // SyncEngine.lastSuccessfulPingKey, + // clock.now().millisecondsSinceEpoch, + // ); + // async.elapse(const Duration(hours: 2, minutes: 59)); + // expect(api.pingInvocations.count, 0); + // + // final syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // syncEngine.onWiredashInit(); + // async.flushTimers(); + // expect(api.pingInvocations.count, 0); + // }); + // }); + // + // test('opening wiredash within 3h gap triggers ping', () { + // fakeAsync((async) { + // // Given last ping was almost 3h ago + // prefs.setInt( + // SyncEngine.lastSuccessfulPingKey, + // clock.now().millisecondsSinceEpoch, + // ); + // async.elapse(const Duration(hours: 2, minutes: 59)); + // expect(api.pingInvocations.count, 0); + // + // final syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // syncEngine.onUserOpenedWiredash(); + // async.flushTimers(); + // expect(api.pingInvocations.count, 1); + // }); + // }); + // + // test('last successful ping date is saved', () async { + // // Silence SDK for two days + // api.pingInvocations.interceptor = (_) async { + // return PingResponse(latestMessageId: 'asdf'); + // }; + // + // expect(prefs.getInt(SyncEngine.lastSuccessfulPingKey), isNull); + // final syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // await syncEngine.onUserOpenedWiredash(); + // expect(prefs.getInt(SyncEngine.lastSuccessfulPingKey), isNotNull); + // }); + // + // test('latest message id is saved', () async { + // // Silence SDK for two days + // api.pingInvocations.interceptor = (_) async { + // return PingResponse(latestMessageId: 'asdf'); + // }; + // + // expect(prefs.getString(SyncEngine.latestMessageIdKey), isNull); + // final syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // await syncEngine.onUserOpenedWiredash(); + // expect(prefs.getString(SyncEngine.latestMessageIdKey), 'asdf'); + // }); + // + // group('Kill Switch', () { + // test('will silence ping on wiredash initialize', () { + // // We really, really, really don't want million of wiredash users + // // to kill our backend when something hits the fan + // fakeAsync((async) { + // // user opened app before + // prefs.setInt( + // SyncEngine.lastSuccessfulPingKey, + // clock.now().millisecondsSinceEpoch, + // ); + // async.elapse(const Duration(days: 1)); + // + // // Silence SDK for two days + // api.pingInvocations.interceptor = (_) async { + // throw KillSwitchException(clock.now().add(const Duration(days: 2))); + // }; + // + // var syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // + // // When SDK receives `silentUntil`, the sdk stops pinging automatically + // syncEngine.onWiredashInit(); + // async.flushTimers(); + // expect(api.pingInvocations.count, 1); + // + // // doesn't ping within 2 day periode + // async.elapse(const Duration(days: 1)); + // syncEngine.dispose(); + // syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // syncEngine.onWiredashInit(); + // async.flushTimers(); + // expect(api.pingInvocations.count, 1); + // + // // When the silent duration is over (day 3) + // // the sdk pings again on appstart + // async.elapse(const Duration(days: 2)); + // syncEngine.dispose(); + // syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // syncEngine.onWiredashInit(); + // async.flushTimers(); + // expect(api.pingInvocations.count, 2); + // }); + // }); + // + // test('Not silent when manually open wiredash', () { + // fakeAsync((async) { + // // user opened app before + // prefs.setInt( + // SyncEngine.lastSuccessfulPingKey, + // clock.now().millisecondsSinceEpoch, + // ); + // async.elapse(const Duration(days: 1)); + // + // // Silence SDK for two days + // api.pingInvocations.interceptor = (_) async { + // throw KillSwitchException(clock.now().add(const Duration(days: 2))); + // }; + // + // // When SDK receives `silentUntil`, the sdk stops pinging + // var syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // syncEngine.onWiredashInit(); + // async.flushTimers(); + // expect(api.pingInvocations.count, 1); + // + // // app start, silenced, no ping + // syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // syncEngine.onWiredashInit(); + // async.flushTimers(); + // expect(api.pingInvocations.count, 1); + // + // // manual open, pings + // syncEngine = SyncEngine(api, () async => prefs); + // addTearDown(() => syncEngine.dispose()); + // syncEngine.onUserOpenedWiredash(); + // expect(api.pingInvocations.count, 2); + // }); + // }); + // }); }); } From 183b5393e5c239c9a4ed0b9a1bf29eef235d278d Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sat, 2 Jul 2022 17:01:05 +0200 Subject: [PATCH 15/20] Make sure widget tests don't accidentally ping --- lib/src/core/network/wiredash_api.dart | 4 +- lib/src/core/services/services.dart | 27 ++- lib/src/core/services/streampod.dart | 9 + lib/src/core/sync/ping_job.dart | 1 - lib/src/core/sync/sync_engine.dart | 33 ++-- lib/src/core/wiredash_model.dart | 2 + lib/src/core/wiredash_widget.dart | 48 ++--- test/sync/ping_job_test.dart | 125 ++++++++++++ test/sync/sync_engine_test.dart | 255 ++++--------------------- test/util/robot.dart | 10 + test/wiredash_widget_test.dart | 56 +++++- 11 files changed, 304 insertions(+), 266 deletions(-) create mode 100644 test/sync/ping_job_test.dart diff --git a/lib/src/core/network/wiredash_api.dart b/lib/src/core/network/wiredash_api.dart index a4089183..fa352a9a 100644 --- a/lib/src/core/network/wiredash_api.dart +++ b/lib/src/core/network/wiredash_api.dart @@ -156,7 +156,7 @@ extension UploadScreenshotApi on WiredashApi { /// Generic error from the Wiredash API class WiredashApiException implements Exception { - WiredashApiException({this.message, this.response}); + const WiredashApiException({this.message, this.response}); String? get messageFromServer { try { @@ -537,7 +537,7 @@ class PingResponse { /// Backend returns an error which silences the SDK for one week class KillSwitchException extends WiredashApiException { - KillSwitchException({Response? response}) : super(response: response); + const KillSwitchException({Response? response}) : super(response: response); @override String toString() { return 'KillSwitchException{${response?.statusCode}, body: ${response?.body}}'; diff --git a/lib/src/core/services/services.dart b/lib/src/core/services/services.dart index 1670e337..00e22772 100644 --- a/lib/src/core/services/services.dart +++ b/lib/src/core/services/services.dart @@ -9,8 +9,11 @@ import 'package:path_provider/path_provider.dart'; import 'package:shared_preferences/shared_preferences.dart'; import 'package:wiredash/src/core/network/wiredash_api.dart'; import 'package:wiredash/src/core/options/wiredash_options_data.dart'; +import 'package:wiredash/src/core/project_credential_validator.dart'; import 'package:wiredash/src/core/services/streampod.dart'; +import 'package:wiredash/src/core/sync/ping_job.dart'; import 'package:wiredash/src/core/sync/sync_engine.dart'; +import 'package:wiredash/src/core/sync/sync_feedback_job.dart'; import 'package:wiredash/src/core/widgets/backdrop/wiredash_backdrop.dart'; import 'package:wiredash/src/core/wiredash_model.dart'; import 'package:wiredash/src/core/wiredash_widget.dart'; @@ -67,6 +70,8 @@ class WiredashServices extends ChangeNotifier { DiscardNpsUseCase get discardNps => _locator.get(); + ProjectCredentialValidator get projectCredentialValidator => _locator.get(); + void updateWidget(Wiredash wiredashWidget) { inject((_) => wiredashWidget); } @@ -106,6 +111,8 @@ void _setupServices(WiredashServices sl) { ); sl.inject((_) => DeviceIdGenerator()); sl.inject((_) => BuildInfoManager()); + sl.inject( + (_) => const ProjectCredentialValidator()); sl.inject( (_) => BackdropController(), dispose: (model) => model.dispose(), @@ -170,7 +177,25 @@ void _setupServices(WiredashServices sl) { ); sl.inject( - (locator) => SyncEngine(), + (locator) { + final engine = SyncEngine(); + + engine.addJob( + 'ping', + PingJob( + api: locator.api, + sharedPreferencesProvider: SharedPreferences.getInstance, + ), + ); + engine.addJob( + 'feedback', + UploadPendingFeedbackJob( + feedbackSubmitter: locator.feedbackSubmitter, + ), + ); + + return engine; + }, dispose: (engine) => engine.onWiredashDispose(), ); diff --git a/lib/src/core/services/streampod.dart b/lib/src/core/services/streampod.dart index d8dd2964..577cb39a 100644 --- a/lib/src/core/services/streampod.dart +++ b/lib/src/core/services/streampod.dart @@ -5,14 +5,20 @@ class Locator { final Map _registry = {}; + bool _disposed = false; + void dispose() { for (final item in _registry.values) { item.dispose?.call(); } + _disposed = true; } /// Retrieve a instance of type [T] T get() { + if (_disposed) { + throw Exception('Locator is disposed'); + } final provider = _registry[T]; return provider!.instance as T; } @@ -22,6 +28,9 @@ class Locator { T Function(Locator, T oldInstance)? update, void Function(T)? dispose, }) { + if (_disposed) { + throw Exception('Locator is disposed'); + } late InstanceFactory provider; provider = InstanceFactory(this, create, update, () { final instance = provider._instance; diff --git a/lib/src/core/sync/ping_job.dart b/lib/src/core/sync/ping_job.dart index 40b69860..efd6926b 100644 --- a/lib/src/core/sync/ping_job.dart +++ b/lib/src/core/sync/ping_job.dart @@ -17,7 +17,6 @@ class PingJob extends Job { static const minPingGap = Duration(hours: 3); static const killSwitchSilenceDuration = Duration(days: 7); - static const pingErrorSilenceDuration = Duration(minutes: 15); @override bool shouldExecute(SdkEvent event) { diff --git a/lib/src/core/sync/sync_engine.dart b/lib/src/core/sync/sync_engine.dart index 145f1b38..90c79757 100644 --- a/lib/src/core/sync/sync_engine.dart +++ b/lib/src/core/sync/sync_engine.dart @@ -2,7 +2,7 @@ import 'dart:async'; import 'package:flutter/cupertino.dart'; -const _kSyncDebugPrint = true; +const _kSyncDebugPrint = false; void syncDebugPrint(Object? message) { if (_kSyncDebugPrint) { @@ -18,24 +18,21 @@ enum SdkEvent { } /// Executes sync jobs with the network at certain times +/// +/// Add a new job with [addJob] and it will execute when your +/// [Job.shouldExecute] returns `true`. class SyncEngine { SyncEngine(); Timer? _initTimer; - static const minSyncGap = Duration(hours: 3); - - static const lastSuccessfulPingKey = 'io.wiredash.last_successful_ping'; - static const lastFeedbackSubmissionKey = - 'io.wiredash.last_feedback_submission'; - static const silenceUntilKey = 'io.wiredash.silence_until'; - static const latestMessageIdKey = 'io.wiredash.latest_message_id'; - final Map _jobs = {}; bool get _mounted => _initTimer != null; - /// Adds a job to be executed at the right time + /// Adds a job to be executed for certain [SdkEvent] events. + /// + /// See [removeJob] to remove the job. void addJob( String name, Job job, @@ -48,6 +45,16 @@ class SyncEngine { syncDebugPrint('Added job $name (${job.runtimeType})'); } + /// Removes a jobs that was previously registered with [addJob]. + Job? removeJob(String name) { + final job = _jobs.remove(name); + if (job == null) { + return null; + } + job._name = null; + return job; + } + /// Called when the SDK is initialized (by wrapping the app) /// /// Triggers [SdkEvent.appStart] after the app settled down. @@ -59,6 +66,7 @@ class SyncEngine { return true; }()); + // _triggerEvent(SdkEvent.appStart); // Delay app start a bit, so that Wiredash doesn't slow down the app start _initTimer?.cancel(); _initTimer = Timer(const Duration(seconds: 5), () { @@ -73,9 +81,6 @@ class SyncEngine { _initTimer = null; } - /// Called when the user manually opened Wiredash - /// - /// This 100% calls the backend, forcing a sync Future onUserOpenedWiredash() async { await _triggerEvent(SdkEvent.appStart); } @@ -108,6 +113,8 @@ class SyncEngine { } } +/// A job that will be executed by [SyncEngine] when [shouldExecute] matches a +/// triggered [SdkEvent] abstract class Job { String get name => _name ?? 'unnamed'; String? _name; diff --git a/lib/src/core/wiredash_model.dart b/lib/src/core/wiredash_model.dart index 7cbf2eef..0d9af790 100644 --- a/lib/src/core/wiredash_model.dart +++ b/lib/src/core/wiredash_model.dart @@ -95,6 +95,8 @@ class WiredashModel with ChangeNotifier { .map((element) => services.backdropController.hasState) .firstWhere((element) => element); + unawaited(services.syncEngine.onUserOpenedWiredash()); + await services.backdropController.animateToOpen(); } diff --git a/lib/src/core/wiredash_widget.dart b/lib/src/core/wiredash_widget.dart index 989b7f21..27166555 100644 --- a/lib/src/core/wiredash_widget.dart +++ b/lib/src/core/wiredash_widget.dart @@ -156,7 +156,7 @@ class Wiredash extends StatefulWidget { class WiredashState extends State { final GlobalKey _appKey = GlobalKey(debugLabel: 'app'); - final WiredashServices _services = WiredashServices(); + final WiredashServices _services = _createServices(); late final WiredashBackButtonDispatcher _backButtonDispatcher; @@ -179,7 +179,7 @@ class WiredashState extends State { @override void initState() { super.initState(); - debugProjectCredentialValidator.validate( + _services.projectCredentialValidator.validate( projectId: widget.projectId, secret: widget.secret, ); @@ -188,35 +188,12 @@ class WiredashState extends State { _services.wiredashModel.addListener(_markNeedsBuild); _services.backdropController.addListener(_markNeedsBuild); - _services.syncEngine.addJob( - 'ping', - PingJob( - api: _services.api, - sharedPreferencesProvider: SharedPreferences.getInstance), - ); - _services.syncEngine.addJob( - 'feedback', - UploadPendingFeedbackJob(feedbackSubmitter: _services.feedbackSubmitter), - ); - - _services.syncEngine.onWiredashInit(); + // start the sync engine + unawaited(_services.syncEngine.onWiredashInit()); _backButtonDispatcher = WiredashBackButtonDispatcher()..initialize(); } - /// Submits pending feedbacks on app start (slightly delayed) - void scheduleFeedbackSubmission() { - _submitTimer = null; - final submitter = _services.feedbackSubmitter; - if (submitter is RetryingFeedbackSubmitter) { - submitter.submitPendingFeedbackItems(); - - if (kDebugMode) { - submitter.deletePendingFeedbacks(); - } - } - } - void _markNeedsBuild() { // rebuild the Wiredash widget state setState(() {}); @@ -228,14 +205,13 @@ class WiredashState extends State { _submitTimer = null; _services.dispose(); _backButtonDispatcher.dispose(); - _services.syncEngine.onWiredashDispose(); super.dispose(); } @override void didUpdateWidget(Wiredash oldWidget) { super.didUpdateWidget(oldWidget); - debugProjectCredentialValidator.validate( + _services.projectCredentialValidator.validate( projectId: widget.projectId, secret: widget.secret, ); @@ -356,6 +332,16 @@ Locale get _defaultLocale { return locale ?? const Locale('en', 'US'); } +/// Can be used to inject mock services for testing @visibleForTesting -ProjectCredentialValidator debugProjectCredentialValidator = - const ProjectCredentialValidator(); +WiredashServices Function()? debugServicesCreator; + +WiredashServices _createServices() { + WiredashServices? services; + assert(() { + services = debugServicesCreator?.call(); + return true; + }()); + + return services ?? WiredashServices(); +} diff --git a/test/sync/ping_job_test.dart b/test/sync/ping_job_test.dart new file mode 100644 index 00000000..22958474 --- /dev/null +++ b/test/sync/ping_job_test.dart @@ -0,0 +1,125 @@ +import 'dart:async'; +import 'dart:io'; + +import 'package:fake_async/fake_async.dart'; +import 'package:shared_preferences/shared_preferences.dart'; +import 'package:test/test.dart'; +import 'package:wiredash/src/core/network/wiredash_api.dart'; +import 'package:wiredash/src/core/sync/ping_job.dart'; + +import '../feedback/data/pending_feedback_item_storage_test.dart'; +import '../util/mock_api.dart'; + +const tenSeconds = Duration(seconds: 10); + +void main() { + group('Triggering ping', () { + late MockWiredashApi api; + late InMemorySharedPreferences prefs; + + Future prefsProvider() async => prefs; + + PingJob createPingJob() => PingJob( + api: api, + sharedPreferencesProvider: prefsProvider, + ); + + setUp(() { + api = MockWiredashApi(); + api.pingInvocations.interceptor = (invocation) async { + // 200 success + return PingResponse(); + }; + prefs = InMemorySharedPreferences(); + }); + + test('ping gets submitted', () { + fakeAsync((async) { + final pingJob = createPingJob(); + pingJob.execute(); + async.flushTimers(); + expect(api.pingInvocations.count, 1); + }); + }); + + test('do not ping again within minPingGap window', () { + fakeAsync((async) { + final pingJob = createPingJob(); + pingJob.execute(); + async.flushTimers(); + expect(api.pingInvocations.count, 1); + + async.elapse(PingJob.minPingGap - tenSeconds); + pingJob.execute(); + async.flushTimers(); + expect(api.pingInvocations.count, 1); + }); + }); + + test('ping after minPingGap window', () { + fakeAsync((async) { + final pingJob = createPingJob(); + pingJob.execute(); + async.flushTimers(); + expect(api.pingInvocations.count, 1); + + async.elapse(PingJob.minPingGap); + pingJob.execute(); + async.flushTimers(); + expect(api.pingInvocations.count, 2); + }); + }); + + test('silence for 1w after KillSwitchException', () { + fakeAsync((async) { + api.pingInvocations.interceptor = (invocation) { + throw const KillSwitchException(); + }; + final pingJob = createPingJob(); + pingJob.execute(); + async.flushTimers(); + expect(api.pingInvocations.count, 1); + + async.elapse(const Duration(days: 7) - tenSeconds); + pingJob.execute(); + async.flushTimers(); + expect(api.pingInvocations.count, 1); + }); + }); + + test('ping after KillSwitchException resumes after 1w', () { + fakeAsync((async) { + api.pingInvocations.interceptor = (invocation) { + throw const KillSwitchException(); + }; + final pingJob = + PingJob(api: api, sharedPreferencesProvider: prefsProvider); + pingJob.execute(); + async.flushTimers(); + expect(api.pingInvocations.count, 1); + + async.elapse(const Duration(days: 7)); + pingJob.execute(); + async.flushTimers(); + expect(api.pingInvocations.count, 2); + }); + }); + + test('any general Exception thrown by ping does not silence the job', () { + fakeAsync((async) { + api.pingInvocations.interceptor = (invocation) { + throw const SocketException('message'); + }; + final pingJob = + PingJob(api: api, sharedPreferencesProvider: prefsProvider); + pingJob.execute(); + async.flushTimers(); + expect(api.pingInvocations.count, 1); + + pingJob.execute(); + async.flushTimers(); + expect(api.pingInvocations.count, 2); + }); + }); + }); +} diff --git a/test/sync/sync_engine_test.dart b/test/sync/sync_engine_test.dart index cd5c7cde..0b76f754 100644 --- a/test/sync/sync_engine_test.dart +++ b/test/sync/sync_engine_test.dart @@ -1,229 +1,56 @@ +import 'dart:async'; + import 'package:clock/clock.dart'; import 'package:fake_async/fake_async.dart'; -import 'package:test/fake.dart'; import 'package:test/test.dart'; -import 'package:wiredash/src/core/network/wiredash_api.dart'; import 'package:wiredash/src/core/sync/sync_engine.dart'; -import '../feedback/data/pending_feedback_item_storage_test.dart'; -import '../util/invocation_catcher.dart'; - void main() { - group('Triggering ping', () { - late _MockWiredashApi api; - late InMemorySharedPreferences prefs; + group('sync engine', () { + test('onWiredashInit triggers SdkEvent.sppStart 5s after ', () { + fakeAsync((async) { + final syncEngine = SyncEngine(); + addTearDown(() => syncEngine.onWiredashDispose()); + + DateTime? lastExecution; + final testJob = TestJob( + trigger: [SdkEvent.appStart], + block: () { + lastExecution = clock.now(); + }, + ); + syncEngine.addJob('test', testJob); - setUp(() { - api = _MockWiredashApi(); - prefs = InMemorySharedPreferences(); + // After init + syncEngine.onWiredashInit(); + // Jobs listening to appStart are not triggered directly + async.elapse(const Duration(seconds: 4)); + expect(lastExecution, isNull); + + // but after 5s + async.elapse(const Duration(seconds: 1)); + expect(lastExecution, isNotNull); + }); }); - // - // // TODO not relevant anymore, we always want to ping - // test('Never opened Wiredash does not trigger ping', () { - // fakeAsync((async) { - // final syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // syncEngine.onWiredashInit(); - // async.elapse(const Duration(seconds: 10)); - // expect(api.pingInvocations.count, 0); - // }); - // }); - // - // test( - // 'appstart pings when the user submitted feedback/sent message ' - // 'in the last 30 days (delayed by 2 seconds)', () { - // fakeAsync((async) { - // final syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // - // // Given last feedback 29 days ago - // syncEngine.onSubmitFeedback(); - // async.elapse(const Duration(days: 29)); - // - // // Given last sync 6 hours ago - // prefs.setInt( - // SyncEngine.lastSuccessfulPingKey, - // clock.now().millisecondsSinceEpoch, - // ); - // async.elapse(const Duration(hours: 6)); - // - // syncEngine.onWiredashInit(); - // async.elapse(const Duration(milliseconds: 1990)); - // expect(api.pingInvocations.count, 0); - // async.elapse(const Duration(milliseconds: 100)); - // expect(api.pingInvocations.count, 1); - // }); - // }); - // - // test('opening wiredash triggers ping immediately', () { - // fakeAsync((async) { - // expect(api.pingInvocations.count, 0); - // final syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // syncEngine.onUserOpenedWiredash(); - // async.flushTimers(); - // expect(api.pingInvocations.count, 1); - // }); - // }); - // - // test('opening the app twice within 3h gap does nothing', () { - // fakeAsync((async) { - // // Given last ping was almost 3h ago - // prefs.setInt( - // SyncEngine.lastSuccessfulPingKey, - // clock.now().millisecondsSinceEpoch, - // ); - // async.elapse(const Duration(hours: 2, minutes: 59)); - // expect(api.pingInvocations.count, 0); - // - // final syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // syncEngine.onWiredashInit(); - // async.flushTimers(); - // expect(api.pingInvocations.count, 0); - // }); - // }); - // - // test('opening wiredash within 3h gap triggers ping', () { - // fakeAsync((async) { - // // Given last ping was almost 3h ago - // prefs.setInt( - // SyncEngine.lastSuccessfulPingKey, - // clock.now().millisecondsSinceEpoch, - // ); - // async.elapse(const Duration(hours: 2, minutes: 59)); - // expect(api.pingInvocations.count, 0); - // - // final syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // syncEngine.onUserOpenedWiredash(); - // async.flushTimers(); - // expect(api.pingInvocations.count, 1); - // }); - // }); - // - // test('last successful ping date is saved', () async { - // // Silence SDK for two days - // api.pingInvocations.interceptor = (_) async { - // return PingResponse(latestMessageId: 'asdf'); - // }; - // - // expect(prefs.getInt(SyncEngine.lastSuccessfulPingKey), isNull); - // final syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // await syncEngine.onUserOpenedWiredash(); - // expect(prefs.getInt(SyncEngine.lastSuccessfulPingKey), isNotNull); - // }); - // - // test('latest message id is saved', () async { - // // Silence SDK for two days - // api.pingInvocations.interceptor = (_) async { - // return PingResponse(latestMessageId: 'asdf'); - // }; - // - // expect(prefs.getString(SyncEngine.latestMessageIdKey), isNull); - // final syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // await syncEngine.onUserOpenedWiredash(); - // expect(prefs.getString(SyncEngine.latestMessageIdKey), 'asdf'); - // }); - // - // group('Kill Switch', () { - // test('will silence ping on wiredash initialize', () { - // // We really, really, really don't want million of wiredash users - // // to kill our backend when something hits the fan - // fakeAsync((async) { - // // user opened app before - // prefs.setInt( - // SyncEngine.lastSuccessfulPingKey, - // clock.now().millisecondsSinceEpoch, - // ); - // async.elapse(const Duration(days: 1)); - // - // // Silence SDK for two days - // api.pingInvocations.interceptor = (_) async { - // throw KillSwitchException(clock.now().add(const Duration(days: 2))); - // }; - // - // var syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // - // // When SDK receives `silentUntil`, the sdk stops pinging automatically - // syncEngine.onWiredashInit(); - // async.flushTimers(); - // expect(api.pingInvocations.count, 1); - // - // // doesn't ping within 2 day periode - // async.elapse(const Duration(days: 1)); - // syncEngine.dispose(); - // syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // syncEngine.onWiredashInit(); - // async.flushTimers(); - // expect(api.pingInvocations.count, 1); - // - // // When the silent duration is over (day 3) - // // the sdk pings again on appstart - // async.elapse(const Duration(days: 2)); - // syncEngine.dispose(); - // syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // syncEngine.onWiredashInit(); - // async.flushTimers(); - // expect(api.pingInvocations.count, 2); - // }); - // }); - // - // test('Not silent when manually open wiredash', () { - // fakeAsync((async) { - // // user opened app before - // prefs.setInt( - // SyncEngine.lastSuccessfulPingKey, - // clock.now().millisecondsSinceEpoch, - // ); - // async.elapse(const Duration(days: 1)); - // - // // Silence SDK for two days - // api.pingInvocations.interceptor = (_) async { - // throw KillSwitchException(clock.now().add(const Duration(days: 2))); - // }; - // - // // When SDK receives `silentUntil`, the sdk stops pinging - // var syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // syncEngine.onWiredashInit(); - // async.flushTimers(); - // expect(api.pingInvocations.count, 1); - // - // // app start, silenced, no ping - // syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // syncEngine.onWiredashInit(); - // async.flushTimers(); - // expect(api.pingInvocations.count, 1); - // - // // manual open, pings - // syncEngine = SyncEngine(api, () async => prefs); - // addTearDown(() => syncEngine.dispose()); - // syncEngine.onUserOpenedWiredash(); - // expect(api.pingInvocations.count, 2); - // }); - // }); - // }); }); } -class _MockWiredashApi extends Fake implements WiredashApi { - final MethodInvocationCatcher pingInvocations = - MethodInvocationCatcher('ping'); +class TestJob extends Job { + final List trigger; + final FutureOr Function() block; + + TestJob({ + required this.trigger, + required this.block, + }); + + @override + Future execute() async { + await block(); + } @override - Future ping() async { - final mockedReturnValue = - pingInvocations.addAsyncMethodCall(); - if (mockedReturnValue != null) { - return mockedReturnValue.future; - } - throw "Not implemented"; + bool shouldExecute(SdkEvent event) { + return trigger.contains(event); } } diff --git a/test/util/robot.dart b/test/util/robot.dart index 30e63876..2bb69575 100644 --- a/test/util/robot.dart +++ b/test/util/robot.dart @@ -33,6 +33,10 @@ class WiredashTestRobot { channel.setMockMethodCallHandler((MethodCall methodCall) async { return '.'; }); + + debugServicesCreator = () => createMockServices(); + addTearDown(() => debugServicesCreator = null); + await tester.pumpWidget( Wiredash( projectId: 'test', @@ -322,6 +326,12 @@ class WiredashTestRobot { } } +WiredashServices createMockServices() { + final services = WiredashServices(); + services.inject((locator) => MockWiredashApi()); + return services; +} + class WiredashTestLocalizationDelegate extends LocalizationsDelegate { @override diff --git a/test/wiredash_widget_test.dart b/test/wiredash_widget_test.dart index 0b46402e..433b6b41 100644 --- a/test/wiredash_widget_test.dart +++ b/test/wiredash_widget_test.dart @@ -4,15 +4,19 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:shared_preferences/shared_preferences.dart'; import 'package:wiredash/src/core/project_credential_validator.dart'; +import 'package:wiredash/src/core/services/services.dart'; import 'package:wiredash/src/core/wiredash_widget.dart'; import 'util/invocation_catcher.dart'; +import 'util/mock_api.dart'; import 'util/robot.dart'; void main() { group('Wiredash', () { setUp(() { SharedPreferences.setMockInitialValues({}); + debugServicesCreator = createMockServices; + addTearDown(() => debugServicesCreator = null); }); testWidgets('widget can be created', (tester) async { @@ -27,15 +31,53 @@ void main() { expect(find.byType(Wiredash), findsOneWidget); }); + testWidgets('readding Wiredash simply works and sends pings again', + (tester) async { + await tester.pumpWidget( + const Wiredash( + projectId: 'test', + secret: 'test', + // this widget never settles, allowing us to jump in the future + child: CircularProgressIndicator(), + ), + ); + await tester.pump(const Duration(seconds: 1)); + + final api1 = findWireadshServices.api as MockWiredashApi; + expect(api1.pingInvocations.count, 0); + await tester.pump(const Duration(seconds: 5)); + expect(api1.pingInvocations.count, 1); + + // remove wiredash + expect(find.byType(Wiredash), findsOneWidget); + await tester.pumpWidget(const SizedBox()); + await tester.pumpAndSettle(); + + // add it a second time + await tester.pumpWidget( + const Wiredash( + projectId: 'test', + secret: 'test', + child: SizedBox(), + ), + ); + await tester.pump(const Duration(seconds: 1)); + + final api2 = findWireadshServices.api as MockWiredashApi; + expect(api2.pingInvocations.count, 0); + await tester.pump(const Duration(seconds: 5)); + expect(api2.pingInvocations.count, 1); + }); + testWidgets( 'calls ProjectCredentialValidator.validate() initially', (tester) async { final _MockProjectCredentialValidator validator = _MockProjectCredentialValidator(); - debugProjectCredentialValidator = validator; - addTearDown(() { - debugProjectCredentialValidator = const ProjectCredentialValidator(); - }); + + debugServicesCreator = () => createMockServices() + ..inject((p0) => validator); + addTearDown(() => debugServicesCreator = null); await tester.pumpWidget( const Wiredash( @@ -85,6 +127,12 @@ class _MockProjectCredentialValidator extends Fake } } +WiredashServices get findWireadshServices { + final found = find.byType(Wiredash).evaluate().first as StatefulElement; + final wiredashState = found.state as WiredashState; + return wiredashState.debugServices; +} + class _FakeApp extends StatefulWidget { const _FakeApp({Key? key}) : super(key: key); From ccae088e2c3c11964f39f9244b6b6636c4765cd7 Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sat, 2 Jul 2022 17:21:30 +0200 Subject: [PATCH 16/20] Ignore coverage files --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 5c279906..05bea965 100644 --- a/.gitignore +++ b/.gitignore @@ -74,3 +74,4 @@ pubspec.lock !**/ios/**/default.pbxuser !**/ios/**/default.perspectivev3 !/packages/flutter_tools/test/data/dart_dependencies_test/**/.packages +coverage From bed24afc720899de78b4092a5472eec88962353c Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sat, 2 Jul 2022 17:21:41 +0200 Subject: [PATCH 17/20] Test job removal --- test/sync/sync_engine_test.dart | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/sync/sync_engine_test.dart b/test/sync/sync_engine_test.dart index 0b76f754..e5007da4 100644 --- a/test/sync/sync_engine_test.dart +++ b/test/sync/sync_engine_test.dart @@ -32,6 +32,31 @@ void main() { expect(lastExecution, isNotNull); }); }); + + test('Removing a job does not execute it anymore', () async { + final syncEngine = SyncEngine(); + addTearDown(() => syncEngine.onWiredashDispose()); + + DateTime? lastExecution; + final testJob = TestJob( + trigger: [SdkEvent.appStart], + block: () { + lastExecution = clock.now(); + }, + ); + syncEngine.addJob('test', testJob); + + await syncEngine.onWiredashInit(); + expect(lastExecution, isNull); + final firstRun = lastExecution; + + final removed = syncEngine.removeJob('test'); + expect(removed, testJob); + + await syncEngine.onWiredashInit(); + // did not update, was not executed again + expect(lastExecution, firstRun); + }); }); } From f7a13e14d3ae042518587d4a72572f385a22481f Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sat, 2 Jul 2022 17:28:24 +0200 Subject: [PATCH 18/20] Document and trigger all events --- lib/src/core/sync/sync_engine.dart | 9 +++++++++ lib/src/feedback/feedback_model.dart | 3 +++ lib/src/nps/nps_model.dart | 3 +++ 3 files changed, 15 insertions(+) diff --git a/lib/src/core/sync/sync_engine.dart b/lib/src/core/sync/sync_engine.dart index 90c79757..63908294 100644 --- a/lib/src/core/sync/sync_engine.dart +++ b/lib/src/core/sync/sync_engine.dart @@ -10,10 +10,19 @@ void syncDebugPrint(Object? message) { } } +/// Events that are triggered by the user that can be used to trigger registered +/// [Job]s. enum SdkEvent { + /// User launched the app that is wrapped in Wiredash appStart, + + /// User opened the Wiredash UI openedWiredash, + + /// User submitted feedback. It might not yet be delivered to the backend but the task is completed by the user submittedFeedback, + + /// User submitted the NPS submittedNps, } diff --git a/lib/src/feedback/feedback_model.dart b/lib/src/feedback/feedback_model.dart index a90d8b48..da61996c 100644 --- a/lib/src/feedback/feedback_model.dart +++ b/lib/src/feedback/feedback_model.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter/foundation.dart'; import 'package:flutter/widgets.dart'; import 'package:wiredash/src/_wiredash_internal.dart'; @@ -330,6 +332,7 @@ class FeedbackModel extends ChangeNotifier2 { if (submission == SubmissionState.pending) { if (kDebugMode) print("Feedback is pending"); } + unawaited(_services.syncEngine.onSubmitFeedback()); _feedbackProcessed = true; notifyListeners(); } catch (e, stack) { diff --git a/lib/src/nps/nps_model.dart b/lib/src/nps/nps_model.dart index f31abb1a..4a65aa97 100644 --- a/lib/src/nps/nps_model.dart +++ b/lib/src/nps/nps_model.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter/foundation.dart'; import 'package:wiredash/src/_wiredash_internal.dart'; import 'package:wiredash/src/core/version.dart'; @@ -54,6 +56,7 @@ class NpsModel extends ChangeNotifier2 { platformUserAgent: deviceInfo.userAgent, ); await _services.api.sendNps(body); + unawaited(_services.syncEngine.onSubmitNPS()); _closeDelay?.dispose(); _closeDelay = Delay(const Duration(seconds: 1)); await _closeDelay!.future; From cc53f9c942ef4cac40e2a616c3155e86b35a6283 Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sat, 2 Jul 2022 17:29:15 +0200 Subject: [PATCH 19/20] Cleanup --- lib/src/core/services/services.dart | 3 ++- lib/src/core/wiredash_widget.dart | 5 ----- test/feedback/data/retrying_feedback_submitter_test.dart | 4 +++- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/src/core/services/services.dart b/lib/src/core/services/services.dart index 00e22772..66f662b2 100644 --- a/lib/src/core/services/services.dart +++ b/lib/src/core/services/services.dart @@ -112,7 +112,8 @@ void _setupServices(WiredashServices sl) { sl.inject((_) => DeviceIdGenerator()); sl.inject((_) => BuildInfoManager()); sl.inject( - (_) => const ProjectCredentialValidator()); + (_) => const ProjectCredentialValidator(), + ); sl.inject( (_) => BackdropController(), dispose: (model) => model.dispose(), diff --git a/lib/src/core/wiredash_widget.dart b/lib/src/core/wiredash_widget.dart index 27166555..daccd264 100644 --- a/lib/src/core/wiredash_widget.dart +++ b/lib/src/core/wiredash_widget.dart @@ -2,18 +2,13 @@ import 'dart:async'; import 'dart:ui' as ui; import 'package:collection/collection.dart'; -import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; -import 'package:shared_preferences/shared_preferences.dart'; import 'package:wiredash/src/_wiredash_internal.dart'; import 'package:wiredash/src/_wiredash_ui.dart'; import 'package:wiredash/src/core/context_cache.dart'; import 'package:wiredash/src/core/options/wiredash_options.dart'; -import 'package:wiredash/src/core/project_credential_validator.dart'; import 'package:wiredash/src/core/support/back_button_interceptor.dart'; import 'package:wiredash/src/core/support/not_a_widgets_app.dart'; -import 'package:wiredash/src/core/sync/ping_job.dart'; -import 'package:wiredash/src/core/sync/sync_feedback_job.dart'; import 'package:wiredash/src/feedback/_feedback.dart'; import 'package:wiredash/src/feedback/feedback_backdrop.dart'; import 'package:wiredash/src/nps/nps_backdrop.dart'; diff --git a/test/feedback/data/retrying_feedback_submitter_test.dart b/test/feedback/data/retrying_feedback_submitter_test.dart index bfe87cda..4ee7160a 100644 --- a/test/feedback/data/retrying_feedback_submitter_test.dart +++ b/test/feedback/data/retrying_feedback_submitter_test.dart @@ -199,7 +199,9 @@ void main() { mockApi.sendFeedbackInvocations.interceptor = (iv) { if (!firstFileSubmitted) { firstFileSubmitted = true; - throw WiredashApiException(message: "Something unexpected happened"); + throw const WiredashApiException( + message: "Something unexpected happened", + ); } return null /*void*/; }; From 03678ecb88be71a2c1b6e1abcccbcf16e29ec560 Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sat, 2 Jul 2022 17:39:49 +0200 Subject: [PATCH 20/20] Format --- lib/src/core/sync/ping_job.dart | 8 +++++--- lib/src/core/sync/sync_engine.dart | 14 ++++++++------ lib/src/core/wiredash_widget.dart | 10 ++++++---- lib/src/feedback/data/persisted_feedback_item.dart | 2 ++ lib/src/metadata/device_info/device_info.dart | 1 + 5 files changed, 22 insertions(+), 13 deletions(-) diff --git a/lib/src/core/sync/ping_job.dart b/lib/src/core/sync/ping_job.dart index efd6926b..53f5e3a7 100644 --- a/lib/src/core/sync/ping_job.dart +++ b/lib/src/core/sync/ping_job.dart @@ -35,9 +35,11 @@ class PingJob extends Job { final lastPing = await _getLastSuccessfulPing(); if (lastPing != null && now.difference(lastPing) <= minPingGap) { - syncDebugPrint('Not syncing because within minSyncGapWindow\n' - 'now: $now lastPing:$lastPing\n' - 'diff (${now.difference(lastPing)}) <= minSyncGap ($minPingGap)'); + syncDebugPrint( + 'Not syncing because within minSyncGapWindow\n' + 'now: $now lastPing:$lastPing\n' + 'diff (${now.difference(lastPing)}) <= minSyncGap ($minPingGap)', + ); // don't ping too often on app start, only once every minPingGap return; } diff --git a/lib/src/core/sync/sync_engine.dart b/lib/src/core/sync/sync_engine.dart index 63908294..6bfd789c 100644 --- a/lib/src/core/sync/sync_engine.dart +++ b/lib/src/core/sync/sync_engine.dart @@ -68,12 +68,14 @@ class SyncEngine { /// /// Triggers [SdkEvent.appStart] after the app settled down. Future onWiredashInit() async { - assert(() { - if (_initTimer != null) { - debugPrint("Warning: called onWiredashInitialized multiple times"); - } - return true; - }()); + assert( + () { + if (_initTimer != null) { + debugPrint("Warning: called onWiredashInitialized multiple times"); + } + return true; + }(), + ); // _triggerEvent(SdkEvent.appStart); // Delay app start a bit, so that Wiredash doesn't slow down the app start diff --git a/lib/src/core/wiredash_widget.dart b/lib/src/core/wiredash_widget.dart index daccd264..e341a71f 100644 --- a/lib/src/core/wiredash_widget.dart +++ b/lib/src/core/wiredash_widget.dart @@ -333,10 +333,12 @@ WiredashServices Function()? debugServicesCreator; WiredashServices _createServices() { WiredashServices? services; - assert(() { - services = debugServicesCreator?.call(); - return true; - }()); + assert( + () { + services = debugServicesCreator?.call(); + return true; + }(), + ); return services ?? WiredashServices(); } diff --git a/lib/src/feedback/data/persisted_feedback_item.dart b/lib/src/feedback/data/persisted_feedback_item.dart index b401d7a4..4d8e43ab 100644 --- a/lib/src/feedback/data/persisted_feedback_item.dart +++ b/lib/src/feedback/data/persisted_feedback_item.dart @@ -63,6 +63,7 @@ class PersistedFeedbackItem { @override int get hashCode => + // ignore: deprecated_member_use hashList(attachments) ^ buildInfo.hashCode ^ deviceId.hashCode ^ @@ -71,6 +72,7 @@ class PersistedFeedbackItem { userId.hashCode ^ sdkVersion.hashCode ^ deviceInfo.hashCode ^ + // ignore: deprecated_member_use hashList(labels) ^ appInfo.hashCode ^ const DeepCollectionEquality.unordered().hash(customMetaData); diff --git a/lib/src/metadata/device_info/device_info.dart b/lib/src/metadata/device_info/device_info.dart index a16ddaa5..c4d78b60 100644 --- a/lib/src/metadata/device_info/device_info.dart +++ b/lib/src/metadata/device_info/device_info.dart @@ -184,6 +184,7 @@ class FlutterDeviceInfo { @override int get hashCode => platformLocale.hashCode ^ + // ignore: deprecated_member_use hashList(platformSupportedLocales) ^ padding.hashCode ^ physicalSize.hashCode ^