Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clearer text about what happens with --disable-telemetry + enable-telemetry command #125995

Merged
44 changes: 34 additions & 10 deletions packages/flutter_tools/lib/runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -63,31 +63,55 @@ Future<int> run(
StackTrace? firstStackTrace;
return runZoned<Future<int>>(() async {
try {
if (args.contains('--disable-telemetry') &&
args.contains('--enable-telemetry')) {
throwToolExit(
'Both enable and disable telemetry commands were detected '
'when only one can be supplied per invocation.',
exitCode: 1);
}
Comment on lines +68 to +72
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is an exit code of 1 appropriate for this exit code? @gspencergoog

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so. Anything non-zero would work.


// Disable analytics if user passes in the `--disable-telemetry` option
// `flutter --disable-telemetry`
//
// Same functionality as `flutter config --no-analytics` for disabling
// except with the `value` hard coded as false
if (args.contains('--disable-telemetry')) {
const bool value = false;
// The tool sends the analytics event *before* toggling the flag
// intentionally to be sure that opt-out events are sent correctly.
AnalyticsConfigEvent(enabled: value).send();
if (!value) {
// Normally, the tool waits for the analytics to all send before the
// tool exits, but only when analytics are enabled. When reporting that
// analytics have been disable, the wait must be done here instead.
await globals.flutterUsage.ensureAnalyticsSent();
}
globals.flutterUsage.enabled = value;
AnalyticsConfigEvent(enabled: false).send();

// Normally, the tool waits for the analytics to all send before the
// tool exits, but only when analytics are enabled. When reporting that
// analytics have been disable, the wait must be done here instead.
await globals.flutterUsage.ensureAnalyticsSent();

globals.flutterUsage.enabled = false;
globals.printStatus('Analytics reporting disabled.');

// TODO(eliasyishak): Set the telemetry for the unified_analytics
// package as well, the above will be removed once we have
// fully transitioned to using the new package
await globals.analytics.setTelemetry(value);
await globals.analytics.setTelemetry(false);
}

// Enable analytics if user passes in the `--enable-telemetry` option
// `flutter --enable-telemetry`
//
// Same functionality as `flutter config --analytics` for enabling
// except with the `value` hard coded as true
if (args.contains('--enable-telemetry')) {
// The tool sends the analytics event *before* toggling the flag
// intentionally to be sure that opt-out events are sent correctly.
AnalyticsConfigEvent(enabled: true).send();

globals.flutterUsage.enabled = true;
globals.printStatus('Analytics reporting enabled.');

await globals.analytics.setTelemetry(true);
}


await runner.run(args);

// Triggering [runZoned]'s error callback does not necessarily mean that
Expand Down
13 changes: 10 additions & 3 deletions packages/flutter_tools/lib/src/runner/flutter_command_runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,12 @@ class FlutterCommandRunner extends CommandRunner<void> {
help: 'Suppress analytics reporting for the current CLI invocation.');
argParser.addFlag('disable-telemetry',
negatable: false,
help: 'Disable telemetry reporting when this command runs.');
help: 'Disable telemetry reporting each time a flutter or dart '
'command runs, until it is re-enabled.');
argParser.addFlag('enable-telemetry',
negatable: false,
help: 'Enable telemetry reporting each time a flutter or dart '
'command runs.');
argParser.addOption('packages',
hide: !verboseHelp,
help: 'Path to your "package_config.json" file.');
Expand Down Expand Up @@ -188,8 +193,10 @@ class FlutterCommandRunner extends CommandRunner<void> {
Future<void> runCommand(ArgResults topLevelResults) async {
final Map<Type, Object?> contextOverrides = <Type, Object?>{};

// If the disable-telemetry flag has been passed, return out
if (topLevelResults.wasParsed('disable-telemetry')) {
// If the flag for enabling or disabling telemetry is passed in,
// we will return out
if (topLevelResults.wasParsed('disable-telemetry') ||
eliasyishak marked this conversation as resolved.
Show resolved Hide resolved
topLevelResults.wasParsed('enable-telemetry')) {
return;
}

Expand Down
73 changes: 66 additions & 7 deletions packages/flutter_tools/test/general.shard/runner/runner_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -317,11 +317,6 @@ void main() {
});

group('unified_analytics', () {
final Usage legacyAnalytics = TestUsage();
setUp(() {
legacyAnalytics.enabled = false;
});

Comment on lines -320 to -324
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't being used anywhere in the tests, removing

testUsingContext(
'runner disable telemetry with flag',
() async {
Expand All @@ -346,6 +341,62 @@ void main() {
ProcessManager: () => FakeProcessManager.any(),
},
);

testUsingContext(
'runner enabling telemetry with flag',
() async {
io.setExitFunctionForTests((int exitCode) {});

expect(globals.analytics.telemetryEnabled, false);
expect(globals.analytics.shouldShowMessage, false);

await runner.run(
<String>['--enable-telemetry'],
() => <FlutterCommand>[],
// This flutterVersion disables crash reporting.
flutterVersion: '[user-branch]/',
shutdownHooks: ShutdownHooks(),
);

expect(globals.analytics.telemetryEnabled, true);
},
overrides: <Type, Generator>{
Analytics: () => FakeAnalytics(fakeTelemetryStatusOverride: false),
FileSystem: () => MemoryFileSystem.test(),
ProcessManager: () => FakeProcessManager.any(),
},
);

testUsingContext(
'throw error when both flags passed',
() async {
io.setExitFunctionForTests((int exitCode) {});

expect(globals.analytics.telemetryEnabled, true);
expect(globals.analytics.shouldShowMessage, true);

final int exitCode = await runner.run(
<String>[
'--disable-telemetry',
'--enable-telemetry',
],
() => <FlutterCommand>[],
// This flutterVersion disables crash reporting.
flutterVersion: '[user-branch]/',
shutdownHooks: ShutdownHooks(),
);

expect(exitCode, 1,
reason: 'Should return 1 due to conflicting options for telemetry');
expect(globals.analytics.telemetryEnabled, true,
reason: 'Should not have changed from initialization');
},
overrides: <Type, Generator>{
Analytics: () => FakeAnalytics(),
FileSystem: () => MemoryFileSystem.test(),
ProcessManager: () => FakeProcessManager.any(),
},
);
});
}

Expand Down Expand Up @@ -489,8 +540,16 @@ class WaitingCrashReporter implements CrashReporter {
/// A fake [Analytics] that will be used to test
/// the --disable-telemetry flag
class FakeAnalytics extends Fake implements Analytics {
bool _fakeTelemetryStatus = true;
bool _fakeShowMessage = true;

FakeAnalytics({bool fakeTelemetryStatusOverride = true})
: _fakeTelemetryStatus = fakeTelemetryStatusOverride,
_fakeShowMessage = fakeTelemetryStatusOverride;

// Both of the members below can be initialized with [fakeTelemetryStatusOverride]
// because if we pass in false for the status, that means we can also
// assume the message has been shown before
bool _fakeTelemetryStatus;
bool _fakeShowMessage;

@override
String get getConsentMessage => 'message';
Expand Down