From 04e0b3441647a894d73ba5bb36e45550b79de46f Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Tue, 5 May 2020 21:15:32 -0700 Subject: [PATCH 01/10] Fix path --- src/client/activation/node/languageClientFactory.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/activation/node/languageClientFactory.ts b/src/client/activation/node/languageClientFactory.ts index 8490c4910041..d607afdd5d31 100644 --- a/src/client/activation/node/languageClientFactory.ts +++ b/src/client/activation/node/languageClientFactory.ts @@ -20,7 +20,7 @@ export class NodeLanguageClientFactory implements ILanguageClientFactory { constructor( @inject(IFileSystem) private readonly fs: IFileSystem, @inject(ILanguageServerFolderService) private readonly languageServerFolderService: ILanguageServerFolderService - ) {} + ) { } public async createLanguageClient( resource: Resource, From 4477900742b0dea66a9c1219a8e16b5254f2f69d Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 18 Jun 2020 11:40:57 -0700 Subject: [PATCH 02/10] Actually fix settings --- src/client/testing/common/updateTestSettings.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/client/testing/common/updateTestSettings.ts b/src/client/testing/common/updateTestSettings.ts index 1f5c79faf32c..1a309a66c1b5 100644 --- a/src/client/testing/common/updateTestSettings.ts +++ b/src/client/testing/common/updateTestSettings.ts @@ -31,7 +31,7 @@ export class UpdateTestSettingService implements IExtensionActivationService { const filesToBeFixed = await this.getFilesToBeFixed(resource); await Promise.all(filesToBeFixed.map((file) => this.fixSettingInFile(file))); } - public getSettingsFiles(resource: Resource) { + public getSettingsFiles(resource: Resource): string[] { const settingsFiles: string[] = []; if (this.application.userSettingsFile) { settingsFiles.push(this.application.userSettingsFile); @@ -42,7 +42,7 @@ export class UpdateTestSettingService implements IExtensionActivationService { } return settingsFiles; } - public async getFilesToBeFixed(resource: Resource) { + public async getFilesToBeFixed(resource: Resource): Promise { const files = this.getSettingsFiles(resource); const result = await Promise.all( files.map(async (file) => { @@ -87,10 +87,11 @@ export class UpdateTestSettingService implements IExtensionActivationService { return fileContents; } - public async doesFileNeedToBeFixed(filePath: string) { + public async doesFileNeedToBeFixed(filePath: string): Promise { try { const contents = await this.fs.readFile(filePath); return ( + contents.indexOf('python.jediEnabled') > 0 || contents.indexOf('python.unitTest.') > 0 || contents.indexOf('.pyTest') > 0 || contents.indexOf('.pep8') > 0 From d0d50dedd938bb5380c1413c25b6150dc420a938 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 18 Jun 2020 11:45:41 -0700 Subject: [PATCH 03/10] Add news --- news/2 Fixes/12429.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/2 Fixes/12429.md diff --git a/news/2 Fixes/12429.md b/news/2 Fixes/12429.md new file mode 100644 index 000000000000..4fea2a4f8f5c --- /dev/null +++ b/news/2 Fixes/12429.md @@ -0,0 +1 @@ +Fixed issue when `python.jediEnabled` setting was not removed and `python.languageServer` setting was not updated. From 03aa5f92820ac00a443d1b0cec174f8b0ad5e98c Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 18 Jun 2020 11:53:02 -0700 Subject: [PATCH 04/10] Add test --- .../diagnostics/checks/updateTestSettings.unit.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts b/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts index 819b27b9f8cb..c9a5bd704ee0 100644 --- a/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts +++ b/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts @@ -168,6 +168,12 @@ suite('Application Diagnostics - Check Test Settings', () => { assert.ok(!needsToBeFixed); verify(fs.readFile(__filename)).once(); }); + test('Verify `python.jediEnabled` is found in user settings', async () => { + when(fs.readFile(__filename)).thenResolve('"python.jediEnabled": false'); + const needsToBeFixed = await diagnosticService.doesFileNeedToBeFixed(__filename); + assert.ok(needsToBeFixed); + verify(fs.readFile(__filename)).once(); + }); [ { From e4a032f43211c9082e5ae4f0db96deee6160471f Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 18 Jun 2020 11:54:54 -0700 Subject: [PATCH 05/10] Format --- src/client/activation/node/languageClientFactory.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/activation/node/languageClientFactory.ts b/src/client/activation/node/languageClientFactory.ts index 8bd72f7d60b0..a8d9cbdab41d 100644 --- a/src/client/activation/node/languageClientFactory.ts +++ b/src/client/activation/node/languageClientFactory.ts @@ -20,7 +20,7 @@ export class NodeLanguageClientFactory implements ILanguageClientFactory { constructor( @inject(IFileSystem) private readonly fs: IFileSystem, @inject(ILanguageServerFolderService) private readonly languageServerFolderService: ILanguageServerFolderService - ) { } + ) {} public async createLanguageClient( resource: Resource, From 27eeec76f8519ed0beb45274db2cd9200bde1f26 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 18 Jun 2020 16:29:44 -0700 Subject: [PATCH 06/10] Suppress 'jediEnabled' removal --- src/client/testing/common/updateTestSettings.ts | 13 +++---------- .../checks/updateTestSettings.unit.test.ts | 12 ++++++------ 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/client/testing/common/updateTestSettings.ts b/src/client/testing/common/updateTestSettings.ts index 1a309a66c1b5..b7b60aad8ad8 100644 --- a/src/client/testing/common/updateTestSettings.ts +++ b/src/client/testing/common/updateTestSettings.ts @@ -107,13 +107,13 @@ export class UpdateTestSettingService implements IExtensionActivationService { // - `true` or missing then set to `languageServer: Jedi`. // - `false` and `languageServer` is present, do nothing. // - `false` and `languageServer` is NOT present, set `languageServer` to `Microsoft`. - // `jediEnabled` is then removed. + // `jediEnabled` is NOT removed since JSONC parser may also remove comments. const jediEnabledPath = ['python.jediEnabled']; const languageServerPath = ['python.languageServer']; try { - let ast = parseTree(fileContent); - let jediEnabledNode = findNodeAtLocation(ast, jediEnabledPath); + const ast = parseTree(fileContent); + const jediEnabledNode = findNodeAtLocation(ast, jediEnabledPath); const jediEnabled = jediEnabledNode ? getNodeValue(jediEnabledNode) : true; const languageServerNode = findNodeAtLocation(ast, languageServerPath); const formattingOptions: FormattingOptions = { @@ -135,13 +135,6 @@ export class UpdateTestSettingService implements IExtensionActivationService { } fileContent = applyEdits(fileContent, edits); - // Remove jediEnabled - ast = parseTree(fileContent); - jediEnabledNode = findNodeAtLocation(ast, jediEnabledPath); - if (jediEnabledNode) { - edits = modify(fileContent, jediEnabledPath, undefined, { formattingOptions }); - fileContent = applyEdits(fileContent, edits); - } // tslint:disable-next-line:no-empty } catch {} return fileContent; diff --git a/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts b/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts index c9a5bd704ee0..d70dd05728ef 100644 --- a/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts +++ b/src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts @@ -224,32 +224,32 @@ suite('Application Diagnostics - Check Test Settings', () => { { testTitle: 'jediEnabled: true, no languageServer setting', contents: '{ "python.jediEnabled": true }', - expectedContent: '{"python.languageServer": "Jedi"}' + expectedContent: '{ "python.jediEnabled": true, "python.languageServer": "Jedi"}' }, { testTitle: 'jediEnabled: true, languageServer setting present', contents: '{ "python.jediEnabled": true }', - expectedContent: '{"python.languageServer": "Jedi"}' + expectedContent: '{ "python.jediEnabled": true, "python.languageServer": "Jedi"}' }, { testTitle: 'jediEnabled: false, no languageServer setting', contents: '{ "python.jediEnabled": false }', - expectedContent: '{"python.languageServer": "Microsoft"}' + expectedContent: '{ "python.jediEnabled": false, "python.languageServer": "Microsoft"}' }, { testTitle: 'jediEnabled: false, languageServer is Microsoft', contents: '{ "python.jediEnabled": false, "python.languageServer": "Microsoft" }', - expectedContent: '{"python.languageServer": "Microsoft"}' + expectedContent: '{ "python.jediEnabled": false, "python.languageServer": "Microsoft"}' }, { testTitle: 'jediEnabled: false, languageServer is None', contents: '{ "python.jediEnabled": false, "python.languageServer": "None" }', - expectedContent: '{"python.languageServer": "None"}' + expectedContent: '{ "python.jediEnabled": false, "python.languageServer": "None"}' }, { testTitle: 'jediEnabled: false, languageServer is Jedi', contents: '{ "python.jediEnabled": false, "python.languageServer": "Jedi" }', - expectedContent: '{"python.languageServer": "Jedi"}' + expectedContent: '{ "python.jediEnabled": false, "python.languageServer": "Jedi"}' } ].forEach((item) => { test(item.testTitle, async () => { From 3ed655b7f239d923598226cbc7ae9461f98ae266 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 26 Jun 2020 08:31:45 -0700 Subject: [PATCH 07/10] Drop survey first launch threshold --- src/client/languageServices/languageServerSurveyBanner.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/languageServices/languageServerSurveyBanner.ts b/src/client/languageServices/languageServerSurveyBanner.ts index 3d58991a6094..721b9d05d965 100644 --- a/src/client/languageServices/languageServerSurveyBanner.ts +++ b/src/client/languageServices/languageServerSurveyBanner.ts @@ -44,8 +44,8 @@ export class LanguageServerSurveyBanner implements IPythonExtensionBanner { @inject(IPersistentStateFactory) private persistentState: IPersistentStateFactory, @inject(IBrowserService) private browserService: IBrowserService, @inject(ILanguageServerFolderService) private lsService: ILanguageServerFolderService, - showAfterMinimumEventsCount: number = 100, - showBeforeMaximumEventsCount: number = 500 + showAfterMinimumEventsCount: number = 30, + showBeforeMaximumEventsCount: number = 200 ) { this.minCompletionsBeforeShow = showAfterMinimumEventsCount; this.maxCompletionsBeforeShow = showBeforeMaximumEventsCount; From 20c7c531d7656bbf40bd7d74dd91ee4b8ba0f5d7 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 10 Sep 2020 09:35:48 -0700 Subject: [PATCH 08/10] Telemetry for exceptions --- src/client/activation/node/languageServerProxy.ts | 7 ++++++- src/client/telemetry/index.ts | 5 ++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/client/activation/node/languageServerProxy.ts b/src/client/activation/node/languageServerProxy.ts index bdae2938b2a9..22dcf20cba9d 100644 --- a/src/client/activation/node/languageServerProxy.ts +++ b/src/client/activation/node/languageServerProxy.ts @@ -126,7 +126,12 @@ export class NodeLanguageServerProxy implements ILanguageServerProxy { // Replace all slashes in the method name so it doesn't get scrubbed by vscode-extension-telemetry. method: telemetryEvent.Properties.method?.replace(/\//g, '.') }; - sendTelemetryEvent(eventName, telemetryEvent.Measurements, formattedProperties); + sendTelemetryEvent( + eventName, + telemetryEvent.Measurements, + formattedProperties, + telemetryEvent.Exception + ); }); } await this.registerTestServices(); diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 434b929f59c6..120abf625e1b 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -118,7 +118,7 @@ export function sendTelemetryEvent

= {}; - let eventNameSent = eventName as string; + const eventNameSent = eventName as string; if (ex) { // When sending telemetry events for exceptions no need to send custom properties. @@ -126,9 +126,8 @@ export function sendTelemetryEvent

Date: Thu, 10 Sep 2020 14:14:19 -0700 Subject: [PATCH 09/10] Remove custom exception telemetry --- src/client/telemetry/index.ts | 36 +----------- src/test/telemetry/index.unit.test.ts | 85 +++------------------------ 2 files changed, 10 insertions(+), 111 deletions(-) diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 120abf625e1b..07cb80ee85bf 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -126,7 +126,7 @@ export function sendTelemetryEvent

0) { - parts.push(frame.getTypeName()); - } - if (typeof frame.getMethodName() === 'string' && frame.getMethodName().length > 0) { - parts.push(frame.getMethodName()); - } - if (typeof frame.getFunctionName() === 'string' && frame.getFunctionName().length > 0) { - if (parts.length !== 2 || parts.join('.') !== frame.getFunctionName()) { - parts.push(frame.getFunctionName()); - } - } - return parts.join('.'); -} - /** * Map all shared properties to their data types. */ diff --git a/src/test/telemetry/index.unit.test.ts b/src/test/telemetry/index.unit.test.ts index 13c8487fb5b7..e5ce2f9cb0a8 100644 --- a/src/test/telemetry/index.unit.test.ts +++ b/src/test/telemetry/index.unit.test.ts @@ -30,6 +30,8 @@ suite('Telemetry', () => { public static properties: Record[] = []; public static measures: {}[] = []; public static errorProps: string[] | undefined; + public static exception: Error | undefined; + public static clear() { Reporter.eventName = []; Reporter.properties = []; @@ -45,6 +47,10 @@ suite('Telemetry', () => { this.sendTelemetryEvent(eventName, properties, measures); Reporter.errorProps = errorProps; } + public sendTelemetryException(error: Error, properties?: {}, measures?: {}): void { + this.sendTelemetryEvent('Exception', properties, measures); + Reporter.exception = error; + } } setup(() => { @@ -155,95 +161,22 @@ suite('Telemetry', () => { expect(Reporter.measures).to.deep.equal([measures]); expect(Reporter.properties).to.deep.equal([expectedProperties]); }); - test('Send Error Telemetry', () => { - rewiremock.enable(); - const error = new Error('Boo'); - rewiremock('vscode-extension-telemetry').with({ default: Reporter }); - - const eventName = 'Testing'; - const properties = { hello: 'world', foo: 'bar' }; - const measures = { start: 123, end: 987 }; - - // tslint:disable-next-line:no-any - sendTelemetryEvent(eventName as any, measures, properties as any, error); - - const expectedErrorProperties = { - originalEventName: eventName - }; - - expect(Reporter.eventName).to.deep.equal(['ERROR']); - expect(Reporter.measures).to.deep.equal([measures]); - expect(Reporter.properties[0].stackTrace).to.be.length.greaterThan(1); - delete Reporter.properties[0].stackTrace; - expect(Reporter.properties).to.deep.equal([expectedErrorProperties]); - expect(Reporter.errorProps).to.deep.equal([]); - }); - test('Send Error Telemetry with stack trace', () => { + test('Send Exception Telemetry', () => { rewiremock.enable(); const error = new Error('Boo'); - const root = EXTENSION_ROOT_DIR.replace(/\\/g, '/'); - error.stack = [ - 'Error: Boo', - `at Context.test (${root}/src/test/telemetry/index.unit.test.ts:50:23)`, - `at callFn (${root}/node_modules/mocha/lib/runnable.js:372:21)`, - `at Test.Runnable.run (${root}/node_modules/mocha/lib/runnable.js:364:7)`, - `at Runner.runTest (${root}/node_modules/mocha/lib/runner.js:455:10)`, - `at ${root}/node_modules/mocha/lib/runner.js:573:12`, - `at next (${root}/node_modules/mocha/lib/runner.js:369:14)`, - `at ${root}/node_modules/mocha/lib/runner.js:379:7`, - `at next (${root}/node_modules/mocha/lib/runner.js:303:14)`, - `at ${root}/node_modules/mocha/lib/runner.js:342:7`, - `at done (${root}/node_modules/mocha/lib/runnable.js:319:5)`, - `at callFn (${root}/node_modules/mocha/lib/runnable.js:395:7)`, - `at Hook.Runnable.run (${root}/node_modules/mocha/lib/runnable.js:364:7)`, - `at next (${root}/node_modules/mocha/lib/runner.js:317:10)`, - `at Immediate. (${root}/node_modules/mocha/lib/runner.js:347:5)`, - 'at runCallback (timers.js:789:20)', - 'at tryOnImmediate (timers.js:751:5)', - 'at processImmediate [as _immediateCallback] (timers.js:722:5)' - ].join('\n\t'); rewiremock('vscode-extension-telemetry').with({ default: Reporter }); const eventName = 'Testing'; const properties = { hello: 'world', foo: 'bar' }; - const measures = { start: 123, end: 987 }; // tslint:disable-next-line:no-any - sendTelemetryEvent(eventName as any, measures, properties as any, error); + sendTelemetryEvent(eventName as any, {}, properties as any, error); const expectedErrorProperties = { originalEventName: eventName }; - const stackTrace = Reporter.properties[0].stackTrace; - delete Reporter.properties[0].stackTrace; - - expect(Reporter.eventName).to.deep.equal(['ERROR']); - expect(Reporter.measures).to.deep.equal([measures]); expect(Reporter.properties).to.deep.equal([expectedErrorProperties]); - expect(stackTrace).to.be.length.greaterThan(1); - expect(Reporter.errorProps).to.deep.equal([]); - - const expectedStack = [ - `at Context.test ${root}/src/test/telemetry/index.unit.test.ts:50:23`, - `at callFn ${root}/node_modules/mocha/lib/runnable.js:372:21`, - `at Test.Runnable.run ${root}/node_modules/mocha/lib/runnable.js:364:7`, - `at Runner.runTest ${root}/node_modules/mocha/lib/runner.js:455:10`, - `at ${root}/node_modules/mocha/lib/runner.js:573:12`, - `at next ${root}/node_modules/mocha/lib/runner.js:369:14`, - `at ${root}/node_modules/mocha/lib/runner.js:379:7`, - `at next ${root}/node_modules/mocha/lib/runner.js:303:14`, - `at ${root}/node_modules/mocha/lib/runner.js:342:7`, - `at done ${root}/node_modules/mocha/lib/runnable.js:319:5`, - `at callFn ${root}/node_modules/mocha/lib/runnable.js:395:7`, - `at Hook.Runnable.run ${root}/node_modules/mocha/lib/runnable.js:364:7`, - `at next ${root}/node_modules/mocha/lib/runner.js:317:10`, - `at Immediate ${root}/node_modules/mocha/lib/runner.js:347:5`, - 'at runCallback timers.js:789:20', - 'at tryOnImmediate timers.js:751:5', - 'at processImmediate [as _immediateCallback] timers.js:722:5' - ].join('\n\t'); - - expect(stackTrace).to.be.equal(expectedStack); + expect(Reporter.exception).to.deep.equal(error); }); }); From ce5680475524b0cd93e924d9f4986ac0963afc09 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Thu, 10 Sep 2020 15:17:41 -0700 Subject: [PATCH 10/10] Remove unused --- src/client/telemetry/index.ts | 1 - src/test/telemetry/index.unit.test.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 07cb80ee85bf..9c71a5ec6fbc 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -2,7 +2,6 @@ // Licensed under the MIT License. import type { JSONObject } from '@phosphor/coreutils'; -import * as stackTrace from 'stack-trace'; // tslint:disable-next-line: import-name import TelemetryReporter from 'vscode-extension-telemetry/lib/telemetryReporter'; diff --git a/src/test/telemetry/index.unit.test.ts b/src/test/telemetry/index.unit.test.ts index e5ce2f9cb0a8..63bb5e9fef1b 100644 --- a/src/test/telemetry/index.unit.test.ts +++ b/src/test/telemetry/index.unit.test.ts @@ -11,7 +11,6 @@ import { instance, mock, verify, when } from 'ts-mockito'; import { WorkspaceConfiguration } from 'vscode'; import { IWorkspaceService } from '../../client/common/application/types'; import { WorkspaceService } from '../../client/common/application/workspace'; -import { EXTENSION_ROOT_DIR } from '../../client/constants'; import { _resetSharedProperties, clearTelemetryReporter,