From 387629a02b75f0cfdc5a1fae7c2bc11ebec6cbf9 Mon Sep 17 00:00:00 2001 From: JulesFaucherre Date: Wed, 9 Aug 2023 10:31:28 +0200 Subject: [PATCH] fix: Added scenarios for telemetry disabled-default Telemetry is disabled when: - os.Getenv("CI") == "true" beacuse it means we are in a CI environment and we don't want to ask for telemetry approval - stdout is not a TTY because we can not ask for telemetry approval. Plus it also implies we are in a script context and we may not want to be prompt anything --- .circleci/config.yml | 4 + cmd/create_telemetry.go | 6 +- cmd/create_telemetry_test.go | 206 ++++++++++++++++++++++++----------- settings/settings.go | 31 +++--- 4 files changed, 169 insertions(+), 78 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index edd87a19d..31dbf46a5 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -137,6 +137,10 @@ jobs: working_directory: integration_tests environment: TESTING: "true" + - run: | + # Make sure you can run simple command without blocking the CI + circleci version + test: executor: go steps: diff --git a/cmd/create_telemetry.go b/cmd/create_telemetry.go index 7559d1eb9..645dfe675 100644 --- a/cmd/create_telemetry.go +++ b/cmd/create_telemetry.go @@ -20,6 +20,7 @@ import ( var ( CreateUUID = func() string { return uuid.New().String() } isStdinATTY = term.IsTerminal(int(os.Stdin.Fd())) + isStdoutATTY = term.IsTerminal(int(os.Stdout.Fd())) anonymousUser = telemetry.User{ UniqueID: "cli-anonymous-telemetry", } @@ -125,8 +126,9 @@ func loadTelemetrySettings(settings *settings.TelemetrySettings, user *telemetry return } - // If stdin is not available, send telemetry event, disable telemetry and return - if !isStdinATTY { + // If stdin is not a TTY, stdout is not a TTY or we are in a CI environment, send telemetry event, disable telemetry and return + isDisabledByDefault := !isStdinATTY || !isStdoutATTY || os.Getenv("CI") == trueString + if isDisabledByDefault { settings.IsEnabled = false err := telemetry.SendTelemetryApproval(anonymousUser, telemetry.NoStdin) if err != nil { diff --git a/cmd/create_telemetry_test.go b/cmd/create_telemetry_test.go index 19c65ac70..92b5b9e9f 100644 --- a/cmd/create_telemetry_test.go +++ b/cmd/create_telemetry_test.go @@ -1,6 +1,7 @@ package cmd import ( + "os" "path/filepath" "testing" @@ -10,64 +11,12 @@ import ( "gotest.tools/v3/assert" ) -type testTelemetry struct { - events []telemetry.Event - User telemetry.User -} - -func (cli *testTelemetry) Close() error { return nil } - -func (cli *testTelemetry) Track(event telemetry.Event) error { - newEvent := event - properties := map[string]interface{}{} - if cli.User.UniqueID != "" { - properties["UUID"] = cli.User.UniqueID - } - - if cli.User.UserID != "" { - properties["user_id"] = cli.User.UserID - } - - properties["is_self_hosted"] = cli.User.IsSelfHosted - - if cli.User.OS != "" { - properties["os"] = cli.User.OS - } - - if cli.User.Version != "" { - properties["cli_version"] = cli.User.Version - } - - if cli.User.TeamName != "" { - properties["team_name"] = cli.User.TeamName - } - - if len(properties) > 0 { - newEvent.Properties = properties - } - - cli.events = append(cli.events, newEvent) - return nil -} - -type telemetryTestUI struct { - Approved bool -} - -func (ui telemetryTestUI) AskUserToApproveTelemetry(message string) bool { - return ui.Approved -} - -type telemetryTestAPIClient struct { - id string - err error -} - -func (me telemetryTestAPIClient) GetMyUserId() (string, error) { - return me.id, me.err -} - func TestLoadTelemetrySettings(t *testing.T) { + // Delete env.CI variable + oldEnvCIValue := os.Getenv("CI") + os.Setenv("CI", "") + defer os.Setenv("CI", oldEnvCIValue) + // Mock HTTP userId := "id" uniqueId := "unique-id" @@ -80,6 +29,8 @@ func TestLoadTelemetrySettings(t *testing.T) { // Create test cases type args struct { closeStdin bool + closeStdout bool + env map[string]string promptApproval bool settings settings.TelemetrySettings } @@ -172,12 +123,11 @@ func TestLoadTelemetrySettings(t *testing.T) { settings: settings.TelemetrySettings{ HasAnsweredPrompt: true, }, - fileNotCreated: true, - telemetryEvents: []telemetry.Event{}, + fileNotCreated: true, }, }, { - name: "Does not change telemetry settings if stdin is not open", + name: "Does not change telemetry settings if stdin is not a TTY", args: args{closeStdin: true}, want: want{ fileNotCreated: true, @@ -189,6 +139,65 @@ func TestLoadTelemetrySettings(t *testing.T) { }, }, }, + { + name: "Does not change telemetry settings if stdout is not a TTY", + args: args{closeStdout: true}, + want: want{ + fileNotCreated: true, + telemetryEvents: []telemetry.Event{ + {Object: "cli-telemetry", Action: "disabled_default", Properties: map[string]interface{}{ + "UUID": "cli-anonymous-telemetry", + "is_self_hosted": false, + }}, + }, + }, + }, + { + name: "Does not change telemetry settings if env.CI == true", + args: args{env: map[string]string{"CI": "true"}}, + want: want{ + fileNotCreated: true, + telemetryEvents: []telemetry.Event{ + {Object: "cli-telemetry", Action: "disabled_default", Properties: map[string]interface{}{ + "UUID": "cli-anonymous-telemetry", + "is_self_hosted": false, + }}, + }, + }, + }, + { + name: "Should try loading user id if user already answered prompt but has no user id", + args: args{ + settings: settings.TelemetrySettings{ + HasAnsweredPrompt: true, + IsEnabled: true, + }, + }, + want: want{ + settings: settings.TelemetrySettings{ + HasAnsweredPrompt: true, + IsEnabled: true, + UserID: userId, + }, + }, + }, + { + name: "Should consider has_answered_prompt before disabled-default scenarios", + args: args{ + env: map[string]string{"CI": "true"}, + settings: settings.TelemetrySettings{ + HasAnsweredPrompt: true, + IsEnabled: true, + }, + }, + want: want{ + settings: settings.TelemetrySettings{ + HasAnsweredPrompt: true, + IsEnabled: true, + UserID: userId, + }, + }, + }, } for _, tt := range testCases { @@ -203,6 +212,11 @@ func TestLoadTelemetrySettings(t *testing.T) { isStdinATTY = !tt.args.closeStdin defer (func() { isStdinATTY = oldIsStdinOpen })() + // Mock stdout + oldIsStdoutOpen := isStdoutATTY + isStdoutATTY = !tt.args.closeStdout + defer (func() { isStdoutATTY = oldIsStdoutOpen })() + // Mock telemetry telemetryClient := testTelemetry{events: make([]telemetry.Event, 0)} oldCreateActiveTelemetry := telemetry.CreateActiveTelemetry @@ -212,12 +226,25 @@ func TestLoadTelemetrySettings(t *testing.T) { } defer (func() { telemetry.CreateActiveTelemetry = oldCreateActiveTelemetry })() + // Mock env + if tt.args.env != nil { + for k, v := range tt.args.env { + oldEnvValue := os.Getenv(k) + os.Setenv(k, v) + defer os.Setenv(k, oldEnvValue) + } + } + // Run tested function loadTelemetrySettings(&tt.args.settings, &telemetry.User{}, telemetryTestAPIClient{userId, nil}, telemetryTestUI{tt.args.promptApproval}) assert.DeepEqual(t, &tt.args.settings, &tt.want.settings) // Verify good telemetry events were sent - assert.DeepEqual(t, telemetryClient.events, tt.want.telemetryEvents) + expectedEvents := tt.want.telemetryEvents + if expectedEvents == nil { + expectedEvents = []telemetry.Event{} + } + assert.DeepEqual(t, telemetryClient.events, expectedEvents) // Verify if settings file exist exist, err := settings.FS.Exists(filepath.Join(settings.SettingsPath(), "telemetry.yml")) @@ -235,3 +262,60 @@ func TestLoadTelemetrySettings(t *testing.T) { }) } } + +type testTelemetry struct { + events []telemetry.Event + User telemetry.User +} + +func (cli *testTelemetry) Close() error { return nil } + +func (cli *testTelemetry) Track(event telemetry.Event) error { + newEvent := event + properties := map[string]interface{}{} + if cli.User.UniqueID != "" { + properties["UUID"] = cli.User.UniqueID + } + + if cli.User.UserID != "" { + properties["user_id"] = cli.User.UserID + } + + properties["is_self_hosted"] = cli.User.IsSelfHosted + + if cli.User.OS != "" { + properties["os"] = cli.User.OS + } + + if cli.User.Version != "" { + properties["cli_version"] = cli.User.Version + } + + if cli.User.TeamName != "" { + properties["team_name"] = cli.User.TeamName + } + + if len(properties) > 0 { + newEvent.Properties = properties + } + + cli.events = append(cli.events, newEvent) + return nil +} + +type telemetryTestUI struct { + Approved bool +} + +func (ui telemetryTestUI) AskUserToApproveTelemetry(message string) bool { + return ui.Approved +} + +type telemetryTestAPIClient struct { + id string + err error +} + +func (me telemetryTestAPIClient) GetMyUserId() (string, error) { + return me.id, me.err +} diff --git a/settings/settings.go b/settings/settings.go index 8e920e5f5..7a9e7f0e4 100644 --- a/settings/settings.go +++ b/settings/settings.go @@ -28,21 +28,22 @@ var ( // Config is used to represent the current state of a CLI instance. type Config struct { - Host string `yaml:"host"` - DlHost string `yaml:"-"` - Endpoint string `yaml:"endpoint"` - Token string `yaml:"token"` - RestEndpoint string `yaml:"rest_endpoint"` - TLSCert string `yaml:"tls_cert"` - TLSInsecure bool `yaml:"tls_insecure"` - HTTPClient *http.Client `yaml:"-"` - Data *data.DataBag `yaml:"-"` - Debug bool `yaml:"-"` - Address string `yaml:"-"` - FileUsed string `yaml:"-"` - GitHubAPI string `yaml:"-"` - SkipUpdateCheck bool `yaml:"-"` - IsTelemetryDisabled bool `yaml:"-"` + Host string `yaml:"host"` + DlHost string `yaml:"-"` + Endpoint string `yaml:"endpoint"` + Token string `yaml:"token"` + RestEndpoint string `yaml:"rest_endpoint"` + TLSCert string `yaml:"tls_cert"` + TLSInsecure bool `yaml:"tls_insecure"` + HTTPClient *http.Client `yaml:"-"` + Data *data.DataBag `yaml:"-"` + Debug bool `yaml:"-"` + Address string `yaml:"-"` + FileUsed string `yaml:"-"` + GitHubAPI string `yaml:"-"` + SkipUpdateCheck bool `yaml:"-"` + // Parameter used to disable telemetry from tests + IsTelemetryDisabled bool `yaml:"-"` // If this value is defined, the telemetry will write all its events a file // The value of this field is the path where the telemetry will be written MockTelemetry string `yaml:"-"`