From 84bd3355a1b7a11a72a2ed4cd4e332b79df3fd18 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 4 May 2022 19:12:12 -0700 Subject: [PATCH 1/4] Improve information collected by the `Python: Report Issue` command --- news/1 Enhancements/19067.md | 1 + package.json | 1 + resources/report_issue_template.md | 2 +- resources/report_issue_user_settings.json | 7 +-- .../commands/reportIssueCommand.ts | 56 ++++++++++++++++--- .../commands/reportIssueCommand.unit.test.ts | 37 +++++++++++- 6 files changed, 88 insertions(+), 16 deletions(-) create mode 100644 news/1 Enhancements/19067.md diff --git a/news/1 Enhancements/19067.md b/news/1 Enhancements/19067.md new file mode 100644 index 000000000000..db37e023f7e6 --- /dev/null +++ b/news/1 Enhancements/19067.md @@ -0,0 +1 @@ +Improve information collected by the `Python: Report Issue` command. diff --git a/package.json b/package.json index 582bae0903b2..236c7b1585d8 100644 --- a/package.json +++ b/package.json @@ -922,6 +922,7 @@ "type": "string" }, "python.tensorBoard.logDirectory": { + "default":"", "description": "Set this setting to your preferred TensorBoard log directory to skip log directory prompt when starting TensorBoard.", "scope": "application", "type": "string" diff --git a/resources/report_issue_template.md b/resources/report_issue_template.md index f2fbc21d77ad..838a6f6aa9e7 100644 --- a/resources/report_issue_template.md +++ b/resources/report_issue_template.md @@ -26,7 +26,7 @@ XXX

``` -{3} +{3}{4} ```

diff --git a/resources/report_issue_user_settings.json b/resources/report_issue_user_settings.json index e30a8adc1736..d1c13e7b9e37 100644 --- a/resources/report_issue_user_settings.json +++ b/resources/report_issue_user_settings.json @@ -3,7 +3,7 @@ "pythonPath": "placeholder", "onDidChange": false, "defaultInterpreterPath": "placeholder", - "defaultLS": true, + "defaultLS": false, "envFile": "placeholder", "venvPath": "placeholder", "venvFolders": "placeholder", @@ -25,7 +25,7 @@ "linting": { "enabled": true, "cwd": "placeholder", - "Flake8Args": "placeholder", + "flake8Args": "placeholder", "flake8CategorySeverity": false, "flake8Enabled": true, "flake8Path": "placeholder", @@ -85,9 +85,6 @@ "testing": { "cwd": "placeholder", "debugPort": true, - "nosetestArgs": "placeholder", - "nosetestsEnabled": true, - "nosetestPath": "placeholder", "promptToConfigure": true, "pytestArgs": "placeholder", "pytestEnabled": true, diff --git a/src/client/common/application/commands/reportIssueCommand.ts b/src/client/common/application/commands/reportIssueCommand.ts index 18c95f669d78..d18299e6698e 100644 --- a/src/client/common/application/commands/reportIssueCommand.ts +++ b/src/client/common/application/commands/reportIssueCommand.ts @@ -7,8 +7,9 @@ import * as fs from 'fs-extra'; import * as os from 'os'; import * as path from 'path'; import { inject, injectable } from 'inversify'; +import { isEqual } from 'lodash'; import { IExtensionSingleActivationService } from '../../../activation/types'; -import { ICommandManager, IWorkspaceService } from '../types'; +import { IApplicationEnvironment, ICommandManager, IWorkspaceService } from '../types'; import { EXTENSION_ROOT_DIR } from '../../../constants'; import { IInterpreterService } from '../../../interpreter/contracts'; import { Commands } from '../../constants'; @@ -16,6 +17,8 @@ import { IConfigurationService, IPythonSettings } from '../../types'; import { sendTelemetryEvent } from '../../../telemetry'; import { EventName } from '../../../telemetry/constants'; import { EnvironmentType } from '../../../pythonEnvironments/info'; +import { PythonSettings } from '../../configSettings'; +import { SystemVariables } from '../../variables/systemVariables'; /** * Allows the user to report an issue related to the Python extension using our template. @@ -24,12 +27,18 @@ import { EnvironmentType } from '../../../pythonEnvironments/info'; export class ReportIssueCommandHandler implements IExtensionSingleActivationService { public readonly supportedWorkspaceTypes = { untrustedWorkspace: false, virtualWorkspace: true }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + private readonly packageJSONSettings: any; + constructor( @inject(ICommandManager) private readonly commandManager: ICommandManager, @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(IInterpreterService) private readonly interpreterService: IInterpreterService, @inject(IConfigurationService) protected readonly configurationService: IConfigurationService, - ) {} + @inject(IApplicationEnvironment) appEnvironment: IApplicationEnvironment, + ) { + this.packageJSONSettings = appEnvironment.packageJson?.contributes?.configuration?.properties; + } public async activate(): Promise { this.commandManager.registerCommand(Commands.ReportIssue, this.openReportIssue, this); @@ -48,20 +57,31 @@ export class ReportIssueCommandHandler implements IExtensionSingleActivationServ const argSetting = argSettings[property]; if (argSetting) { if (typeof argSetting === 'object') { - userSettings = userSettings.concat(os.EOL, property, os.EOL); + let propertyHeaderAdded = false; const argSettingsDict = (settings[property] as unknown) as Record; if (typeof argSettingsDict === 'object') { Object.keys(argSetting).forEach((item) => { const prop = argSetting[item]; if (prop) { - const value = prop === true ? JSON.stringify(argSettingsDict[item]) : '""'; - userSettings = userSettings.concat('• ', item, ': ', value, os.EOL); + const defaultValue = this.getDefaultValue(`${property}.${item}`); + if (defaultValue === undefined || !isEqual(defaultValue, argSettingsDict[item])) { + if (!propertyHeaderAdded) { + userSettings = userSettings.concat(os.EOL, property, os.EOL); + propertyHeaderAdded = true; + } + const value = + prop === true ? JSON.stringify(argSettingsDict[item]) : '""'; + userSettings = userSettings.concat('• ', item, ': ', value, os.EOL); + } } }); } } else { - const value = argSetting === true ? JSON.stringify(settings[property]) : '""'; - userSettings = userSettings.concat(os.EOL, property, ': ', value, os.EOL); + const defaultValue = this.getDefaultValue(property); + if (defaultValue === undefined || !isEqual(defaultValue, settings[property])) { + const value = argSetting === true ? JSON.stringify(settings[property]) : '""'; + userSettings = userSettings.concat(os.EOL, property, ': ', value, os.EOL); + } } } }); @@ -72,10 +92,30 @@ export class ReportIssueCommandHandler implements IExtensionSingleActivationServ this.workspaceService.getConfiguration('python').get('languageServer') || 'Not Found'; const virtualEnvKind = interpreter?.envType || EnvironmentType.Unknown; + const hasMultipleFolders = (this.workspaceService.workspaceFolders?.length ?? 0) > 1; + const hasMultipleFoldersText = + hasMultipleFolders && userSettings !== '' + ? `Multiroot scenario, following user settings may not apply:${os.EOL}` + : ''; await this.commandManager.executeCommand('workbench.action.openIssueReporter', { extensionId: 'ms-python.python', - issueBody: template.format(pythonVersion, virtualEnvKind, languageServer, userSettings), + issueBody: template.format( + pythonVersion, + virtualEnvKind, + languageServer, + hasMultipleFoldersText, + userSettings, + ), }); sendTelemetryEvent(EventName.USE_REPORT_ISSUE_COMMAND, undefined, {}); } + + private getDefaultValue(settingKey: string) { + if (!this.packageJSONSettings) { + return undefined; + } + const resource = PythonSettings.getSettingsUriAndTarget(undefined, this.workspaceService).uri; + const systemVariables = new SystemVariables(resource, undefined, this.workspaceService); + return systemVariables.resolveAny(this.packageJSONSettings[`python.${settingKey}`]?.default); + } } diff --git a/src/test/common/application/commands/reportIssueCommand.unit.test.ts b/src/test/common/application/commands/reportIssueCommand.unit.test.ts index 59b25221d7e3..2ebe6f14c1fa 100644 --- a/src/test/common/application/commands/reportIssueCommand.unit.test.ts +++ b/src/test/common/application/commands/reportIssueCommand.unit.test.ts @@ -12,7 +12,11 @@ import * as Telemetry from '../../../../client/telemetry'; import { LanguageServerType } from '../../../../client/activation/types'; import { CommandManager } from '../../../../client/common/application/commandManager'; import { ReportIssueCommandHandler } from '../../../../client/common/application/commands/reportIssueCommand'; -import { ICommandManager, IWorkspaceService } from '../../../../client/common/application/types'; +import { + IApplicationEnvironment, + ICommandManager, + IWorkspaceService, +} from '../../../../client/common/application/types'; import { WorkspaceService } from '../../../../client/common/application/workspace'; import { IInterpreterService } from '../../../../client/interpreter/contracts'; import { MockWorkspaceConfiguration } from '../../../mocks/mockWorkspaceConfig'; @@ -31,12 +35,14 @@ suite('Report Issue Command', () => { let workspaceService: IWorkspaceService; let interpreterService: IInterpreterService; let configurationService: IConfigurationService; + let appEnvironment: IApplicationEnvironment; setup(async () => { workspaceService = mock(WorkspaceService); cmdManager = mock(CommandManager); interpreterService = mock(InterpreterService); configurationService = mock(ConfigurationService); + appEnvironment = mock(); when(cmdManager.executeCommand('workbench.action.openIssueReporter', anything())).thenResolve(); when(workspaceService.getConfiguration('python')).thenReturn( @@ -67,6 +73,7 @@ suite('Report Issue Command', () => { instance(workspaceService), instance(interpreterService), instance(configurationService), + instance(appEnvironment), ); await reportIssueCommandHandler.activate(); }); @@ -75,7 +82,33 @@ suite('Report Issue Command', () => { sinon.restore(); }); - test('Test if issue body is filled', async () => { + test('Test if issue body is filled including all the settings', async () => { + await reportIssueCommandHandler.openReportIssue(); + + const templatePath = path.join( + EXTENSION_ROOT_DIR_FOR_TESTS, + 'src', + 'test', + 'common', + 'application', + 'commands', + 'issueTemplateVenv1.md', + ); + const expectedIssueBody = fs.readFileSync(templatePath, 'utf8'); + + const args: [string, { extensionId: string; issueBody: string }] = capture< + AllCommands, + { extensionId: string; issueBody: string } + >(cmdManager.executeCommand).last(); + + verify(cmdManager.registerCommand(Commands.ReportIssue, anything(), anything())).once(); + verify(cmdManager.executeCommand('workbench.action.openIssueReporter', anything())).once(); + expect(args[0]).to.be.equal('workbench.action.openIssueReporter'); + const actual = args[1].issueBody; + expect(actual).to.be.equal(expectedIssueBody); + }); + + test('Test if issue body is filled, if every setting is explicitly set', async () => { await reportIssueCommandHandler.openReportIssue(); const templatePath = path.join( From 604e702089aaf445718161f5affca3b0bb7f34e1 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 4 May 2022 19:18:11 -0700 Subject: [PATCH 2/4] Fix package.json --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 236c7b1585d8..cdf527ae753d 100644 --- a/package.json +++ b/package.json @@ -922,7 +922,7 @@ "type": "string" }, "python.tensorBoard.logDirectory": { - "default":"", + "default": "", "description": "Set this setting to your preferred TensorBoard log directory to skip log directory prompt when starting TensorBoard.", "scope": "application", "type": "string" From 203b129bc639910c40880395ed32e0194cf53c8c Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 5 May 2022 10:43:30 -0700 Subject: [PATCH 3/4] Add test --- .../commands/issueTemplateVenv1.md | 4 +- .../commands/issueTemplateVenv2.md | 38 +++++++++++++++++++ .../commands/reportIssueCommand.unit.test.ts | 25 ++++++++---- 3 files changed, 59 insertions(+), 8 deletions(-) create mode 100644 src/test/common/application/commands/issueTemplateVenv2.md diff --git a/src/test/common/application/commands/issueTemplateVenv1.md b/src/test/common/application/commands/issueTemplateVenv1.md index 5b2062628633..181049f2a965 100644 --- a/src/test/common/application/commands/issueTemplateVenv1.md +++ b/src/test/common/application/commands/issueTemplateVenv1.md @@ -28,12 +28,14 @@ XXX ``` experiments -• enabled: true +• enabled: false • optInto: [] • optOutFrom: [] venvPath: "" +pipenvPath: "" + ```

diff --git a/src/test/common/application/commands/issueTemplateVenv2.md b/src/test/common/application/commands/issueTemplateVenv2.md new file mode 100644 index 000000000000..235f6a1b2948 --- /dev/null +++ b/src/test/common/application/commands/issueTemplateVenv2.md @@ -0,0 +1,38 @@ + +# Behaviour +## Expected vs. Actual + +XXX + +## Steps to reproduce: + +1. XXX + + + + +# Diagnostic data + +- Python version (& distribution if applicable, e.g. Anaconda): 3.9.0 +- Type of virtual environment used (e.g. conda, venv, virtualenv, etc.): Venv +- Value of the `python.languageServer` setting: Pylance + +
+ +User Settings + +

+ +``` + +experiments +• enabled: false + +venvPath: "" + +``` + +

+
diff --git a/src/test/common/application/commands/reportIssueCommand.unit.test.ts b/src/test/common/application/commands/reportIssueCommand.unit.test.ts index 2ebe6f14c1fa..acc82e1d19a1 100644 --- a/src/test/common/application/commands/reportIssueCommand.unit.test.ts +++ b/src/test/common/application/commands/reportIssueCommand.unit.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable global-require */ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. @@ -20,14 +21,14 @@ import { import { WorkspaceService } from '../../../../client/common/application/workspace'; import { IInterpreterService } from '../../../../client/interpreter/contracts'; import { MockWorkspaceConfiguration } from '../../../mocks/mockWorkspaceConfig'; -import { EXTENSION_ROOT_DIR_FOR_TESTS } from '../../../constants'; import { InterpreterService } from '../../../../client/interpreter/interpreterService'; -import { Commands } from '../../../../client/common/constants'; +import { Commands, EXTENSION_ROOT_DIR } from '../../../../client/common/constants'; import { AllCommands } from '../../../../client/common/application/commands'; import { ConfigurationService } from '../../../../client/common/configuration/service'; import { IConfigurationService } from '../../../../client/common/types'; import { EventName } from '../../../../client/telemetry/constants'; import { EnvironmentType, PythonEnvironment } from '../../../../client/pythonEnvironments/info'; +import { EXTENSION_ROOT_DIR_FOR_TESTS } from '../../../constants'; suite('Report Issue Command', () => { let reportIssueCommandHandler: ReportIssueCommandHandler; @@ -57,12 +58,13 @@ suite('Report Issue Command', () => { when(interpreterService.getActiveInterpreter()).thenResolve(interpreter); when(configurationService.getSettings()).thenReturn({ experiments: { - enabled: true, + enabled: false, optInto: [], optOutFrom: [], }, initialize: true, venvPath: 'path', + pipenvPath: 'pipenv', // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any); @@ -82,7 +84,7 @@ suite('Report Issue Command', () => { sinon.restore(); }); - test('Test if issue body is filled including all the settings', async () => { + test('Test if issue body is filled correctly when including all the settings', async () => { await reportIssueCommandHandler.openReportIssue(); const templatePath = path.join( @@ -108,7 +110,17 @@ suite('Report Issue Command', () => { expect(actual).to.be.equal(expectedIssueBody); }); - test('Test if issue body is filled, if every setting is explicitly set', async () => { + test('Test if issue body is filled when only including settings which are explicitly set', async () => { + // eslint-disable-next-line import/no-dynamic-require + when(appEnvironment.packageJson).thenReturn(require(path.join(EXTENSION_ROOT_DIR, 'package.json'))); + reportIssueCommandHandler = new ReportIssueCommandHandler( + instance(cmdManager), + instance(workspaceService), + instance(interpreterService), + instance(configurationService), + instance(appEnvironment), + ); + await reportIssueCommandHandler.activate(); await reportIssueCommandHandler.openReportIssue(); const templatePath = path.join( @@ -118,7 +130,7 @@ suite('Report Issue Command', () => { 'common', 'application', 'commands', - 'issueTemplateVenv1.md', + 'issueTemplateVenv2.md', ); const expectedIssueBody = fs.readFileSync(templatePath, 'utf8'); @@ -127,7 +139,6 @@ suite('Report Issue Command', () => { { extensionId: string; issueBody: string } >(cmdManager.executeCommand).last(); - verify(cmdManager.registerCommand(Commands.ReportIssue, anything(), anything())).once(); verify(cmdManager.executeCommand('workbench.action.openIssueReporter', anything())).once(); expect(args[0]).to.be.equal('workbench.action.openIssueReporter'); const actual = args[1].issueBody; From 2b96d3d8e97a2f6733aa4ce1520dc90e9c37eff5 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 5 May 2022 10:54:13 -0700 Subject: [PATCH 4/4] Add test for multiroot --- src/test/common/application/commands/issueTemplateVenv2.md | 1 + .../application/commands/reportIssueCommand.unit.test.ts | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/test/common/application/commands/issueTemplateVenv2.md b/src/test/common/application/commands/issueTemplateVenv2.md index 235f6a1b2948..974b0f113086 100644 --- a/src/test/common/application/commands/issueTemplateVenv2.md +++ b/src/test/common/application/commands/issueTemplateVenv2.md @@ -26,6 +26,7 @@ XXX

``` +Multiroot scenario, following user settings may not apply: experiments • enabled: false diff --git a/src/test/common/application/commands/reportIssueCommand.unit.test.ts b/src/test/common/application/commands/reportIssueCommand.unit.test.ts index acc82e1d19a1..92b4c0725f3f 100644 --- a/src/test/common/application/commands/reportIssueCommand.unit.test.ts +++ b/src/test/common/application/commands/reportIssueCommand.unit.test.ts @@ -9,6 +9,7 @@ import * as fs from 'fs-extra'; import * as path from 'path'; import { anything, capture, instance, mock, verify, when } from 'ts-mockito'; import { expect } from 'chai'; +import { WorkspaceFolder } from 'vscode-languageserver-protocol'; import * as Telemetry from '../../../../client/telemetry'; import { LanguageServerType } from '../../../../client/activation/types'; import { CommandManager } from '../../../../client/common/application/commandManager'; @@ -113,6 +114,10 @@ suite('Report Issue Command', () => { test('Test if issue body is filled when only including settings which are explicitly set', async () => { // eslint-disable-next-line import/no-dynamic-require when(appEnvironment.packageJson).thenReturn(require(path.join(EXTENSION_ROOT_DIR, 'package.json'))); + when(workspaceService.workspaceFolders).thenReturn([ + instance(mock(WorkspaceFolder)), + instance(mock(WorkspaceFolder)), + ]); // Multiroot scenario reportIssueCommandHandler = new ReportIssueCommandHandler( instance(cmdManager), instance(workspaceService),