fix: Report analytics before running command. (#8)

* fix: report analytics before running command

* docs: add explanatory comment

* test: adds missing test cases

* fix: Add more context to comment

lib/src/better_command_runner/better_command_runner.dart

@@ -1,3 +1,5 @@
+import 'dart:async';
+
 import 'package:args/args.dart';
 import 'package:args/command_runner.dart';
 import 'package:cli_tools/src/better_command_runner/exit_exception.dart';
@@ -134,22 +136,36 @@ class BetterCommandRunner extends CommandRunner {
       _onAnalyticsEvent = null;
     }
 
-    await _onBeforeRunCommand?.call(this);
-
-    try {
-      await super.runCommand(topLevelResults);
+    unawaited(Future(() async {
       var command = topLevelResults.command;
-      if (command == null) {
-        _onAnalyticsEvent?.call(BetterCommandRunnerAnalyticsEvents.help);
-      } else {
+      if (command != null) {
         // Command name can only be null for top level results.
         // But since we are taking the name of a command from the top level
         // results there should always be a name specified.
         assert(command.name != null, 'Command name should never be null.');
         _onAnalyticsEvent?.call(
           command.name ?? BetterCommandRunnerAnalyticsEvents.invalid,
         );
+        return;
+      }
+
+      // Checks if the command is valid (i.e. no unexpected arguments).
+      // If there are unexpected arguments this will trigger a [UsageException]
+      // which will be caught in the try catch around the super.runCommand call.
+      // Therefore, this ensures that the help event is not sent for
+      // commands that are invalid.
+      // Note that there are other scenarios that also trigger a [UsageException]
+      // so the try/catch statement can't be fully compensated for handled here.
+      var noUnexpectedArgs = topLevelResults.rest.isEmpty;
+      if (noUnexpectedArgs) {
+        _onAnalyticsEvent?.call(BetterCommandRunnerAnalyticsEvents.help);
       }
+    }));
+
+    await _onBeforeRunCommand?.call(this);
+
+    try {
+      await super.runCommand(topLevelResults);
     } on UsageException catch (e) {
       _logError?.call(e.toString());
       _onAnalyticsEvent?.call(BetterCommandRunnerAnalyticsEvents.invalid);

test/better_command_runner/analytics_test.dart

@@ -1,7 +1,11 @@
+import 'dart:async';
+
 import 'package:args/command_runner.dart';
 import 'package:cli_tools/better_command_runner.dart';
 import 'package:test/test.dart';
 
+import '../test_utils.dart';
+
 class MockCommand extends Command {
   static String commandName = 'mock-command';
 
@@ -23,6 +27,31 @@ class MockCommand extends Command {
   }
 }
 
+class CompletableMockCommand extends Command {
+  static String commandName = 'completable-mock-command';
+
+  @override
+  String get description => 'Mock command used to test when process hangs';
+
+  Completer<void> completer = Completer<void>();
+
+  @override
+  void run() async {
+    await completer.future;
+  }
+
+  @override
+  String get name => commandName;
+
+  CompletableMockCommand() {
+    argParser.addOption(
+      'name',
+      defaultsTo: 'serverpod',
+      allowed: <String>['serverpod'],
+    );
+  }
+}
+
 void main() {
   late BetterCommandRunner runner;
   group('Given runner with null onAnalyticsEvent callback', () {
@@ -79,6 +108,8 @@ void main() {
       var args = ['--no-${BetterCommandRunnerFlags.analytics}'];
       await runner.run(args);
 
+      await flushEventQueue();
+
       expect(runner.analyticsEnabled(), isFalse);
     });
 
@@ -92,6 +123,8 @@ void main() {
         // Ignore any exception
       }
 
+      await flushEventQueue();
+
       expect(events, hasLength(1));
       expect(events.first, equals('invalid'));
     });
@@ -107,6 +140,8 @@ void main() {
         // Ignore any exception
       }
 
+      await flushEventQueue();
+
       expect(events, hasLength(1));
       expect(events.first, equals('invalid'));
     });
@@ -115,6 +150,8 @@ void main() {
         () async {
       await runner.run([]);
 
+      await flushEventQueue();
+
       expect(events, hasLength(1));
       expect(events.first, equals('help'));
     });
@@ -124,6 +161,28 @@ void main() {
         () async {
       await runner.run(['--${BetterCommandRunnerFlags.analytics}']);
 
+      await flushEventQueue();
+
+      expect(events, hasLength(1));
+      expect(events.first, equals('help'));
+    });
+
+    test('when running with help command then "help" analytics event is sent.',
+        () async {
+      await runner.run(['help']);
+
+      await flushEventQueue();
+
+      expect(events, hasLength(1));
+      expect(events.first, equals('help'));
+    });
+
+    test('when running with help flag then "help" analytics event is sent.',
+        () async {
+      await runner.run(['--help']);
+
+      await flushEventQueue();
+
       expect(events, hasLength(1));
       expect(events.first, equals('help'));
     });
@@ -150,6 +209,8 @@ void main() {
 
       await runner.run(args);
 
+      await flushEventQueue();
+
       expect(events, hasLength(1));
       expect(events.first, equals(MockCommand.commandName));
     });
@@ -161,6 +222,8 @@ void main() {
 
       await runner.run(args);
 
+      await flushEventQueue();
+
       expect(events, hasLength(1));
       expect(events.first, equals(MockCommand.commandName));
     });
@@ -176,8 +239,75 @@ void main() {
         // Ignore any exception
       }
 
+      await flushEventQueue();
+
       expect(events, hasLength(1));
       expect(events.first, equals('invalid'));
     });
   });
+
+  group('Given runner with registered command and analytics enabled', () {
+    List<String> events = [];
+    late CompletableMockCommand command;
+    setUp(() {
+      command = CompletableMockCommand();
+      runner = BetterCommandRunner(
+        'test',
+        'this is a test cli',
+        onAnalyticsEvent: (event) => events.add(event),
+      )..addCommand(command);
+      assert(runner.analyticsEnabled());
+    });
+
+    tearDown(() {
+      events = [];
+    });
+
+    test(
+        'when running with registered command that hangs then command name is sent',
+        () async {
+      var args = [CompletableMockCommand.commandName];
+
+      unawaited(runner.run(args));
+
+      await flushEventQueue();
+
+      expect(events, hasLength(1));
+      expect(events.first, equals(CompletableMockCommand.commandName));
+
+      command.completer.complete();
+    });
+
+    test(
+        'when running with registered command that hangs and option then command name is sent,',
+        () async {
+      var args = [CompletableMockCommand.commandName, '--name', 'serverpod'];
+
+      unawaited(runner.run(args));
+
+      await flushEventQueue();
+
+      expect(events, hasLength(1));
+      expect(events.first, equals(CompletableMockCommand.commandName));
+
+      command.completer.complete();
+    });
+
+    test(
+        'when running with registered command that hangs but invalid option then "invalid" analytics event is sent,',
+        () async {
+      var args = [CompletableMockCommand.commandName, '--name', 'invalid'];
+
+      unawaited(runner.run(args).catchError((_) {
+        // Ignore parse error
+      }));
+
+      await flushEventQueue();
+
+      expect(events, hasLength(1));
+      expect(events.first, equals('invalid'));
+
+      command.completer.complete();
+    });
+  });
 }

test/test_utils.dart

@@ -0,0 +1,5 @@
+/// Test helper to flush the event queue.
+/// Useful for waiting for async events to complete before continuing the test.
+Future<void> flushEventQueue() {
+  return Future.delayed(Duration.zero);
+}