fix(test): telemetry settings errors

many settings errors like this in the test logs:

    Failed to read key "telemetry": TypeError: Failed to cast type
    "object" to convertLegacy: TypeError: Unknown telemetry setting:
    [object Object],[object Object]: Error:
        at TelemetryConfig.get (/Users/runner/work/aws-toolkit-vscode/aws-toolkit-vscode/src/shared/settings.ts:193:27)

Author: justinmk3

src/shared/settings.ts

@@ -166,18 +166,26 @@ function createSettingsClass<T extends TypeDescriptor>(section: string, descript
     type Inner = FromDescriptor<T>
 
     // Class names are not always stable, especially when bundling
-    function makeLogger(name = 'Settings') {
+    function makeLogger(name = 'Settings', loglevel: 'debug' | 'error') {
         const prefix = `${isNameMangled() ? 'Settings' : name} (${section})`
-        return (message: string) => getLogger().debug(`${prefix}: ${message}`)
+        return (message: string) =>
+            loglevel === 'debug'
+                ? getLogger().debug(`${prefix}: ${message}`)
+                : getLogger().error(`${prefix}: ${message}`)
     }
 
     return class AnonymousSettings implements TypedSettings<Inner> {
         private readonly config = this.settings.getSection(section)
         private readonly disposables: vscode.Disposable[] = []
         // TODO(sijaden): add metadata prop to `Logger` so we don't need to make one-off log functions
-        protected readonly log = makeLogger(Object.getPrototypeOf(this)?.constructor?.name)
+        protected readonly log = makeLogger(Object.getPrototypeOf(this)?.constructor?.name, 'debug')
+        protected readonly logErr = makeLogger(Object.getPrototypeOf(this)?.constructor?.name, 'error')
 
-        public constructor(private readonly settings: ClassToInterfaceType<Settings> = Settings.instance) {}
+        public constructor(
+            private readonly settings: ClassToInterfaceType<Settings> = Settings.instance,
+            /** Throw an exception if the user config is invalid or the caller type doesn't match the stored type. */
+            private readonly throwInvalid: boolean = isAutomation()
+        ) {}
 
         public get onDidChange() {
             return this.getChangedEmitter().event
@@ -186,14 +194,11 @@ function createSettingsClass<T extends TypeDescriptor>(section: string, descript
         public get<K extends keyof Inner>(key: K & string, defaultValue?: Inner[K]) {
             try {
                 return this.getOrThrow(key, defaultValue)
-            } catch (error) {
-                this.log(`failed to read key "${key}": ${error}`)
-
-                if (isAutomation() || defaultValue === undefined) {
-                    throw new Error(`Failed to read key "${key}": ${error}`)
+            } catch (e) {
+                if (this.throwInvalid || defaultValue === undefined) {
+                    throw new Error(`Failed to read key "${key}": ${e}`)
                 }
-
-                this.log(`using default value for "${key}"`)
+                this.logErr(`using default for key "${key}", read failed: ${(e as Error).message ?? '?'}`)
 
                 return defaultValue
             }
@@ -255,8 +260,13 @@ function createSettingsClass<T extends TypeDescriptor>(section: string, descript
 
                 for (const key of props.filter(isDifferent)) {
                     this.log(`key "${key}" changed`)
-                    store[key] = this.get(key)
-                    emitter.fire({ key })
+                    try {
+                        store[key] = this.get(key)
+                        emitter.fire({ key })
+                    } catch {
+                        // getChangedEmitter() can't provide a default value so
+                        // we must silence errors here. Logging is done by this.get().
+                    }
                 }
             })
 

src/shared/telemetry/activation.ts

@@ -17,7 +17,6 @@ import { fromExtensionManifest, openSettings, Settings } from '../settings'
 
 const LEGACY_SETTINGS_TELEMETRY_VALUE_DISABLE = 'Disable'
 const LEGACY_SETTINGS_TELEMETRY_VALUE_ENABLE = 'Enable'
-const TELEMETRY_SETTING_DEFAULT = true
 
 export const noticeResponseViewSettings = localize('AWS.telemetry.notificationViewSettings', 'Settings')
 export const noticeResponseOk = localize('AWS.telemetry.notificationOk', 'OK')
@@ -37,7 +36,13 @@ const CURRENT_TELEMETRY_NOTICE_VERSION = 2
 export async function activate(extensionContext: vscode.ExtensionContext, awsContext: AwsContext, settings: Settings) {
     globals.telemetry = new DefaultTelemetryService(extensionContext, awsContext, getComputeRegion())
 
-    const config = new TelemetryConfig(settings)
+    const config = new TelemetryConfig(
+        settings,
+        // Disable `throwInvalid` because:
+        //   1. Telemetry must never prevent normal Toolkit operation.
+        //   2. For tests, bad data is intentional, so these errors add unwanted noise in the test logs.
+        false
+    )
     globals.telemetry.telemetryEnabled = config.isEnabled()
 
     extensionContext.subscriptions.push(
@@ -50,7 +55,7 @@ export async function activate(extensionContext: vscode.ExtensionContext, awsCon
 
     // Prompt user about telemetry if they haven't been
     if (!isCloud9() && !hasUserSeenTelemetryNotice(extensionContext)) {
-        showTelemetryNotice(extensionContext, config)
+        showTelemetryNotice(extensionContext)
     }
 }
 
@@ -71,17 +76,7 @@ export function convertLegacy(value: unknown): boolean {
 
 export class TelemetryConfig extends fromExtensionManifest('aws', { telemetry: convertLegacy }) {
     public isEnabled(): boolean {
-        try {
-            return this.get('telemetry', TELEMETRY_SETTING_DEFAULT)
-        } catch (error) {
-            vscode.window.showErrorMessage(
-                localize(
-                    'AWS.message.error.settings.telemetry.invalid_type',
-                    'The aws.telemetry value must be a boolean'
-                )
-            )
-            return TELEMETRY_SETTING_DEFAULT
-        }
+        return this.get('telemetry', true)
     }
 }
 
@@ -101,7 +96,7 @@ export async function setHasUserSeenTelemetryNotice(extensionContext: vscode.Ext
  * Prompts user to Enable/Disable/Defer on Telemetry, then
  * handles the response appropriately.
  */
-function showTelemetryNotice(extensionContext: vscode.ExtensionContext, config: TelemetryConfig) {
+function showTelemetryNotice(extensionContext: vscode.ExtensionContext) {
     getLogger().verbose('Showing telemetry notice')
 
     const telemetryNoticeText: string = localize(

src/test/shared/settingsConfiguration.test.ts

@@ -5,7 +5,6 @@
 
 import * as assert from 'assert'
 import * as vscode from 'vscode'
-import * as env from '../../shared/vscode/env'
 import { DevSettings, Experiments, fromExtensionManifest, PromptSettings, Settings } from '../../shared/settings'
 import { TestSettings } from '../utilities/testSettingsConfiguration'
 import { ClassToInterfaceType } from '../../shared/utilities/tsUtils'
@@ -208,14 +207,9 @@ describe('DevSetting', function () {
     })
 
     it('only throws in automation', async function () {
-        const previousDesc = Object.getOwnPropertyDescriptor(env, 'isAutomation')
-        assert.ok(previousDesc)
-
+        const noAutomation = new DevSettings(settings, false)
         await settings.update(`aws.dev.${TEST_SETTING}`, 'junk')
-        Object.defineProperty(env, 'isAutomation', { value: () => false })
-        assert.strictEqual(sut.get(TEST_SETTING, true), true)
-
-        Object.defineProperty(env, 'isAutomation', previousDesc)
+        assert.strictEqual(noAutomation.get(TEST_SETTING, true), true)
     })
 })
 

src/test/shared/telemetry/activation.test.ts

@@ -79,7 +79,12 @@ describe('Telemetry on activation', function () {
     let sut: TelemetryConfig
 
     beforeEach(function () {
-        sut = new TelemetryConfig(settings)
+        sut = new TelemetryConfig(
+            settings,
+            // Disable `throwInvalid`. These tests intentionally try invalid
+            // data, so the errors are unwanted noise in the test logs.
+            false
+        )
     })
 
     afterEach(async function () {