From 2e4b2ed81b54dd4884635e14de55f76ff81f2376 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Date: Thu, 7 May 2020 08:05:32 -0700 Subject: [PATCH 1/8] Revert vscode-extension-telemetry changes for the release (#11602) (#11656) * Revert "Fix slashes in telemetry unit tests (#11572)" This reverts commit 7431c9c53fa5f80fd440b47ae19a825286d3d760. * Revert "Use vscode-extension-telemetry for our exceptions & error telemetry (#11524)" This reverts commit d5065e6976ab8bf3289d70be4a34110a53b224c5. * Remove from changelog --- CHANGELOG.md | 2 - gulpfile.js | 14 +- package-lock.json | 84 ++------- package.json | 2 +- src/client/telemetry/index.ts | 108 ++++++----- .../telemetry/vscode-extension-telemetry.d.ts | 33 ++++ src/test/telemetry/index.unit.test.ts | 170 +++++++++++++----- src/test/testing/helper.ts | 2 +- 8 files changed, 241 insertions(+), 174 deletions(-) create mode 100644 src/client/telemetry/vscode-extension-telemetry.d.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 91aad481260c..f5c68097199a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -91,8 +91,6 @@ ([#11221](https://github.com/Microsoft/vscode-python/issues/11221)) 1. Lazy load types from `jupyterlab/services` and similar `npm modules`. ([#11297](https://github.com/Microsoft/vscode-python/issues/11297)) -1. Update telemetry on errors and exceptions to use [vscode-extension-telemetry](https://www.npmjs.com/package/vscode-extension-telemetry). - ([#11436](https://github.com/Microsoft/vscode-python/issues/11436)) 1. Remove IJMPConnection implementation while maintaining tests written for it. ([#11470](https://github.com/Microsoft/vscode-python/issues/11470)) 1. Implement an IJupyterVariables provider for the debugger. diff --git a/gulpfile.js b/gulpfile.js index a290b648282a..f2f91140595b 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -287,9 +287,7 @@ function getAllowedWarningsForWebPack(buildConfig) { 'WARNING in ./node_modules/@jupyterlab/services/node_modules/ws/lib/validation.js', 'WARNING in ./node_modules/any-promise/register.js', 'WARNING in ./node_modules/log4js/lib/appenders/index.js', - 'WARNING in ./node_modules/log4js/lib/clustering.js', - 'WARNING in ./node_modules/diagnostic-channel-publishers/dist/src/azure-coretracing.pub.js', - 'WARNING in ./node_modules/applicationinsights/out/AutoCollection/NativePerformance.js' + 'WARNING in ./node_modules/log4js/lib/clustering.js' ]; case 'extension': return [ @@ -302,16 +300,10 @@ function getAllowedWarningsForWebPack(buildConfig) { 'remove-files-plugin@1.4.0:', 'WARNING in ./node_modules/@jupyterlab/services/node_modules/ws/lib/buffer-util.js', 'WARNING in ./node_modules/@jupyterlab/services/node_modules/ws/lib/validation.js', - 'WARNING in ./node_modules/@jupyterlab/services/node_modules/ws/lib/Validation.js', - 'WARNING in ./node_modules/diagnostic-channel-publishers/dist/src/azure-coretracing.pub.js', - 'WARNING in ./node_modules/applicationinsights/out/AutoCollection/NativePerformance.js' + 'WARNING in ./node_modules/@jupyterlab/services/node_modules/ws/lib/Validation.js' ]; case 'debugAdapter': - return [ - 'WARNING in ./node_modules/vscode-uri/lib/index.js', - 'WARNING in ./node_modules/diagnostic-channel-publishers/dist/src/azure-coretracing.pub.js', - 'WARNING in ./node_modules/applicationinsights/out/AutoCollection/NativePerformance.js' - ]; + return ['WARNING in ./node_modules/vscode-uri/lib/index.js']; default: throw new Error('Unknown WebPack Configuration'); } diff --git a/package-lock.json b/package-lock.json index 76382dd41cdc..3c950b27ef58 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4657,14 +4657,13 @@ } }, "applicationinsights": { - "version": "1.7.4", - "resolved": "https://registry.npmjs.org/applicationinsights/-/applicationinsights-1.7.4.tgz", - "integrity": "sha512-XFLsNlcanpjFhHNvVWEfcm6hr7lu9znnb6Le1Lk5RE03YUV9X2B2n2MfM4kJZRrUdV+C0hdHxvWyv+vWoLfY7A==", + "version": "1.0.6", + "resolved": "https://registry.npmjs.org/applicationinsights/-/applicationinsights-1.0.6.tgz", + "integrity": "sha512-VQT3kBpJVPw5fCO5n+WUeSx0VHjxFtD7znYbILBlVgOS9/cMDuGFmV2Br3ObzFyZUDGNbEfW36fD1y2/vAiCKw==", "requires": { - "cls-hooked": "^4.2.2", - "continuation-local-storage": "^3.2.1", "diagnostic-channel": "0.2.0", - "diagnostic-channel-publishers": "^0.3.3" + "diagnostic-channel-publishers": "0.2.1", + "zone.js": "0.7.6" } }, "aproba": { @@ -5231,28 +5230,11 @@ "integrity": "sha512-z/WhQ5FPySLdvREByI2vZiTWwCnF0moMJ1hK9YQwDTHKh6I7/uSckMetoRGb5UBZPC1z0jlw+n/XCgjeH7y1AQ==", "dev": true }, - "async-hook-jl": { - "version": "1.7.6", - "resolved": "https://registry.npmjs.org/async-hook-jl/-/async-hook-jl-1.7.6.tgz", - "integrity": "sha512-gFaHkFfSxTjvoxDMYqDuGHlcRyUuamF8s+ZTtJdDzqjws4mCt7v0vuV79/E2Wr2/riMQgtG4/yUtXWs1gZ7JMg==", - "requires": { - "stack-chain": "^1.3.7" - } - }, "async-limiter": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/async-limiter/-/async-limiter-1.0.0.tgz", "integrity": "sha512-jp/uFnooOiO+L211eZOoSyzpOITMXx1rBITauYykG3BRYPu8h0UcxsPNB04RR5vo4Tyz3+ay17tR6JVf9qzYWg==" }, - "async-listener": { - "version": "0.6.10", - "resolved": "https://registry.npmjs.org/async-listener/-/async-listener-0.6.10.tgz", - "integrity": "sha512-gpuo6xOyF4D5DE5WvyqZdPA3NGhiT6Qf07l7DCB0wwDEsLvDIbCr6j9S5aj5Ch96dLace5tXVzWBZkxU/c5ohw==", - "requires": { - "semver": "^5.3.0", - "shimmer": "^1.1.0" - } - }, "async-settle": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/async-settle/-/async-settle-1.0.0.tgz", @@ -6825,16 +6807,6 @@ } } }, - "cls-hooked": { - "version": "4.2.2", - "resolved": "https://registry.npmjs.org/cls-hooked/-/cls-hooked-4.2.2.tgz", - "integrity": "sha512-J4Xj5f5wq/4jAvcdgoGsL3G103BtWpZrMo8NEinRltN+xpTZdI+M38pyQqhuFU/P792xkMFvnKSf+Lm81U1bxw==", - "requires": { - "async-hook-jl": "^1.7.6", - "emitter-listener": "^1.0.1", - "semver": "^5.4.1" - } - }, "clsx": { "version": "1.0.4", "resolved": "https://registry.npmjs.org/clsx/-/clsx-1.0.4.tgz", @@ -7176,15 +7148,6 @@ "resolved": "https://registry.npmjs.org/content-type/-/content-type-1.0.4.tgz", "integrity": "sha512-hIP3EEPs8tB9AT1L+NUqtwOAps4mk2Zob89MWXMHjHWg9milF/j4osnnQLXBCBFBk/tvIG/tUc9mOUJiPBhPXA==" }, - "continuation-local-storage": { - "version": "3.2.1", - "resolved": "https://registry.npmjs.org/continuation-local-storage/-/continuation-local-storage-3.2.1.tgz", - "integrity": "sha512-jx44cconVqkCEEyLSKWwkvUXwO561jXMa3LPjTPsm5QR22PA0/mhe33FT4Xb5y74JDvt/Cq+5lm8S8rskLv9ZA==", - "requires": { - "async-listener": "^0.6.0", - "emitter-listener": "^1.1.1" - } - }, "convert-source-map": { "version": "1.6.0", "resolved": "https://registry.npmjs.org/convert-source-map/-/convert-source-map-1.6.0.tgz", @@ -8727,9 +8690,9 @@ } }, "diagnostic-channel-publishers": { - "version": "0.3.4", - "resolved": "https://registry.npmjs.org/diagnostic-channel-publishers/-/diagnostic-channel-publishers-0.3.4.tgz", - "integrity": "sha512-SZ1zMfFiEabf4Qx0Og9V1gMsRoqz3O+5ENkVcNOfI+SMJ3QhQsdEoKX99r0zvreagXot2parPxmrwwUM/ja8ug==" + "version": "0.2.1", + "resolved": "https://registry.npmjs.org/diagnostic-channel-publishers/-/diagnostic-channel-publishers-0.2.1.tgz", + "integrity": "sha1-ji1geottef6IC1SLxYzGvrKIxPM=" }, "diagnostics": { "version": "1.1.1", @@ -9096,14 +9059,6 @@ "minimalistic-crypto-utils": "^1.0.0" } }, - "emitter-listener": { - "version": "1.1.2", - "resolved": "https://registry.npmjs.org/emitter-listener/-/emitter-listener-1.1.2.tgz", - "integrity": "sha512-Bt1sBAGFHY9DKY+4/2cV6izcKJUf5T7/gkdmkxzX/qv9CcGH8xSwVRW5mtX03SWJtRTWSOpzCuWN9rBFYZepZQ==", - "requires": { - "shimmer": "^1.2.0" - } - }, "emoji-regex": { "version": "7.0.3", "resolved": "https://registry.npmjs.org/emoji-regex/-/emoji-regex-7.0.3.tgz", @@ -20713,11 +20668,6 @@ } } }, - "shimmer": { - "version": "1.2.1", - "resolved": "https://registry.npmjs.org/shimmer/-/shimmer-1.2.1.tgz", - "integrity": "sha512-sQTKC1Re/rM6XyFM6fIAGHRPVGvyXfgzIDvzoq608vM+jeyVD0Tu1E6Np0Kc2zAIFWIj963V2800iF/9LPieQw==" - }, "shortid": { "version": "2.2.14", "resolved": "https://registry.npmjs.org/shortid/-/shortid-2.2.14.tgz", @@ -21310,11 +21260,6 @@ "figgy-pudding": "^3.5.1" } }, - "stack-chain": { - "version": "1.3.7", - "resolved": "https://registry.npmjs.org/stack-chain/-/stack-chain-1.3.7.tgz", - "integrity": "sha1-0ZLJ/06moiyUxN1FkXHj8AzqEoU=" - }, "stack-trace": { "version": "0.0.10", "resolved": "https://registry.npmjs.org/stack-trace/-/stack-trace-0.0.10.tgz", @@ -24618,11 +24563,11 @@ "integrity": "sha512-+OMm11R1bGYbpIJ5eQIkwoDGFF4GvBz3Ztl6/VM+/RNNb2Gjk2c0Ku+oMmfhlTmTlPCpgHBsH4JqVCbUYhu5bA==" }, "vscode-extension-telemetry": { - "version": "0.1.4", - "resolved": "https://registry.npmjs.org/vscode-extension-telemetry/-/vscode-extension-telemetry-0.1.4.tgz", - "integrity": "sha512-9U2pUZ/YwZBfA8CkBrHwMxjnq9Ab+ng8daJWJzEQ6CAxlZyRhmck23bx2lqqpEwGWJCiuceQy4k0Me6llEB4zw==", + "version": "0.1.0", + "resolved": "https://registry.npmjs.org/vscode-extension-telemetry/-/vscode-extension-telemetry-0.1.0.tgz", + "integrity": "sha512-WVCnP+uLxlqB6UD98yQNV47mR5Rf79LFxpuZhSPhEf0Sb4tPZed3a63n003/dchhOwyCTCBuNN4n8XKJkLEI1Q==", "requires": { - "applicationinsights": "1.7.4" + "applicationinsights": "1.0.6" } }, "vscode-jsonrpc": { @@ -25736,6 +25681,11 @@ } } } + }, + "zone.js": { + "version": "0.7.6", + "resolved": "https://registry.npmjs.org/zone.js/-/zone.js-0.7.6.tgz", + "integrity": "sha1-+7w50+AmHQmG8boGMG6zrrDSIAk=" } } } diff --git a/package.json b/package.json index 892c9fe62aa1..e2d8413913ee 100644 --- a/package.json +++ b/package.json @@ -2980,7 +2980,7 @@ "untildify": "^3.0.2", "vscode-debugadapter": "^1.28.0", "vscode-debugprotocol": "^1.28.0", - "vscode-extension-telemetry": "0.1.4", + "vscode-extension-telemetry": "0.1.0", "vscode-jsonrpc": "^5.0.1", "vscode-languageclient": "^6.2.0-next.2", "vscode-languageserver": "^6.2.0-next.2", diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 386f3fb352e9..89a918976ca9 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -1,14 +1,15 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. - +// tslint:disable:no-reference no-any import-name no-any function-name +/// import type { JSONObject } from '@phosphor/coreutils'; +import { basename as pathBasename, sep as pathSep } from 'path'; import * as stackTrace from 'stack-trace'; -// tslint:disable-next-line: import-name -import TelemetryReporter from 'vscode-extension-telemetry/lib/telemetryReporter'; +import TelemetryReporter from 'vscode-extension-telemetry'; import { DiagnosticCodes } from '../application/diagnostics/constants'; import { IWorkspaceService } from '../common/application/types'; -import { AppinsightsKey, isTestExecution, PVSC_EXTENSION_ID } from '../common/constants'; +import { AppinsightsKey, EXTENSION_ROOT_DIR, isTestExecution, PVSC_EXTENSION_ID } from '../common/constants'; import { traceError, traceInfo } from '../common/logger'; import { TerminalShellType } from '../common/terminal/types'; import { StopWatch } from '../common/utils/stopWatch'; @@ -27,12 +28,10 @@ import { TestProvider } from '../testing/common/types'; import { EventName, PlatformErrors } from './constants'; import { LinterTrigger, TestTool } from './types'; -// tslint:disable: no-any - /** * Checks whether telemetry is supported. * Its possible this function gets called within Debug Adapter, vscode isn't available in there. - * Within DA, there's a completely different way to send telemetry. + * Withiin DA, there's a completely different way to send telemetry. * @returns {boolean} */ function isTelemetrySupported(): boolean { @@ -64,12 +63,14 @@ function getTelemetryReporter() { const extensionId = PVSC_EXTENSION_ID; // tslint:disable-next-line:no-require-imports const extensions = (require('vscode') as typeof import('vscode')).extensions; + // tslint:disable-next-line:no-non-null-assertion const extension = extensions.getExtension(extensionId)!; + // tslint:disable-next-line:no-unsafe-any const extensionVersion = extension.packageJSON.version; // tslint:disable-next-line:no-require-imports const reporter = require('vscode-extension-telemetry').default as typeof TelemetryReporter; - return (telemetryReporter = new reporter(extensionId, extensionVersion, AppinsightsKey, true)); + return (telemetryReporter = new reporter(extensionId, extensionVersion, AppinsightsKey)); } export function clearTelemetryReporter() { @@ -87,46 +88,45 @@ export function sendTelemetryEvent

= {}; - let eventNameSent = eventName as string; - if (ex) { - // When sending telemetry events for exceptions no need to send custom properties. + if (ex && (eventName as any) !== 'ERROR') { + // When sending `ERROR` telemetry event no need to send custom properties. // Else we have to review all properties every time as part of GDPR. // Assume we have 10 events all with their own properties. // As we have errors for each event, those properties are treated as new data items. // Hence they need to be classified as part of the GDPR process, and thats unnecessary and onerous. - eventNameSent = 'ERROR'; - customProperties = { originalEventName: eventName as string, stackTrace: serializeStackTrace(ex) }; - reporter.sendTelemetryErrorEvent(eventNameSent, customProperties, measures, []); - } else { - if (properties) { - const data = properties as any; - Object.getOwnPropertyNames(data).forEach((prop) => { - if (data[prop] === undefined || data[prop] === null) { - return; - } - try { - // If there are any errors in serializing one property, ignore that and move on. - // Else nothing will be sent. - customProperties[prop] = - typeof data[prop] === 'string' - ? data[prop] - : typeof data[prop] === 'object' - ? 'object' - : data[prop].toString(); - } catch (ex) { - traceError(`Failed to serialize ${prop} for ${eventName}`, ex); - } - }); - } - - reporter.sendTelemetryEvent(eventNameSent, customProperties, measures); + const props: Record = {}; + props.stackTrace = getStackTrace(ex); + props.originalEventName = (eventName as any) as string; + reporter.sendTelemetryEvent('ERROR', props, measures); } - + const customProperties: Record = {}; + if (properties) { + // tslint:disable-next-line:prefer-type-cast no-any + const data = properties as any; + Object.getOwnPropertyNames(data).forEach((prop) => { + if (data[prop] === undefined || data[prop] === null) { + return; + } + try { + // If there are any errors in serializing one property, ignore that and move on. + // Else nothign will be sent. + // tslint:disable-next-line:prefer-type-cast no-any no-unsafe-any + (customProperties as any)[prop] = + typeof data[prop] === 'string' + ? data[prop] + : typeof data[prop] === 'object' + ? 'object' + : data[prop].toString(); + } catch (ex) { + traceError(`Failed to serialize ${prop} for ${eventName}`, ex); + } + }); + } + reporter.sendTelemetryEvent((eventName as any) as string, customProperties, measures); if (process.env && process.env.VSC_PYTHON_LOG_TELEMETRY) { traceInfo( - `Telemetry Event : ${eventNameSent} Measures: ${JSON.stringify(measures)} Props: ${JSON.stringify( + `Telemetry Event : ${eventName} Measures: ${JSON.stringify(measures)} Props: ${JSON.stringify( customProperties )} ` ); @@ -246,12 +246,32 @@ export function sendTelemetryWhenDone

${filename.substring(EXTENSION_ROOT_DIR.length)}`; + } else { + // We don't really care about files outside our extension. + filename = `${pathSep}${pathBasename(filename)}`; + } + return filename; +} + +function sanitizeName(name: string): string { + if (name.indexOf('/') === -1 && name.indexOf('\\') === -1) { + return name; + } else { + return ''; + } +} + +function getStackTrace(ex: Error): string { + // We aren't showing the error message (ex.message) since it might + // contain PII. let trace = ''; for (const frame of stackTrace.parse(ex)) { - const filename = frame.getFileName(); + let filename = frame.getFileName(); if (filename) { + filename = sanitizeFilename(filename); const lineno = frame.getLineNumber(); const colno = frame.getColumnNumber(); trace += `\n\tat ${getCallsite(frame)} ${filename}:${lineno}:${colno}`; @@ -277,7 +297,7 @@ function getCallsite(frame: stackTrace.StackFrame) { parts.push(frame.getFunctionName()); } } - return parts.join('.'); + return parts.map(sanitizeName).join('.'); } // Map all events to their properties diff --git a/src/client/telemetry/vscode-extension-telemetry.d.ts b/src/client/telemetry/vscode-extension-telemetry.d.ts new file mode 100644 index 000000000000..a5d3d6294739 --- /dev/null +++ b/src/client/telemetry/vscode-extension-telemetry.d.ts @@ -0,0 +1,33 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +declare module 'vscode-extension-telemetry' { + export default class TelemetryReporter { + /** + * Constructs a new telemetry reporter + * @param {string} extensionId All events will be prefixed with this event name + * @param {string} extensionVersion Extension version to be reported with each event + * @param {string} key The application insights key + */ + // tslint:disable-next-line:no-empty + constructor(extensionId: string, extensionVersion: string, key: string); + + /** + * Sends a telemetry event + * @param {string} eventName The event name + * @param {object} properties An associative array of strings + * @param {object} measures An associative array of numbers + */ + // tslint:disable-next-line:member-access + public sendTelemetryEvent( + eventName: string, + properties?: { + [key: string]: string; + }, + measures?: { + [key: string]: number; + // tslint:disable-next-line:no-empty + } + ): void; + } +} diff --git a/src/test/telemetry/index.unit.test.ts b/src/test/telemetry/index.unit.test.ts index 0bc9d757649e..aad20386af82 100644 --- a/src/test/telemetry/index.unit.test.ts +++ b/src/test/telemetry/index.unit.test.ts @@ -23,22 +23,16 @@ suite('Telemetry', () => { public static eventName: string[] = []; public static properties: Record[] = []; public static measures: {}[] = []; - public static errorProps: string[] | undefined; public static clear() { Reporter.eventName = []; Reporter.properties = []; Reporter.measures = []; - Reporter.errorProps = undefined; } public sendTelemetryEvent(eventName: string, properties?: {}, measures?: {}) { Reporter.eventName.push(eventName); Reporter.properties.push(properties!); Reporter.measures.push(measures!); } - public sendTelemetryErrorEvent(eventName: string, properties?: {}, measures?: {}, errorProps?: string[]) { - this.sendTelemetryEvent(eventName, properties, measures); - Reporter.errorProps = errorProps; - } } setup(() => { @@ -100,7 +94,7 @@ suite('Telemetry', () => { expect(Reporter.measures).to.deep.equal([measures]); expect(Reporter.properties).to.deep.equal([properties]); }); - test('Send Telemetry with properties', () => { + test('Send Telemetry', () => { rewiremock.enable(); rewiremock('vscode-extension-telemetry').with({ default: Reporter }); @@ -128,33 +122,31 @@ suite('Telemetry', () => { originalEventName: eventName }; - expect(Reporter.eventName).to.deep.equal(['ERROR']); - expect(Reporter.measures).to.deep.equal([measures]); + expect(Reporter.eventName).to.deep.equal(['ERROR', eventName]); + expect(Reporter.measures).to.deep.equal([measures, 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([]); + expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]); }); - test('Send Error Telemetry with stack trace', () => { + test('Send Error 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 Context.test (${EXTENSION_ROOT_DIR}/src/test/telemetry/index.unit.test.ts:50:23)`, + `at callFn (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:372:21)`, + `at Test.Runnable.run (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:364:7)`, + `at Runner.runTest (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:455:10)`, + `at ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:573:12`, + `at next (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:369:14)`, + `at ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:379:7`, + `at next (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:303:14)`, + `at ${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:342:7`, + `at done (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:319:5)`, + `at callFn (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:395:7)`, + `at Hook.Runnable.run (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runnable.js:364:7)`, + `at next (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:317:10)`, + `at Immediate. (${EXTENSION_ROOT_DIR}/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)' @@ -175,30 +167,112 @@ suite('Telemetry', () => { 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(Reporter.eventName).to.deep.equal(['ERROR', eventName]); + expect(Reporter.measures).to.deep.equal([measures, measures]); + expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]); + expect(stackTrace).to.be.length.greaterThan(1); + + const expectedStack = [ + 'at Context.test /src/test/telemetry/index.unit.test.ts:50:23\n\tat callFn /node_modules/mocha/lib/runnable.js:372:21', + 'at Test.Runnable.run /node_modules/mocha/lib/runnable.js:364:7', + 'at Runner.runTest /node_modules/mocha/lib/runner.js:455:10', + 'at /node_modules/mocha/lib/runner.js:573:12', + 'at next /node_modules/mocha/lib/runner.js:369:14', + 'at /node_modules/mocha/lib/runner.js:379:7', + 'at next /node_modules/mocha/lib/runner.js:303:14', + 'at /node_modules/mocha/lib/runner.js:342:7', + 'at done /node_modules/mocha/lib/runnable.js:319:5', + 'at callFn /node_modules/mocha/lib/runnable.js:395:7', + 'at Hook.Runnable.run /node_modules/mocha/lib/runnable.js:364:7', + 'at next /node_modules/mocha/lib/runner.js:317:10', + 'at Immediate /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); + }); + test('Ensure non extension file paths are stripped from stack trace', () => { + rewiremock.enable(); + const error = new Error('Boo'); + error.stack = [ + 'Error: Boo', + `at Context.test (${EXTENSION_ROOT_DIR}/src/test/telemetry/index.unit.test.ts:50:23)`, + 'at callFn (c:/one/two/user/node_modules/mocha/lib/runnable.js:372:21)', + 'at Test.Runnable.run (/usr/Paul/Homer/desktop/node_modules/mocha/lib/runnable.js:364:7)', + 'at Runner.runTest (\\wowwee/node_modules/mocha/lib/runner.js:455:10)', + `at Immediate. (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:347: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); + + const expectedErrorProperties = { + originalEventName: eventName + }; + + const stackTrace = Reporter.properties[0].stackTrace; + delete Reporter.properties[0].stackTrace; + + expect(Reporter.eventName).to.deep.equal(['ERROR', eventName]); + expect(Reporter.measures).to.deep.equal([measures, measures]); + expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]); + expect(stackTrace).to.be.length.greaterThan(1); + + const expectedStack = [ + 'at Context.test /src/test/telemetry/index.unit.test.ts:50:23', + 'at callFn /runnable.js:372:21', + 'at Test.Runnable.run /runnable.js:364:7', + 'at Runner.runTest /runner.js:455:10', + 'at Immediate /node_modules/mocha/lib/runner.js:347:5' + ].join('\n\t'); + + expect(stackTrace).to.be.equal(expectedStack); + }); + test('Ensure non function names containing file names (unlikely, but for sake of completeness) are stripped from stack trace', () => { + rewiremock.enable(); + const error = new Error('Boo'); + error.stack = [ + 'Error: Boo', + `at Context.test (${EXTENSION_ROOT_DIR}/src/test/telemetry/index.unit.test.ts:50:23)`, + 'at callFn (c:/one/two/user/node_modules/mocha/lib/runnable.js:372:21)', + 'at Test./usr/Paul/Homer/desktop/node_modules/mocha/lib/runnable.run (/usr/Paul/Homer/desktop/node_modules/mocha/lib/runnable.js:364:7)', + 'at Runner.runTest (\\wowwee/node_modules/mocha/lib/runner.js:455:10)', + `at Immediate. (${EXTENSION_ROOT_DIR}/node_modules/mocha/lib/runner.js:347: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); + + const expectedErrorProperties = { + originalEventName: eventName + }; + + const stackTrace = Reporter.properties[0].stackTrace; + delete Reporter.properties[0].stackTrace; + + expect(Reporter.eventName).to.deep.equal(['ERROR', eventName]); + expect(Reporter.measures).to.deep.equal([measures, measures]); + expect(Reporter.properties).to.deep.equal([expectedErrorProperties, properties]); 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' + 'at Context.test /src/test/telemetry/index.unit.test.ts:50:23', + 'at callFn /runnable.js:372:21', + 'at .run /runnable.js:364:7', + 'at Runner.runTest /runner.js:455:10', + 'at Immediate /node_modules/mocha/lib/runner.js:347:5' ].join('\n\t'); expect(stackTrace).to.be.equal(expectedStack); diff --git a/src/test/testing/helper.ts b/src/test/testing/helper.ts index 3f9ffbf44c89..d973f28d07d6 100644 --- a/src/test/testing/helper.ts +++ b/src/test/testing/helper.ts @@ -32,7 +32,7 @@ export function lookForTestFile(tests: Tests, testFile: string) { // Only "/" (forward slash) in the given filename is affected. // // This helps with readability in test code. It allows us to use -// literals for filenames and dirnames instead of path.join(). +// literals for filenames and dirnames instead of oath.join(). export function fixPath(filename: string): string { return filename.replace(/\//, sep); } From c5e366f3ad47b0a11c4e018bc8568346d6ebaf7b Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 7 May 2020 12:08:42 -0700 Subject: [PATCH 2/8] Port storage fix to release branch (#11673) * Fix storage not being used (#11649) * Fix storage not being used * Add disposable to storage so it won't write after shutdown * Fix dirty title * Hack to get tests to pass * Another way to get run all to not interfere * Update changelog --- CHANGELOG.md | 2 + .../nativeEditorOldWebView.ts | 5 ++ .../interactive-ipynb/nativeEditorStorage.ts | 49 ++++++++++++--- .../datascience/dataScienceIocContainer.ts | 15 +++++ .../nativeEditorStorage.unit.test.ts | 62 +++++++++++++++++-- .../nativeEditor.functional.test.tsx | 32 +++++----- 6 files changed, 137 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5c68097199a..ab8c4a0e0d83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,8 @@ ([#11576](https://github.com/Microsoft/vscode-python/issues/11576)) 1. Ensure kernel daemons are disposed correctly when closing notebooks. ([#11579](https://github.com/Microsoft/vscode-python/issues/11579)) +1. When VS quits, make sure to save contents of notebook for next reopen. + ([#11557](https://github.com/Microsoft/vscode-python/issues/11557)) ### Code Health diff --git a/src/client/datascience/interactive-ipynb/nativeEditorOldWebView.ts b/src/client/datascience/interactive-ipynb/nativeEditorOldWebView.ts index ebe572e0d1e9..2d0af9a7d66d 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorOldWebView.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorOldWebView.ts @@ -139,6 +139,11 @@ export class NativeEditorOldWebView extends NativeEditor { // Update our title to match this.setTitle(path.basename(model.file.fsPath)); + // Update dirty if model started out that way + if (this.model?.isDirty) { + this.setDirty().ignoreErrors(); + } + // Show ourselves await this.show(); this.model?.changed(() => { diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index b522323b1620..522248e448e5 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -8,7 +8,14 @@ import { concatMultilineStringInput, splitMultilineString } from '../../../datas import { createCodeCell } from '../../../datascience-ui/common/cellFactory'; import { traceError } from '../../common/logger'; import { IFileSystem } from '../../common/platform/types'; -import { GLOBAL_MEMENTO, ICryptoUtils, IExtensionContext, IMemento, WORKSPACE_MEMENTO } from '../../common/types'; +import { + GLOBAL_MEMENTO, + ICryptoUtils, + IDisposableRegistry, + IExtensionContext, + IMemento, + WORKSPACE_MEMENTO +} from '../../common/types'; import { noop } from '../../common/utils/misc'; import { PythonInterpreter } from '../../interpreter/contracts'; import { Identifiers, KnownNotebookLanguages, Telemetry } from '../constants'; @@ -19,6 +26,8 @@ import { CellState, ICell, IJupyterExecution, IJupyterKernelSpec, INotebookModel // tslint:disable-next-line:no-require-imports no-var-requires import detectIndent = require('detect-indent'); +import { IDisposable } from 'monaco-editor'; +import { IWorkspaceService } from '../../common/application/types'; import { sendTelemetryEvent } from '../../telemetry'; import { pruneCell } from '../common'; // tslint:disable-next-line:no-require-imports no-var-requires @@ -35,7 +44,7 @@ interface INativeEditorStorageState { } @injectable() -export class NativeEditorStorage implements INotebookModel, INotebookStorage { +export class NativeEditorStorage implements INotebookModel, INotebookStorage, IDisposable { public get isDirty(): boolean { return this._state.changeCount !== this._state.saveChangeCount; } @@ -67,6 +76,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { }; private indentAmount: string = ' '; private debouncedWriteToStorage = debounce(this.writeToStorage.bind(this), 250); + private disposed = false; constructor( @inject(IJupyterExecution) private jupyterExecution: IJupyterExecution, @@ -74,8 +84,16 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { @inject(ICryptoUtils) private crypto: ICryptoUtils, @inject(IExtensionContext) private context: IExtensionContext, @inject(IMemento) @named(GLOBAL_MEMENTO) private globalStorage: Memento, - @inject(IMemento) @named(WORKSPACE_MEMENTO) private localStorage: Memento - ) {} + @inject(IMemento) @named(WORKSPACE_MEMENTO) private localStorage: Memento, + @inject(IWorkspaceService) private workspaceService: IWorkspaceService, + @inject(IDisposableRegistry) disposableRegistry: IDisposableRegistry + ) { + disposableRegistry.push(this); + } + + public dispose() { + this.disposed = true; + } public async load(file: Uri, possibleContents?: string): Promise { // Reload our cells @@ -141,9 +159,21 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { // Write but debounced (wait at least 250 ms) return this.debouncedWriteToStorage(filePath, specialContents, cancelToken); } + + private async saveToStorage(): Promise { + // Skip doing this if auto save is enabled or is untitled + const filesConfig = this.workspaceService.getConfiguration('files', this.file); + const autoSave = filesConfig.get('autoSave', 'off'); + if (autoSave === 'off' && !this.isUntitled) { + // Write but debounced (wait at least 250 ms) + return this.storeContentsInHotExitFile(undefined); + } + } + private async writeToStorage(filePath: string, contents?: string, cancelToken?: CancellationToken): Promise { try { - if (!cancelToken?.isCancellationRequested) { + // Only write to storage if not disposed. + if (!cancelToken?.isCancellationRequested && !this.disposed) { if (contents) { await this.fileSystem.createDirectory(path.dirname(filePath)); if (!cancelToken?.isCancellationRequested) { @@ -198,6 +228,10 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { // Forward onto our listeners if necessary if (changed || this.isDirty !== oldDirty) { this._changedEmitter.fire({ ...change, newDirty: this.isDirty, oldDirty }); + if (this.isDirty) { + // Save to temp storage so we don't lose the file if the user exits VS code + this.saveToStorage().ignoreErrors(); + } } // Slightly different for the event we send to VS code. Skip version and file changes. Only send user events. if ( @@ -451,6 +485,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { const dirtyContents = await this.getStoredContents(); if (dirtyContents) { // This means we're dirty. Indicate dirty and load from this content + this._state.saveChangeCount = -1; // Indicates dirty this.loadContents(dirtyContents); } else { // Load without setting dirty @@ -621,8 +656,8 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage { if (data && !this.isUntitled && data.contents) { return data.contents; } - } catch { - noop(); + } catch (exc) { + traceError(`Exception reading from temporary storage for ${key}`, exc); } } diff --git a/src/test/datascience/dataScienceIocContainer.ts b/src/test/datascience/dataScienceIocContainer.ts index 872f52f821e8..68e2bc003473 100644 --- a/src/test/datascience/dataScienceIocContainer.ts +++ b/src/test/datascience/dataScienceIocContainer.ts @@ -3,6 +3,8 @@ //tslint:disable:trailing-comma no-any import * as child_process from 'child_process'; import { ReactWrapper } from 'enzyme'; +import * as fs from 'fs-extra'; +import * as glob from 'glob'; import { interfaces } from 'inversify'; import * as os from 'os'; import * as path from 'path'; @@ -24,6 +26,7 @@ import { import * as vsls from 'vsls/vscode'; import { KernelDaemonPool } from '../../client/datascience/kernel-launcher/kernelDaemonPool'; +import { promisify } from 'util'; import { LanguageServerExtensionActivationService } from '../../client/activation/activationService'; import { LanguageServerDownloader } from '../../client/activation/common/downloader'; import { JediExtensionActivator } from '../../client/activation/jedi'; @@ -479,6 +482,18 @@ export class DataScienceIocContainer extends UnitTestIocContainer { } public async dispose(): Promise { + try { + // Make sure to delete any temp files written by native editor storage + const globPr = promisify(glob); + const tempLocation = this.serviceManager.get(IExtensionContext).globalStoragePath; + const tempFiles = await globPr(`${tempLocation}/*.ipynb`); + if (tempFiles && tempFiles.length) { + await Promise.all(tempFiles.map((t) => fs.remove(t))); + } + } catch (exc) { + // tslint:disable-next-line: no-console + console.log(`Exception on cleanup: ${exc}`); + } await this.asyncRegistry.dispose(); await super.dispose(); this.disposed = true; diff --git a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts index c4c508af62a6..1ca47e02850d 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts @@ -11,6 +11,7 @@ import { ConfigurationChangeEvent, ConfigurationTarget, EventEmitter, + FileType, TextEditor, Uri, WorkspaceConfiguration @@ -31,6 +32,7 @@ import { ConfigurationService } from '../../../client/common/configuration/servi import { CryptoUtils } from '../../../client/common/crypto'; import { IFileSystem } from '../../../client/common/platform/types'; import { IConfigurationService, ICryptoUtils, IDisposable, IExtensionContext } from '../../../client/common/types'; +import { sleep } from '../../../client/common/utils/async'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { IEditorContentChange, @@ -87,7 +89,7 @@ class MockWorkspaceConfiguration implements WorkspaceConfiguration { } // tslint:disable: max-func-body-length -suite('Data Science - Native Editor Storage', () => { +suite('DataScience - Native Editor Storage', () => { let workspace: IWorkspaceService; let configService: IConfigurationService; let fileSystem: typemoq.IMock; @@ -321,6 +323,7 @@ suite('Data Science - Native Editor Storage', () => { let listener: IWebPanelMessageListener; const webPanel = mock(WebPanel); + const startTime = Date.now(); class WebPanelCreateMatcher extends Matcher { public match(value: any) { listener = value.listener; @@ -351,16 +354,26 @@ suite('Data Science - Native Editor Storage', () => { .returns((_a1) => { return Promise.resolve(lastWriteFileValue); }); + fileSystem + .setup((f) => f.stat(typemoq.It.isAny())) + .returns((_a1) => { + return Promise.resolve({ mtime: startTime, type: FileType.File, ctime: startTime, size: 100 }); + }); + storage = createStorage(); + }); - storage = new NativeEditorStorage( + function createStorage() { + return new NativeEditorStorage( instance(executionProvider), fileSystem.object, // Use typemoq so can save values in returns instance(crypto), context.object, globalMemento, - localMemento + localMemento, + instance(workspace), + [] ); - }); + } teardown(() => { globalMemento.clear(); @@ -480,6 +493,47 @@ suite('Data Science - Native Editor Storage', () => { expect(cells).to.be.lengthOf(1); }); + test('Editing a file and closing will keep contents', async () => { + await filesConfig?.update('autoSave', 'off'); + + await storage.load(baseUri); + expect(storage.isDirty).to.be.equal(false, 'Editor should not be dirty'); + editCell( + [ + { + range: { + startLineNumber: 2, + startColumn: 1, + endLineNumber: 2, + endColumn: 1 + }, + rangeOffset: 4, + rangeLength: 0, + text: 'a', + position: { + lineNumber: 1, + column: 1 + } + } + ], + storage.cells[1], + 'a' + ); + + // Wait for a second so it has time to update + await sleep(1000); + + // Recreate + storage = createStorage(); + await storage.load(baseUri); + + const cells = storage.cells; + expect(cells).to.be.lengthOf(3); + expect(cells[1].id).to.be.match(/NotebookImport#1/); + expect(concatMultilineStringInput(cells[1].data.source)).to.be.equals('b=2\nab'); + expect(storage.isDirty).to.be.equal(true, 'Editor should be dirty'); + }); + test('Opening file with local storage but no global will still open with old contents', async () => { await filesConfig?.update('autoSave', 'off'); // This test is really for making sure when a user upgrades to a new extension, we still have their old storage diff --git a/src/test/datascience/nativeEditor.functional.test.tsx b/src/test/datascience/nativeEditor.functional.test.tsx index 3b276bd20804..0c11f63a8895 100644 --- a/src/test/datascience/nativeEditor.functional.test.tsx +++ b/src/test/datascience/nativeEditor.functional.test.tsx @@ -83,6 +83,9 @@ import { use(chaiAsPromised); // tslint:disable:max-func-body-length trailing-comma no-any no-multiline-string +async function updateFileConfig(ioc: DataScienceIocContainer, key: string, value: any) { + return ioc.get(IWorkspaceService).getConfiguration('file').update(key, value); +} suite('DataScience Native Editor', () => { const originalPlatform = window.navigator.platform; @@ -641,6 +644,8 @@ df.head()`; runMountedTest( 'RunAllCells', async (wrapper) => { + // Make sure we don't write to storage for the notebook. It messes up other tests + await updateFileConfig(ioc, 'autoSave', 'onFocusChange'); addMockData(ioc, 'print(1)\na=1', 1); addMockData(ioc, 'a=a+1\nprint(a)', 2); addMockData(ioc, 'print(a+1)', 3); @@ -2122,17 +2127,10 @@ df.head()`; await addCell(wrapper, ioc, 'a', false); } - async function updateFileConfig(key: string, value: any) { - return ioc - .get(IWorkspaceService) - .getConfiguration('file') - .update(key, value); - } - test('Auto save notebook every 1s', async () => { // Configure notebook to save automatically ever 1s. - await updateFileConfig('autoSave', 'afterDelay'); - await updateFileConfig('autoSaveDelay', 1_000); + await updateFileConfig(ioc, 'autoSave', 'afterDelay'); + await updateFileConfig(ioc, 'autoSaveDelay', 1_000); ioc.forceSettingsChanged(undefined, ioc.getSettings().pythonPath); /** @@ -2165,8 +2163,8 @@ df.head()`; test('File saved with same format', async () => { // Configure notebook to save automatically ever 1s. - await updateFileConfig('autoSave', 'afterDelay'); - await updateFileConfig('autoSaveDelay', 2_000); + await updateFileConfig(ioc, 'autoSave', 'afterDelay'); + await updateFileConfig(ioc, 'autoSaveDelay', 2_000); ioc.forceSettingsChanged(undefined, ioc.getSettings().pythonPath); const notebookFileContents = await fs.readFile(notebookFile.filePath, 'utf8'); @@ -2190,8 +2188,8 @@ df.head()`; const notebookFileContents = await fs.readFile(notebookFile.filePath, 'utf8'); // Configure notebook to to never save. - await updateFileConfig('autoSave', 'off'); - await updateFileConfig('autoSaveDelay', 1_000); + await updateFileConfig(ioc, 'autoSave', 'off'); + await updateFileConfig(ioc, 'autoSaveDelay', 1_000); // Update the settings and wait for the component to receive it and process it. const promise = waitForMessage(ioc, InteractiveWindowMessages.SettingsUpdated); @@ -2231,7 +2229,7 @@ df.head()`; await dirtyPromise; // Configure notebook to save when active editor changes. - await updateFileConfig('autoSave', 'onFocusChange'); + await updateFileConfig(ioc, 'autoSave', 'onFocusChange'); ioc.forceSettingsChanged(undefined, ioc.getSettings().pythonPath); // Now that the notebook is dirty, change the active editor. @@ -2263,7 +2261,7 @@ df.head()`; await dirtyPromise; // Configure notebook to save when window state changes. - await updateFileConfig('autoSave', 'onWindowChange'); + await updateFileConfig(ioc, 'autoSave', 'onWindowChange'); ioc.forceSettingsChanged(undefined, ioc.getSettings().pythonPath); // Now that the notebook is dirty, change the active editor. @@ -2289,7 +2287,7 @@ df.head()`; await dirtyPromise; // Configure notebook to save when active editor changes. - await updateFileConfig('autoSave', configSetting); + await updateFileConfig(ioc, 'autoSave', configSetting); ioc.forceSettingsChanged(undefined, ioc.getSettings().pythonPath); // Now that the notebook is dirty, send notification about changes to window state. @@ -2321,7 +2319,7 @@ df.head()`; await dirtyPromise; // Configure notebook to save when active editor changes. - await updateFileConfig('autoSave', 'onFocusChange'); + await updateFileConfig(ioc, 'autoSave', 'onFocusChange'); ioc.forceSettingsChanged(undefined, ioc.getSettings().pythonPath); // Force a view state change From 11c0cbfb9ba643714f5d6b1269fbaaa6faf8220e Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Thu, 7 May 2020 22:54:35 -0700 Subject: [PATCH 3/8] Port scrolling fix to release (#11688) * Fix scrolling (#11681) * Fix scrolling * Review feedback - fix scrolling on expand/collapse * Update changelog * Update package.json Co-authored-by: Jim Griesmer --- CHANGELOG.md | 2 ++ package.json | 6 ++++++ src/client/common/types.ts | 1 + .../history-react/interactivePanel.tsx | 4 +++- .../interactive-common/contentPanel.tsx | 17 +++++++++++++++-- 5 files changed, 27 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab8c4a0e0d83..ec36802066db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,8 @@ ([#11579](https://github.com/Microsoft/vscode-python/issues/11579)) 1. When VS quits, make sure to save contents of notebook for next reopen. ([#11557](https://github.com/Microsoft/vscode-python/issues/11557)) +1. Fix scrolling when clicking in the interactive window to not jump around. + ([#11554](https://github.com/Microsoft/vscode-python/issues/11554)) ### Code Health diff --git a/package.json b/package.json index e2d8413913ee..1c8bbf58fb27 100644 --- a/package.json +++ b/package.json @@ -1761,6 +1761,12 @@ "description": "Maximum size (in pixels) of text output in the Python Interactive window and Notebook Editor before a scrollbar appears. First enable scrolling for cell outputs in settings.", "scope": "resource" }, + "python.dataScience.alwaysScrollOnNewCell": { + "type": "boolean", + "default": false, + "description": "Automatically scroll the interactive window to show the output of the last statement executed. If false, the interactive window will only automatically scroll if the bottom of the prior cell is visible.", + "scope": "resource" + }, "python.dataScience.enableScrollingForCellOutputs": { "type": "boolean", "default": true, diff --git a/src/client/common/types.ts b/src/client/common/types.ts index 656af6d5ff22..ff33f32e19f0 100644 --- a/src/client/common/types.ts +++ b/src/client/common/types.ts @@ -389,6 +389,7 @@ export interface IDataScienceSettings { disableJupyterAutoStart?: boolean; jupyterCommandLineArguments: string[]; widgetScriptSources: WidgetCDNs[]; + alwaysScrollOnNewCell?: boolean; } export type WidgetCDNs = 'unpkg.com' | 'jsdelivr.com'; diff --git a/src/datascience-ui/history-react/interactivePanel.tsx b/src/datascience-ui/history-react/interactivePanel.tsx index 66a06e711a9e..0cbec0042ce3 100644 --- a/src/datascience-ui/history-react/interactivePanel.tsx +++ b/src/datascience-ui/history-react/interactivePanel.tsx @@ -373,7 +373,9 @@ ${buildSettingsCss(this.props.settings)}`} if (this.internalScrollCount > 0) { this.internalScrollCount -= 1; } else if (this.contentPanelRef.current) { - const isAtBottom = this.contentPanelRef.current.computeIsAtBottom(e.currentTarget); + const isAtBottom = this.props.settings?.alwaysScrollOnNewCell + ? true + : this.contentPanelRef.current.computeIsAtBottom(e.currentTarget); this.props.scroll(isAtBottom); } }; diff --git a/src/datascience-ui/interactive-common/contentPanel.tsx b/src/datascience-ui/interactive-common/contentPanel.tsx index fba2e3ad5be3..f8653aa48fa4 100644 --- a/src/datascience-ui/interactive-common/contentPanel.tsx +++ b/src/datascience-ui/interactive-common/contentPanel.tsx @@ -3,6 +3,7 @@ 'use strict'; import * as React from 'react'; +import * as fastDeepEqual from 'fast-deep-equal'; import { IDataScienceExtraSettings } from '../../client/datascience/types'; import { InputHistory } from './inputHistory'; import { ICellViewModel } from './mainState'; @@ -37,8 +38,12 @@ export class ContentPanel extends React.Component { public componentDidMount() { this.scrollToBottom(); } - public componentWillReceiveProps() { - this.scrollToBottom(); + public componentWillReceiveProps(prevProps: IContentPanelProps) { + // Scroll if we suddenly finished or updated a cell. This should happen on + // finish, updating output, etc. + if (!fastDeepEqual(prevProps.cellVMs.map(this.outputCheckable), this.props.cellVMs.map(this.outputCheckable))) { + this.scrollToBottom(); + } } public computeIsAtBottom(parent: HTMLDivElement): boolean { @@ -61,6 +66,14 @@ export class ContentPanel extends React.Component { ); } + private outputCheckable = (cellVM: ICellViewModel) => { + // Return the properties that if they change means a cell updated something + return { + outputs: cellVM.cell.data.outputs, + state: cellVM.cell.state + }; + }; + private renderCells = () => { return this.props.cellVMs.map((cellVM: ICellViewModel, index: number) => { return this.props.renderCell(cellVM, index); From ebaba5c9055a1df26be30da9aa80444957a6c00a Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Fri, 8 May 2020 12:39:12 -0700 Subject: [PATCH 4/8] Cherry-pick pipenv changes and pythonpath prompt changes to release (#11700) * Show the prompt again if user clicks on more info (#11664) * Show the prompt again if user clicks on more info * Review feedback * Use Learn more as text for the link. * Leave pipenv in a corner until the user decides to select an interpreter (#11654) * add onSuggestion option * Swap onActivation with onSuggestion * Update unit tests * Remove registration of IPipenvService * Move didTriggerInterpreterSuggestions logic inside pipenv locator * Fix existing unit tests * Add new unit tests * Replace typemoq any param with object * Shorten the tests * Fix warning * Duplicate teardown Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> --- package.nls.json | 6 +- src/client/activation/activationManager.ts | 2 +- .../checks/macPythonInterpreter.ts | 2 +- .../checks/pythonPathDeprecated.ts | 8 - .../pipEnvActivationProvider.ts | 14 +- src/client/common/utils/localize.ts | 6 +- .../interpreter/autoSelection/rules/system.ts | 2 +- .../interpreterSelector.ts | 2 +- src/client/interpreter/contracts.ts | 5 +- src/client/interpreter/locators/index.ts | 21 +- .../services/cacheableLocatorService.ts | 11 + .../locators/services/pipEnvService.ts | 17 +- src/client/interpreter/serviceRegistry.ts | 2 - src/client/interpreter/virtualEnvs/index.ts | 8 +- src/client/startupTelemetry.ts | 4 +- .../activation/activationManager.unit.test.ts | 8 +- .../checks/macPythonInterpreter.unit.test.ts | 8 +- .../checks/pythonPathDeprecated.unit.test.ts | 16 +- .../interpreterSelector.unit.test.ts | 6 +- .../datascience/dataScienceIocContainer.ts | 2 - .../rules/currentPath.unit.test.ts | 2 +- .../autoSelection/rules/system.unit.test.ts | 15 +- .../interpreters/locators/index.unit.test.ts | 31 +- .../interpreters/pipEnvService.unit.test.ts | 329 ++++++++++-------- .../interpreters/serviceRegistry.unit.test.ts | 2 - .../virtualEnvManager.unit.test.ts | 12 +- .../virtualEnvs/index.unit.test.ts | 11 +- src/test/serviceRegistry.ts | 2 - 28 files changed, 315 insertions(+), 239 deletions(-) diff --git a/package.nls.json b/package.nls.json index 77622a77439c..46aa8b8583e5 100644 --- a/package.nls.json +++ b/package.nls.json @@ -240,9 +240,9 @@ "DataScience.liveShareServiceFailure": "Failure starting '{0}' service during live share connection.", "DataScience.documentMismatch": "Cannot run cells, duplicate documents for {0} found.", "DataScience.pythonInteractiveCreateFailed": "Failure to create a 'Python Interactive' window. Try reinstalling the Python extension.", - "diagnostics.removePythonPathSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the \"python.pythonPath\" entry from your settings.json.", - "diagnostics.removePythonPathCodeWorkspace": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the \"python.pythonPath\" entry from your .code-workspace file.", - "diagnostics.removePythonPathCodeWorkspaceAndSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the \"python.pythonPath\" entry from your settings.json and .code-workspace file.", + "diagnostics.removePythonPathSettingsJson": "The setting \"python.pythonPath\" defined in your settings.json is now deprecated. Do you want us to delete it from your settings.json only? [Learn more](https://aka.ms/AA7jfor).", + "diagnostics.removePythonPathCodeWorkspace": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file only? [Learn more](https://aka.ms/AA7jfor).", + "diagnostics.removePythonPathCodeWorkspaceAndSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? [Learn more](https://aka.ms/AA7jfor).", "diagnostics.warnSourceMaps": "Source map support is enabled in the Python Extension, this will adversely impact performance of the extension.", "diagnostics.disableSourceMaps": "Disable Source Map Support", "diagnostics.warnBeforeEnablingSourceMaps": "Enabling source map support in the Python Extension will adversely impact performance of the extension.", diff --git a/src/client/activation/activationManager.ts b/src/client/activation/activationManager.ts index 4a5333c6111b..f570eb05aa3a 100644 --- a/src/client/activation/activationManager.ts +++ b/src/client/activation/activationManager.ts @@ -73,7 +73,7 @@ export class ExtensionActivationManager implements IExtensionActivationManager { this.experiments.sendTelemetryIfInExperiment(DeprecatePythonPath.control); // Get latest interpreter list in the background. - this.interpreterService.getInterpreters(resource, { onActivation: true }).ignoreErrors(); + this.interpreterService.getInterpreters(resource).ignoreErrors(); await sendActivationTelemetry(this.fileSystem, this.workspaceService, resource); diff --git a/src/client/application/diagnostics/checks/macPythonInterpreter.ts b/src/client/application/diagnostics/checks/macPythonInterpreter.ts index 9515c7b76928..26344f41a607 100644 --- a/src/client/application/diagnostics/checks/macPythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/macPythonInterpreter.ts @@ -101,7 +101,7 @@ export class InvalidMacPythonInterpreterService extends BaseDiagnosticsService { return []; } - const interpreters = await this.interpreterService.getInterpreters(resource, { onActivation: true }); + const interpreters = await this.interpreterService.getInterpreters(resource); if (interpreters.filter((i) => !this.helper.isMacDefaultPythonPath(i.path)).length === 0) { return [ new InvalidMacPythonInterpreterDiagnostic( diff --git a/src/client/application/diagnostics/checks/pythonPathDeprecated.ts b/src/client/application/diagnostics/checks/pythonPathDeprecated.ts index 6ba46dd11042..80dd0d5b0cbc 100644 --- a/src/client/application/diagnostics/checks/pythonPathDeprecated.ts +++ b/src/client/application/diagnostics/checks/pythonPathDeprecated.ts @@ -9,7 +9,6 @@ import { IWorkspaceService } from '../../../common/application/types'; import { DeprecatePythonPath } from '../../../common/experimentGroups'; import { IDisposableRegistry, IExperimentsManager, Resource } from '../../../common/types'; import { Common, Diagnostics } from '../../../common/utils/localize'; -import { learnMoreOnInterpreterSecurityURI } from '../../../interpreter/autoSelection/constants'; import { IServiceContainer } from '../../../ioc/types'; import { BaseDiagnostic, BaseDiagnosticsService } from '../base'; import { IDiagnosticsCommandFactory } from '../commands/types'; @@ -97,13 +96,6 @@ export class PythonPathDeprecatedDiagnosticService extends BaseDiagnosticsServic { prompt: Common.noIWillDoItLater() }, - { - prompt: Common.moreInfo(), - command: commandFactory.createCommand(diagnostic, { - type: 'launch', - options: learnMoreOnInterpreterSecurityURI - }) - }, { prompt: Common.doNotShowAgain(), command: commandFactory.createCommand(diagnostic, { type: 'ignore', options: DiagnosticScope.Global }) diff --git a/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts b/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts index a25862ebd007..4b0ff07129fd 100644 --- a/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts +++ b/src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts @@ -3,10 +3,16 @@ 'use strict'; -import { inject, injectable } from 'inversify'; +import { inject, injectable, named } from 'inversify'; import { Uri } from 'vscode'; import '../../../common/extensions'; -import { IInterpreterService, InterpreterType, IPipEnvService } from '../../../interpreter/contracts'; +import { + IInterpreterLocatorService, + IInterpreterService, + InterpreterType, + IPipEnvService, + PIPENV_SERVICE +} from '../../../interpreter/contracts'; import { IWorkspaceService } from '../../application/types'; import { IFileSystem } from '../../platform/types'; import { ITerminalActivationCommandProvider, TerminalShellType } from '../types'; @@ -15,7 +21,9 @@ import { ITerminalActivationCommandProvider, TerminalShellType } from '../types' export class PipEnvActivationCommandProvider implements ITerminalActivationCommandProvider { constructor( @inject(IInterpreterService) private readonly interpreterService: IInterpreterService, - @inject(IPipEnvService) private readonly pipenvService: IPipEnvService, + @inject(IInterpreterLocatorService) + @named(PIPENV_SERVICE) + private readonly pipenvService: IPipEnvService, @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(IFileSystem) private readonly fs: IFileSystem ) {} diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 526cd0df6035..946eb7d09adc 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -32,15 +32,15 @@ export namespace Diagnostics { ); export const removePythonPathSettingsJson = localize( 'diagnostics.removePythonPathSettingsJson', - 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your settings.json.' + 'The setting "python.pythonPath" defined in your settings.json is now deprecated. Do you want us to delete it from your settings.json only? [Learn more](https://aka.ms/AA7jfor).' ); export const removePythonPathCodeWorkspace = localize( 'diagnostics.removePythonPathCodeWorkspace', - 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your .code-workspace file.' + 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file only? [Learn more](https://aka.ms/AA7jfor).' ); export const removePythonPathCodeWorkspaceAndSettingsJson = localize( 'diagnostics.removePythonPathCodeWorkspaceAndSettingsJson', - 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your settings.json and .code-workspace file.' + 'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? [Learn more](https://aka.ms/AA7jfor).' ); export const invalidPythonPathInDebuggerSettings = localize( 'diagnostics.invalidPythonPathInDebuggerSettings', diff --git a/src/client/interpreter/autoSelection/rules/system.ts b/src/client/interpreter/autoSelection/rules/system.ts index 6785d83a9fa8..9feb4925b9a4 100644 --- a/src/client/interpreter/autoSelection/rules/system.ts +++ b/src/client/interpreter/autoSelection/rules/system.ts @@ -25,7 +25,7 @@ export class SystemWideInterpretersAutoSelectionRule extends BaseRuleService { resource: Resource, manager?: IInterpreterAutoSelectionService ): Promise { - const interpreters = await this.interpreterService.getInterpreters(resource, { onActivation: true }); + const interpreters = await this.interpreterService.getInterpreters(resource); // Exclude non-local interpreters. const filteredInterpreters = interpreters.filter( (int) => diff --git a/src/client/interpreter/configuration/interpreterSelector/interpreterSelector.ts b/src/client/interpreter/configuration/interpreterSelector/interpreterSelector.ts index c328b1a58ad0..f7c95fb7e0e1 100644 --- a/src/client/interpreter/configuration/interpreterSelector/interpreterSelector.ts +++ b/src/client/interpreter/configuration/interpreterSelector/interpreterSelector.ts @@ -27,7 +27,7 @@ export class InterpreterSelector implements IInterpreterSelector { } public async getSuggestions(resource: Resource) { - let interpreters = await this.interpreterManager.getInterpreters(resource); + let interpreters = await this.interpreterManager.getInterpreters(resource, { onSuggestion: true }); if (this.experimentsManager.inExperiment(DeprecatePythonPath.experiment)) { interpreters = interpreters.filter((item) => this.interpreterSecurityService.isSafe(item) !== false); } diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index cfea89200a91..6013ff504bd2 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -28,7 +28,7 @@ export interface IVirtualEnvironmentsSearchPathProvider { } export type GetInterpreterOptions = { - onActivation?: boolean; + onSuggestion?: boolean; }; export type GetInterpreterLocatorOptions = GetInterpreterOptions & { ignoreCache?: boolean }; @@ -38,6 +38,7 @@ export const IInterpreterLocatorService = Symbol('IInterpreterLocatorService'); export interface IInterpreterLocatorService extends Disposable { readonly onLocating: Event>; readonly hasInterpreters: Promise; + didTriggerInterpreterSuggestions?: boolean; getInterpreters(resource?: Uri, options?: GetInterpreterLocatorOptions): Promise; } @@ -126,7 +127,7 @@ export interface IInterpreterHelper { } export const IPipEnvService = Symbol('IPipEnvService'); -export interface IPipEnvService { +export interface IPipEnvService extends IInterpreterLocatorService { executable: string; isRelatedPipEnvironment(dir: string, pythonPath: string): Promise; } diff --git a/src/client/interpreter/locators/index.ts b/src/client/interpreter/locators/index.ts index 76b3443c4a80..dc65eead948f 100644 --- a/src/client/interpreter/locators/index.ts +++ b/src/client/interpreter/locators/index.ts @@ -30,10 +30,13 @@ const flatten = require('lodash/flatten') as typeof import('lodash/flatten'); */ @injectable() export class PythonInterpreterLocatorService implements IInterpreterLocatorService { + public didTriggerInterpreterSuggestions: boolean; + private readonly disposables: Disposable[] = []; private readonly platform: IPlatformService; private readonly interpreterLocatorHelper: IInterpreterLocatorHelper; private readonly _hasInterpreters: Deferred; + constructor( @inject(IServiceContainer) private serviceContainer: IServiceContainer, @inject(InterpreterFilter) private readonly interpreterFilter: IInterpreterFilter @@ -42,6 +45,7 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi serviceContainer.get(IDisposableRegistry).push(this); this.platform = serviceContainer.get(IPlatformService); this.interpreterLocatorHelper = serviceContainer.get(IInterpreterLocatorHelper); + this.didTriggerInterpreterSuggestions = false; } /** * This class should never emit events when we're locating. @@ -106,18 +110,27 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi // The order is important because the data sources at the bottom of the list do not contain all, // the information about the interpreters (e.g. type, environment name, etc). // This way, the items returned from the top of the list will win, when we combine the items returned. - const keys: [string | undefined, OSType | undefined][] = [ + const keys: [string, OSType | undefined][] = [ [WINDOWS_REGISTRY_SERVICE, OSType.Windows], [CONDA_ENV_SERVICE, undefined], [CONDA_ENV_FILE_SERVICE, undefined], - options?.onActivation ? [undefined, undefined] : [PIPENV_SERVICE, undefined], + [PIPENV_SERVICE, undefined], [GLOBAL_VIRTUAL_ENV_SERVICE, undefined], [WORKSPACE_VIRTUAL_ENV_SERVICE, undefined], [KNOWN_PATH_SERVICE, undefined], [CURRENT_PATH_SERVICE, undefined] ]; - return keys - .filter((item) => item[0] !== undefined && (item[1] === undefined || item[1] === this.platform.osType)) + + const locators = keys + .filter((item) => item[1] === undefined || item[1] === this.platform.osType) .map((item) => this.serviceContainer.get(IInterpreterLocatorService, item[0])); + + // Set it to true the first time the user selects an interpreter + if (!this.didTriggerInterpreterSuggestions && options?.onSuggestion === true) { + this.didTriggerInterpreterSuggestions = true; + locators.forEach((locator) => (locator.didTriggerInterpreterSuggestions = true)); + } + + return locators; } } diff --git a/src/client/interpreter/locators/services/cacheableLocatorService.ts b/src/client/interpreter/locators/services/cacheableLocatorService.ts index 320b4c9a21b9..440204780fca 100644 --- a/src/client/interpreter/locators/services/cacheableLocatorService.ts +++ b/src/client/interpreter/locators/services/cacheableLocatorService.ts @@ -70,6 +70,8 @@ export abstract class CacheableLocatorService implements IInterpreterLocatorServ private readonly handlersAddedToResource = new Set(); private readonly cacheKeyPrefix: string; private readonly locating = new EventEmitter>(); + private _didTriggerInterpreterSuggestions: boolean; + constructor( @unmanaged() private readonly name: string, @unmanaged() protected readonly serviceContainer: IServiceContainer, @@ -77,6 +79,15 @@ export abstract class CacheableLocatorService implements IInterpreterLocatorServ ) { this._hasInterpreters = createDeferred(); this.cacheKeyPrefix = `INTERPRETERS_CACHE_v3_${name}`; + this._didTriggerInterpreterSuggestions = false; + } + + public get didTriggerInterpreterSuggestions(): boolean { + return this._didTriggerInterpreterSuggestions; + } + + public set didTriggerInterpreterSuggestions(value: boolean) { + this._didTriggerInterpreterSuggestions = value; } public get onLocating(): Event> { diff --git a/src/client/interpreter/locators/services/pipEnvService.ts b/src/client/interpreter/locators/services/pipEnvService.ts index 55ae9e09738a..f0459c51806b 100644 --- a/src/client/interpreter/locators/services/pipEnvService.ts +++ b/src/client/interpreter/locators/services/pipEnvService.ts @@ -43,9 +43,15 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer this.configService = this.serviceContainer.get(IConfigurationService); this.pipEnvServiceHelper = this.serviceContainer.get(IPipEnvServiceHelper); } + // tslint:disable-next-line:no-empty public dispose() {} + public async isRelatedPipEnvironment(dir: string, pythonPath: string): Promise { + if (!this.didTriggerInterpreterSuggestions) { + return false; + } + // In PipEnv, the name of the cwd is used as a prefix in the virtual env. if (pythonPath.indexOf(`${path.sep}${path.basename(dir)}-`) === -1) { return false; @@ -55,10 +61,14 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer } public get executable(): string { - return this.configService.getSettings().pipenvPath; + return this.didTriggerInterpreterSuggestions ? this.configService.getSettings().pipenvPath : ''; } public async getInterpreters(resource?: Uri, options?: GetInterpreterLocatorOptions): Promise { + if (!this.didTriggerInterpreterSuggestions) { + return []; + } + const stopwatch = new StopWatch(); const startDiscoveryTime = stopwatch.elapsedTime; @@ -71,6 +81,10 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer } protected getInterpretersImplementation(resource?: Uri): Promise { + if (!this.didTriggerInterpreterSuggestions) { + return Promise.resolve([]); + } + const pipenvCwd = this.getPipenvWorkingDirectory(resource); if (!pipenvCwd) { return Promise.resolve([]); @@ -146,6 +160,7 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer } } } + private async checkIfPipFileExists(cwd: string): Promise { const currentProcess = this.serviceContainer.get(ICurrentProcess); const pipFileName = currentProcess.env[pipEnvFileNameVariable]; diff --git a/src/client/interpreter/serviceRegistry.ts b/src/client/interpreter/serviceRegistry.ts index 7c01b14eb03a..d4602eb61363 100644 --- a/src/client/interpreter/serviceRegistry.ts +++ b/src/client/interpreter/serviceRegistry.ts @@ -60,7 +60,6 @@ import { IInterpreterWatcherBuilder, IKnownSearchPathsForInterpreters, INTERPRETER_LOCATOR_SERVICE, - IPipEnvService, IShebangCodeLensProvider, IVirtualEnvironmentsSearchPathProvider, KNOWN_PATH_SERVICE, @@ -200,7 +199,6 @@ export function registerInterpreterTypes(serviceManager: IServiceManager) { WORKSPACE_VIRTUAL_ENV_SERVICE ); serviceManager.addSingleton(IInterpreterLocatorService, PipEnvService, PIPENV_SERVICE); - serviceManager.addSingleton(IPipEnvService, PipEnvService); serviceManager.addSingleton( IInterpreterLocatorService, diff --git a/src/client/interpreter/virtualEnvs/index.ts b/src/client/interpreter/virtualEnvs/index.ts index fb3f7940ef7b..34778820d6f2 100644 --- a/src/client/interpreter/virtualEnvs/index.ts +++ b/src/client/interpreter/virtualEnvs/index.ts @@ -12,7 +12,7 @@ import { ICurrentProcess, IPathUtils } from '../../common/types'; import { getNamesAndValues } from '../../common/utils/enum'; import { noop } from '../../common/utils/misc'; import { IServiceContainer } from '../../ioc/types'; -import { InterpreterType, IPipEnvService } from '../contracts'; +import { IInterpreterLocatorService, InterpreterType, IPipEnvService, PIPENV_SERVICE } from '../contracts'; import { IVirtualEnvironmentManager } from './types'; const PYENVFILES = ['pyvenv.cfg', path.join('..', 'pyvenv.cfg')]; @@ -27,7 +27,10 @@ export class VirtualEnvironmentManager implements IVirtualEnvironmentManager { constructor(@inject(IServiceContainer) private readonly serviceContainer: IServiceContainer) { this.processServiceFactory = serviceContainer.get(IProcessServiceFactory); this.fs = serviceContainer.get(IFileSystem); - this.pipEnvService = serviceContainer.get(IPipEnvService); + this.pipEnvService = serviceContainer.get( + IInterpreterLocatorService, + PIPENV_SERVICE + ) as IPipEnvService; this.workspaceService = serviceContainer.get(IWorkspaceService); } public async getEnvironmentName(pythonPath: string, resource?: Uri): Promise { @@ -37,6 +40,7 @@ export class VirtualEnvironmentManager implements IVirtualEnvironmentManager { const workspaceFolder = resource ? this.workspaceService.getWorkspaceFolder(resource) : undefined; const workspaceUri = workspaceFolder ? workspaceFolder.uri : defaultWorkspaceUri; const grandParentDirName = path.basename(path.dirname(path.dirname(pythonPath))); + if (workspaceUri && (await this.pipEnvService.isRelatedPipEnvironment(workspaceUri.fsPath, pythonPath))) { // In pipenv, return the folder name of the workspace. return path.basename(workspaceUri.fsPath); diff --git a/src/client/startupTelemetry.ts b/src/client/startupTelemetry.ts index e5cc8b03c57a..c84e78c10837 100644 --- a/src/client/startupTelemetry.ts +++ b/src/client/startupTelemetry.ts @@ -129,9 +129,7 @@ async function getActivationTelemetryProps(serviceContainer: IServiceContainer): .then((ver) => (ver ? ver.raw : '')) .catch(() => ''), interpreterService.getActiveInterpreter().catch(() => undefined), - interpreterService - .getInterpreters(mainWorkspaceUri, { onActivation: true }) - .catch(() => []) + interpreterService.getInterpreters(mainWorkspaceUri).catch(() => []) ]); const workspaceFolderCount = workspaceService.hasWorkspaceFolders ? workspaceService.workspaceFolders!.length : 0; const pythonVersion = interpreter && interpreter.version ? interpreter.version.raw : undefined; diff --git a/src/test/activation/activationManager.unit.test.ts b/src/test/activation/activationManager.unit.test.ts index 0dff26f9e6ed..43a14e4978c0 100644 --- a/src/test/activation/activationManager.unit.test.ts +++ b/src/test/activation/activationManager.unit.test.ts @@ -198,7 +198,7 @@ suite('Activation Manager', () => { when(workspaceService.getWorkspaceFolder(resource)).thenReturn(folder2); when(activationService1.activate(resource)).thenResolve(); when(activationService2.activate(resource)).thenResolve(); - when(interpreterService.getInterpreters(anything(), anything())).thenResolve(); + when(interpreterService.getInterpreters(anything())).thenResolve(); autoSelection .setup((a) => a.autoSelectInterpreter(resource)) .returns(() => Promise.resolve()) @@ -232,7 +232,7 @@ suite('Activation Manager', () => { const resource = Uri.parse('two'); when(activationService1.activate(resource)).thenResolve(); when(activationService2.activate(resource)).thenResolve(); - when(interpreterService.getInterpreters(anything(), anything())).thenResolve(); + when(interpreterService.getInterpreters(anything())).thenResolve(); autoSelection .setup((a) => a.autoSelectInterpreter(resource)) .returns(() => Promise.resolve()) @@ -252,7 +252,7 @@ suite('Activation Manager', () => { const resource = Uri.parse('two'); when(activationService1.activate(resource)).thenResolve(); when(activationService2.activate(resource)).thenResolve(); - when(interpreterService.getInterpreters(anything(), anything())).thenResolve(); + when(interpreterService.getInterpreters(anything())).thenResolve(); when(experiments.inExperiment(DeprecatePythonPath.experiment)).thenReturn(true); interpreterPathService .setup((i) => i.copyOldInterpreterStorageValuesToNew(resource)) @@ -278,7 +278,7 @@ suite('Activation Manager', () => { const resource = Uri.parse('two'); when(activationService1.activate(resource)).thenResolve(); when(activationService2.activate(resource)).thenResolve(); - when(interpreterService.getInterpreters(anything(), anything())).thenResolve(); + when(interpreterService.getInterpreters(anything())).thenResolve(); autoSelection .setup((a) => a.autoSelectInterpreter(resource)) .returns(() => Promise.resolve()) diff --git a/src/test/application/diagnostics/checks/macPythonInterpreter.unit.test.ts b/src/test/application/diagnostics/checks/macPythonInterpreter.unit.test.ts index dfaad331374f..eaa9315341d3 100644 --- a/src/test/application/diagnostics/checks/macPythonInterpreter.unit.test.ts +++ b/src/test/application/diagnostics/checks/macPythonInterpreter.unit.test.ts @@ -174,7 +174,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => { .returns(() => Promise.resolve(true)) .verifiable(typemoq.Times.once()); interpreterService - .setup((i) => i.getInterpreters(typemoq.It.isAny(), { onActivation: true })) + .setup((i) => i.getInterpreters(typemoq.It.isAny())) .returns(() => Promise.resolve([{} as any])) .verifiable(typemoq.Times.never()); interpreterService @@ -204,7 +204,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => { .returns(() => Promise.resolve(true)) .verifiable(typemoq.Times.once()); interpreterService - .setup((i) => i.getInterpreters(typemoq.It.isAny(), { onActivation: true })) + .setup((i) => i.getInterpreters(typemoq.It.isAny())) .returns(() => Promise.resolve([{} as any])) .verifiable(typemoq.Times.never()); interpreterService @@ -235,7 +235,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => { .returns(() => false) .verifiable(typemoq.Times.once()); interpreterService - .setup((i) => i.getInterpreters(typemoq.It.isAny(), { onActivation: true })) + .setup((i) => i.getInterpreters(typemoq.It.isAny())) .returns(() => Promise.resolve([{ path: pythonPath } as any, { path: pythonPath } as any])) .verifiable(typemoq.Times.once()); interpreterService @@ -275,7 +275,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => { .returns(() => false) .verifiable(typemoq.Times.once()); interpreterService - .setup((i) => i.getInterpreters(typemoq.It.isAny(), { onActivation: true })) + .setup((i) => i.getInterpreters(typemoq.It.isAny())) .returns(() => Promise.resolve([ { path: pythonPath } as any, diff --git a/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts b/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts index 05bfaa95d6a7..014bf92eff1e 100644 --- a/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts +++ b/src/test/application/diagnostics/checks/pythonPathDeprecated.unit.test.ts @@ -142,7 +142,6 @@ suite('Application Diagnostics - Python Path Deprecated', () => { }); test('Python Path Deprecated Diagnostic is handled as expected', async () => { const diagnostic = new PythonPathDeprecatedDiagnostic('message', resource); - const launchCmd = ({ cmd: 'launchCmd' } as any) as IDiagnosticCommand; const ignoreCmd = ({ cmd: 'ignoreCmd' } as any) as IDiagnosticCommand; filterService .setup((f) => @@ -155,15 +154,6 @@ suite('Application Diagnostics - Python Path Deprecated', () => { .callback((_d, p: MessageCommandPrompt) => (messagePrompt = p)) .returns(() => Promise.resolve()) .verifiable(typemoq.Times.once()); - commandFactory - .setup((f) => - f.createCommand( - typemoq.It.isAny(), - typemoq.It.isObjectWith>({ type: 'launch' }) - ) - ) - .returns(() => launchCmd) - .verifiable(typemoq.Times.once()); commandFactory .setup((f) => @@ -180,16 +170,12 @@ suite('Application Diagnostics - Python Path Deprecated', () => { messageHandler.verifyAll(); commandFactory.verifyAll(); expect(messagePrompt).not.be.equal(undefined, 'Message prompt not set'); - expect(messagePrompt!.commandPrompts.length).to.equal(4, 'Incorrect length'); + expect(messagePrompt!.commandPrompts.length).to.equal(3, 'Incorrect length'); expect(messagePrompt!.commandPrompts[0].command).not.be.equal(undefined, 'Command not set'); expect(messagePrompt!.commandPrompts[0].command!.diagnostic).to.be.deep.equal(diagnostic); expect(messagePrompt!.commandPrompts[0].prompt).to.be.deep.equal(Common.yesPlease()); expect(messagePrompt!.commandPrompts[1]).to.be.deep.equal({ prompt: Common.noIWillDoItLater() }); expect(messagePrompt!.commandPrompts[2]).to.be.deep.equal({ - prompt: Common.moreInfo(), - command: launchCmd - }); - expect(messagePrompt!.commandPrompts[3]).to.be.deep.equal({ prompt: Common.doNotShowAgain(), command: ignoreCmd }); diff --git a/src/test/configuration/interpreterSelector/interpreterSelector.unit.test.ts b/src/test/configuration/interpreterSelector/interpreterSelector.unit.test.ts index 006634c5b2f8..0410a29dac78 100644 --- a/src/test/configuration/interpreterSelector/interpreterSelector.unit.test.ts +++ b/src/test/configuration/interpreterSelector/interpreterSelector.unit.test.ts @@ -106,7 +106,7 @@ suite('Interpreters - selector', () => { return { ...info, ...item }; }); interpreterService - .setup((x) => x.getInterpreters(TypeMoq.It.isAny())) + .setup((x) => x.getInterpreters(TypeMoq.It.isAny(), { onSuggestion: true })) .returns(() => new Promise((resolve) => resolve(initial))); const actual = await selector.getSuggestions(undefined); @@ -139,7 +139,9 @@ suite('Interpreters - selector', () => { test('When in Deprecate PythonPath experiment, remove unsafe interpreters from the suggested interpreters list', async () => { // tslint:disable-next-line: no-any const interpreterList = ['interpreter1', 'interpreter2', 'interpreter3'] as any; - interpreterService.setup((i) => i.getInterpreters(folder1.uri)).returns(() => interpreterList); + interpreterService + .setup((i) => i.getInterpreters(folder1.uri, { onSuggestion: true })) + .returns(() => interpreterList); // tslint:disable-next-line: no-any interpreterSecurityService.setup((i) => i.isSafe('interpreter1' as any)).returns(() => true); // tslint:disable-next-line: no-any diff --git a/src/test/datascience/dataScienceIocContainer.ts b/src/test/datascience/dataScienceIocContainer.ts index 68e2bc003473..5ba7cc96afc7 100644 --- a/src/test/datascience/dataScienceIocContainer.ts +++ b/src/test/datascience/dataScienceIocContainer.ts @@ -327,7 +327,6 @@ import { IKnownSearchPathsForInterpreters, INTERPRETER_LOCATOR_SERVICE, InterpreterType, - IPipEnvService, IShebangCodeLensProvider, IVirtualEnvironmentsSearchPathProvider, KNOWN_PATH_SERVICE, @@ -1065,7 +1064,6 @@ export class DataScienceIocContainer extends UnitTestIocContainer { PipEnvService, PIPENV_SERVICE ); - this.serviceManager.addSingleton(IPipEnvService, PipEnvService); this.serviceManager.addSingleton( IInterpreterLocatorService, WindowsRegistryService, diff --git a/src/test/interpreters/autoSelection/rules/currentPath.unit.test.ts b/src/test/interpreters/autoSelection/rules/currentPath.unit.test.ts index ca3d3b599d06..1215ca88af70 100644 --- a/src/test/interpreters/autoSelection/rules/currentPath.unit.test.ts +++ b/src/test/interpreters/autoSelection/rules/currentPath.unit.test.ts @@ -69,7 +69,7 @@ suite('Interpreters - Auto Selection - Current Path Rule', () => { const manager = mock(InterpreterAutoSelectionService); const resource = Uri.file('x'); - when(locator.getInterpreters(resource, anything())).thenResolve([]); + when(locator.getInterpreters(resource)).thenResolve([]); const nextAction = await rule.onAutoSelectInterpreter(resource, manager); diff --git a/src/test/interpreters/autoSelection/rules/system.unit.test.ts b/src/test/interpreters/autoSelection/rules/system.unit.test.ts index a18baf5d82cf..d5ccb4173cd4 100644 --- a/src/test/interpreters/autoSelection/rules/system.unit.test.ts +++ b/src/test/interpreters/autoSelection/rules/system.unit.test.ts @@ -64,9 +64,8 @@ suite('Interpreters - Auto Selection - System Interpreters Rule', () => { test('Invoke next rule if there are no interpreters in the current path', async () => { const manager = mock(InterpreterAutoSelectionService); const resource = Uri.file('x'); - const options = { onActivation: true }; let setGlobalInterpreterInvoked = false; - when(interpreterService.getInterpreters(resource, deepEqual(options))).thenResolve([]); + when(interpreterService.getInterpreters(resource)).thenResolve([]); when(helper.getBestInterpreter(deepEqual([]))).thenReturn(undefined); rule.setGlobalInterpreter = async (res: any) => { setGlobalInterpreterInvoked = true; @@ -76,17 +75,16 @@ suite('Interpreters - Auto Selection - System Interpreters Rule', () => { const nextAction = await rule.onAutoSelectInterpreter(resource, manager); - verify(interpreterService.getInterpreters(resource, deepEqual(options))).once(); + verify(interpreterService.getInterpreters(resource)).once(); expect(nextAction).to.be.equal(NextAction.runNextRule); expect(setGlobalInterpreterInvoked).to.be.equal(true, 'setGlobalInterpreter not invoked'); }); test('Invoke next rule if there interpreters in the current path but update fails', async () => { const manager = mock(InterpreterAutoSelectionService); const resource = Uri.file('x'); - const options = { onActivation: true }; let setGlobalInterpreterInvoked = false; const interpreterInfo = { path: '1', version: new SemVer('1.0.0') } as any; - when(interpreterService.getInterpreters(resource, deepEqual(options))).thenResolve([interpreterInfo]); + when(interpreterService.getInterpreters(resource)).thenResolve([interpreterInfo]); when(helper.getBestInterpreter(deepEqual([interpreterInfo]))).thenReturn(interpreterInfo); rule.setGlobalInterpreter = async (res: any) => { setGlobalInterpreterInvoked = true; @@ -96,17 +94,16 @@ suite('Interpreters - Auto Selection - System Interpreters Rule', () => { const nextAction = await rule.onAutoSelectInterpreter(resource, manager); - verify(interpreterService.getInterpreters(resource, deepEqual(options))).once(); + verify(interpreterService.getInterpreters(resource)).once(); expect(nextAction).to.be.equal(NextAction.runNextRule); expect(setGlobalInterpreterInvoked).to.be.equal(true, 'setGlobalInterpreter not invoked'); }); test('Do not Invoke next rule if there interpreters in the current path and update does not fail', async () => { const manager = mock(InterpreterAutoSelectionService); const resource = Uri.file('x'); - const options = { onActivation: true }; let setGlobalInterpreterInvoked = false; const interpreterInfo = { path: '1', version: new SemVer('1.0.0') } as any; - when(interpreterService.getInterpreters(resource, deepEqual(options))).thenResolve([interpreterInfo]); + when(interpreterService.getInterpreters(resource)).thenResolve([interpreterInfo]); when(helper.getBestInterpreter(deepEqual([interpreterInfo]))).thenReturn(interpreterInfo); rule.setGlobalInterpreter = async (res: any) => { setGlobalInterpreterInvoked = true; @@ -116,7 +113,7 @@ suite('Interpreters - Auto Selection - System Interpreters Rule', () => { const nextAction = await rule.onAutoSelectInterpreter(resource, manager); - verify(interpreterService.getInterpreters(resource, deepEqual(options))).once(); + verify(interpreterService.getInterpreters(resource)).once(); expect(nextAction).to.be.equal(NextAction.exit); expect(setGlobalInterpreterInvoked).to.be.equal(true, 'setGlobalInterpreter not invoked'); }); diff --git a/src/test/interpreters/locators/index.unit.test.ts b/src/test/interpreters/locators/index.unit.test.ts index 669a2386a205..c4c0fac23ae9 100644 --- a/src/test/interpreters/locators/index.unit.test.ts +++ b/src/test/interpreters/locators/index.unit.test.ts @@ -90,6 +90,7 @@ suite('Interpreters - Locators Index', () => { .setup((l) => l.hasInterpreters) .returns(() => Promise.resolve(true)) .verifiable(TypeMoq.Times.once()); + typeLocator .setup((l) => l.getInterpreters(TypeMoq.It.isValue(resource))) .returns(() => Promise.resolve([interpreter])) @@ -152,6 +153,7 @@ suite('Interpreters - Locators Index', () => { .setup((l) => l.hasInterpreters) .returns(() => Promise.resolve(true)) .verifiable(TypeMoq.Times.once()); + typeLocator .setup((l) => l.getInterpreters(TypeMoq.It.isValue(resource))) .returns(() => Promise.resolve([interpreter])) @@ -171,6 +173,7 @@ suite('Interpreters - Locators Index', () => { }); const expectedInterpreters = locatorsWithInterpreters.map((item) => item.interpreters[0]); + helper .setup((h) => h.mergeInterpreters(TypeMoq.It.isAny())) .returns(() => Promise.resolve(expectedInterpreters)) @@ -180,11 +183,9 @@ suite('Interpreters - Locators Index', () => { locatorsWithInterpreters.forEach((item) => item.locator.verifyAll()); helper.verifyAll(); - expect(interpreters).to.be.lengthOf(locatorsTypes.length); expect(interpreters).to.be.deep.equal(expectedInterpreters); }); - - test(`The pipenv locator service isn't used when the onActivation option is true ${testSuffix}`, async () => { + test(`didTriggerInterpreterSuggestions is set to true in the locators if onSuggestion is true ${testSuffix}`, async () => { const locatorsTypes: string[] = []; if (osType.value === OSType.Windows) { locatorsTypes.push(WINDOWS_REGISTRY_SERVICE); @@ -218,6 +219,7 @@ suite('Interpreters - Locators Index', () => { .setup((l) => l.hasInterpreters) .returns(() => Promise.resolve(true)) .verifiable(TypeMoq.Times.once()); + typeLocator .setup((l) => l.getInterpreters(TypeMoq.It.isValue(resource))) .returns(() => Promise.resolve([interpreter])) @@ -236,24 +238,19 @@ suite('Interpreters - Locators Index', () => { }; }); - const expectedInterpreters: PythonInterpreter[] = []; - locatorsWithInterpreters.forEach((item) => { - if (item.type !== PIPENV_SERVICE) { - expectedInterpreters.push(item.interpreters[0]); - } - }); - - // const expectedInterpreters = locatorsWithInterpreters.map((item) => item.interpreters[0]); helper .setup((h) => h.mergeInterpreters(TypeMoq.It.isAny())) - .returns(() => Promise.resolve(expectedInterpreters)) - .verifiable(TypeMoq.Times.once()); + .returns(() => Promise.resolve(locatorsWithInterpreters.map((item) => item.interpreters[0]))); - const interpreters = await locator.getInterpreters(resource, { onActivation: false }); + await locator.getInterpreters(resource, { onSuggestion: true }); - locatorsWithInterpreters.forEach((item) => item.locator.verifyAll()); - helper.verifyAll(); - expect(interpreters).to.be.deep.equal(expectedInterpreters); + locatorsWithInterpreters.forEach((item) => + item.locator.verify((l) => (l.didTriggerInterpreterSuggestions = true), TypeMoq.Times.once()) + ); + expect(locator.didTriggerInterpreterSuggestions).to.equal( + true, + 'didTriggerInterpreterSuggestions should be set to true.' + ); }); }); }); diff --git a/src/test/interpreters/pipEnvService.unit.test.ts b/src/test/interpreters/pipEnvService.unit.test.ts index bc7b25efe0c6..3d8c2bf995e0 100644 --- a/src/test/interpreters/pipEnvService.unit.test.ts +++ b/src/test/interpreters/pipEnvService.unit.test.ts @@ -60,6 +60,7 @@ suite('Interpreters - PipEnv', () => { let settings: TypeMoq.IMock; let pipenvPathSetting: string; let pipEnvServiceHelper: IPipEnvServiceHelper; + setup(() => { serviceContainer = TypeMoq.Mock.ofType(); const workspaceService = TypeMoq.Mock.ofType(); @@ -138,150 +139,198 @@ suite('Interpreters - PipEnv', () => { pipEnvService = new PipEnvService(serviceContainer.object); }); - test(`Should return an empty list'${testSuffix}`, () => { - const environments = pipEnvService.getInterpreters(resource); - expect(environments).to.be.eventually.deep.equal([]); - }); - test(`Should return an empty list if there is no \'PipFile\'${testSuffix}`, async () => { - const env = {}; - envVarsProvider - .setup((e) => e.getEnvironmentVariables(TypeMoq.It.isAny())) - .returns(() => Promise.resolve({})) - .verifiable(TypeMoq.Times.once()); - currentProcess.setup((c) => c.env).returns(() => env); - fileSystem - .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) - .returns(() => Promise.resolve(false)) - .verifiable(TypeMoq.Times.once()); - const environments = await pipEnvService.getInterpreters(resource); - - expect(environments).to.be.deep.equal([]); - fileSystem.verifyAll(); - }); - test(`Should display warning message if there is a \'PipFile\' but \'pipenv --version\' fails ${testSuffix}`, async () => { - const env = {}; - currentProcess.setup((c) => c.env).returns(() => env); - processService - .setup((p) => - p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--version']), TypeMoq.It.isAny()) - ) - .returns(() => Promise.reject('')); - fileSystem - .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) - .returns(() => Promise.resolve(true)); - const warningMessage = - "Workspace contains Pipfile but 'pipenv' was not found. Make sure 'pipenv' is on the PATH."; - appShell - .setup((a) => a.showWarningMessage(warningMessage)) - .returns(() => Promise.resolve('')) - .verifiable(TypeMoq.Times.once()); - const environments = await pipEnvService.getInterpreters(resource); - - expect(environments).to.be.deep.equal([]); - appShell.verifyAll(); - }); - test(`Should display warning message if there is a \'PipFile\' but \'pipenv --venv\' fails with stderr ${testSuffix}`, async () => { - const env = {}; - currentProcess.setup((c) => c.env).returns(() => env); - processService - .setup((p) => - p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--version']), TypeMoq.It.isAny()) - ) - .returns(() => Promise.resolve({ stderr: '', stdout: 'pipenv, version 2018.11.26' })); - processService - .setup((p) => - p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--venv']), TypeMoq.It.isAny()) - ) - .returns(() => Promise.resolve({ stderr: 'Aborted!', stdout: '' })); - fileSystem - .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) - .returns(() => Promise.resolve(true)); - const warningMessage = - 'Workspace contains Pipfile but the associated virtual environment has not been setup. Setup the virtual environment manually if needed.'; - appShell - .setup((a) => a.showWarningMessage(warningMessage)) - .returns(() => Promise.resolve('')) - .verifiable(TypeMoq.Times.once()); - const environments = await pipEnvService.getInterpreters(resource); - - expect(environments).to.be.deep.equal([]); - appShell.verifyAll(); - }); - test(`Should return interpreter information${testSuffix}`, async () => { - const env = {}; - const pythonPath = 'one'; - envVarsProvider - .setup((e) => e.getEnvironmentVariables(TypeMoq.It.isAny())) - .returns(() => Promise.resolve({})) - .verifiable(TypeMoq.Times.once()); - currentProcess.setup((c) => c.env).returns(() => env); - processService - .setup((p) => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns(() => Promise.resolve({ stdout: pythonPath })); - interpreterHelper - .setup((v) => v.getInterpreterInformation(TypeMoq.It.isAny())) - .returns(() => Promise.resolve({ version: new SemVer('1.0.0') })); - fileSystem - .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) - .returns(() => Promise.resolve(true)) - .verifiable(); - fileSystem - .setup((fs) => fs.fileExists(TypeMoq.It.isValue(pythonPath))) - .returns(() => Promise.resolve(true)) - .verifiable(); - - const environments = await pipEnvService.getInterpreters(resource); - - expect(environments).to.be.lengthOf(1); - fileSystem.verifyAll(); - }); - test(`Should return interpreter information using PipFile defined in Env variable${testSuffix}`, async () => { - const envPipFile = 'XYZ'; - const env = { - PIPENV_PIPFILE: envPipFile - }; - const pythonPath = 'one'; - envVarsProvider - .setup((e) => e.getEnvironmentVariables(TypeMoq.It.isAny())) - .returns(() => Promise.resolve({})) - .verifiable(TypeMoq.Times.once()); - currentProcess.setup((c) => c.env).returns(() => env); - processService - .setup((p) => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns(() => Promise.resolve({ stdout: pythonPath })); - interpreterHelper - .setup((v) => v.getInterpreterInformation(TypeMoq.It.isAny())) - .returns(() => Promise.resolve({ version: new SemVer('1.0.0') })); - fileSystem - .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) - .returns(() => Promise.resolve(false)) - .verifiable(TypeMoq.Times.never()); - fileSystem - .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, envPipFile)))) - .returns(() => Promise.resolve(true)) - .verifiable(TypeMoq.Times.once()); - fileSystem - .setup((fs) => fs.fileExists(TypeMoq.It.isValue(pythonPath))) - .returns(() => Promise.resolve(true)) - .verifiable(); - const environments = await pipEnvService.getInterpreters(resource); - - expect(environments).to.be.lengthOf(1); - fileSystem.verifyAll(); - }); - test("Must use 'python.pipenvPath' setting", async () => { - pipenvPathSetting = 'spam-spam-pipenv-spam-spam'; - const pipenvExe = pipEnvService.executable; - assert.equal(pipenvExe, 'spam-spam-pipenv-spam-spam', 'Failed to identify pipenv.exe'); + suite('With didTriggerInterpreterSuggestions set to true', () => { + setup(() => { + sinon.stub(pipEnvService, 'didTriggerInterpreterSuggestions').get(() => true); + }); + + teardown(() => { + sinon.restore(); + }); + + test(`Should return an empty list'${testSuffix}`, () => { + const environments = pipEnvService.getInterpreters(resource); + expect(environments).to.be.eventually.deep.equal([]); + }); + test(`Should return an empty list if there is no \'PipFile\'${testSuffix}`, async () => { + const env = {}; + envVarsProvider + .setup((e) => e.getEnvironmentVariables(TypeMoq.It.isAny())) + .returns(() => Promise.resolve({})) + .verifiable(TypeMoq.Times.once()); + currentProcess.setup((c) => c.env).returns(() => env); + fileSystem + .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) + .returns(() => Promise.resolve(false)) + .verifiable(TypeMoq.Times.once()); + const environments = await pipEnvService.getInterpreters(resource); + + expect(environments).to.be.deep.equal([]); + fileSystem.verifyAll(); + }); + test(`Should display warning message if there is a \'PipFile\' but \'pipenv --version\' fails ${testSuffix}`, async () => { + const env = {}; + currentProcess.setup((c) => c.env).returns(() => env); + processService + .setup((p) => + p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--version']), TypeMoq.It.isAny()) + ) + .returns(() => Promise.reject('')); + fileSystem + .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) + .returns(() => Promise.resolve(true)); + const warningMessage = + "Workspace contains Pipfile but 'pipenv' was not found. Make sure 'pipenv' is on the PATH."; + appShell + .setup((a) => a.showWarningMessage(warningMessage)) + .returns(() => Promise.resolve('')) + .verifiable(TypeMoq.Times.once()); + const environments = await pipEnvService.getInterpreters(resource); + + expect(environments).to.be.deep.equal([]); + appShell.verifyAll(); + }); + test(`Should display warning message if there is a \'PipFile\' but \'pipenv --venv\' fails with stderr ${testSuffix}`, async () => { + const env = {}; + currentProcess.setup((c) => c.env).returns(() => env); + processService + .setup((p) => + p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--version']), TypeMoq.It.isAny()) + ) + .returns(() => Promise.resolve({ stderr: '', stdout: 'pipenv, version 2018.11.26' })); + processService + .setup((p) => + p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isValue(['--venv']), TypeMoq.It.isAny()) + ) + .returns(() => Promise.resolve({ stderr: 'Aborted!', stdout: '' })); + fileSystem + .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) + .returns(() => Promise.resolve(true)); + const warningMessage = + 'Workspace contains Pipfile but the associated virtual environment has not been setup. Setup the virtual environment manually if needed.'; + appShell + .setup((a) => a.showWarningMessage(warningMessage)) + .returns(() => Promise.resolve('')) + .verifiable(TypeMoq.Times.once()); + const environments = await pipEnvService.getInterpreters(resource); + + expect(environments).to.be.deep.equal([]); + appShell.verifyAll(); + }); + test(`Should return interpreter information${testSuffix}`, async () => { + const env = {}; + const pythonPath = 'one'; + envVarsProvider + .setup((e) => e.getEnvironmentVariables(TypeMoq.It.isAny())) + .returns(() => Promise.resolve({})) + .verifiable(TypeMoq.Times.once()); + currentProcess.setup((c) => c.env).returns(() => env); + processService + .setup((p) => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns(() => Promise.resolve({ stdout: pythonPath })); + interpreterHelper + .setup((v) => v.getInterpreterInformation(TypeMoq.It.isAny())) + .returns(() => Promise.resolve({ version: new SemVer('1.0.0') })); + fileSystem + .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) + .returns(() => Promise.resolve(true)) + .verifiable(); + fileSystem + .setup((fs) => fs.fileExists(TypeMoq.It.isValue(pythonPath))) + .returns(() => Promise.resolve(true)) + .verifiable(); + + const environments = await pipEnvService.getInterpreters(resource); + + expect(environments).to.be.lengthOf(1); + fileSystem.verifyAll(); + }); + test(`Should return interpreter information using PipFile defined in Env variable${testSuffix}`, async () => { + const envPipFile = 'XYZ'; + const env = { + PIPENV_PIPFILE: envPipFile + }; + const pythonPath = 'one'; + envVarsProvider + .setup((e) => e.getEnvironmentVariables(TypeMoq.It.isAny())) + .returns(() => Promise.resolve({})) + .verifiable(TypeMoq.Times.once()); + currentProcess.setup((c) => c.env).returns(() => env); + processService + .setup((p) => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns(() => Promise.resolve({ stdout: pythonPath })); + interpreterHelper + .setup((v) => v.getInterpreterInformation(TypeMoq.It.isAny())) + .returns(() => Promise.resolve({ version: new SemVer('1.0.0') })); + fileSystem + .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, 'Pipfile')))) + .returns(() => Promise.resolve(false)) + .verifiable(TypeMoq.Times.never()); + fileSystem + .setup((fs) => fs.fileExists(TypeMoq.It.isValue(path.join(rootWorkspace, envPipFile)))) + .returns(() => Promise.resolve(true)) + .verifiable(TypeMoq.Times.once()); + fileSystem + .setup((fs) => fs.fileExists(TypeMoq.It.isValue(pythonPath))) + .returns(() => Promise.resolve(true)) + .verifiable(); + const environments = await pipEnvService.getInterpreters(resource); + + expect(environments).to.be.lengthOf(1); + fileSystem.verifyAll(); + }); + test("Must use 'python.pipenvPath' setting", async () => { + pipenvPathSetting = 'spam-spam-pipenv-spam-spam'; + const pipenvExe = pipEnvService.executable; + assert.equal(pipenvExe, 'spam-spam-pipenv-spam-spam', 'Failed to identify pipenv.exe'); + }); + + test('Should send telemetry event when calling getInterpreters', async () => { + const sendTelemetryStub = sinon.stub(Telemetry, 'sendTelemetryEvent'); + + await pipEnvService.getInterpreters(resource); + + sinon.assert.calledWith(sendTelemetryStub, EventName.PIPENV_INTERPRETER_DISCOVERY); + sinon.restore(); + }); }); - test('Should send telemetry event when calling getInterpreters', async () => { - const sendTelemetryStub = sinon.stub(Telemetry, 'sendTelemetryEvent'); + suite('With didTriggerInterpreterSuggestions set to false', () => { + setup(() => { + sinon.stub(pipEnvService, 'didTriggerInterpreterSuggestions').get(() => false); + }); + + teardown(() => { + sinon.restore(); + }); + + test('isRelatedPipEnvironment should exit early', async () => { + processService + .setup((p) => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .verifiable(TypeMoq.Times.never()); + + const result = await pipEnvService.isRelatedPipEnvironment('foo', 'some/python/path'); + + expect(result).to.be.equal(false, 'isRelatedPipEnvironment should return false.'); + processService.verifyAll(); + }); + + test('Executable getter should return an empty string', () => { + const executable = pipEnvService.executable; + + expect(executable).to.be.equal('', 'The executable getter should return an empty string.'); + }); + + test('getInterpreters should exit early', async () => { + processService + .setup((p) => p.exec(TypeMoq.It.isValue('pipenv'), TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .verifiable(TypeMoq.Times.never()); - await pipEnvService.getInterpreters(resource); + const interpreters = await pipEnvService.getInterpreters(resource); - sinon.assert.calledWith(sendTelemetryStub, EventName.PIPENV_INTERPRETER_DISCOVERY); - sinon.restore(); + expect(interpreters).to.be.lengthOf(0); + processService.verifyAll(); + }); }); }); }); diff --git a/src/test/interpreters/serviceRegistry.unit.test.ts b/src/test/interpreters/serviceRegistry.unit.test.ts index 78a116853b4c..93880dad0ee0 100644 --- a/src/test/interpreters/serviceRegistry.unit.test.ts +++ b/src/test/interpreters/serviceRegistry.unit.test.ts @@ -61,7 +61,6 @@ import { IInterpreterWatcherBuilder, IKnownSearchPathsForInterpreters, INTERPRETER_LOCATOR_SERVICE, - IPipEnvService, IShebangCodeLensProvider, IVirtualEnvironmentsSearchPathProvider, KNOWN_PATH_SERVICE, @@ -148,7 +147,6 @@ suite('Interpreters - Service Registry', () => { [IInterpreterLocatorService, GlobalVirtualEnvService, GLOBAL_VIRTUAL_ENV_SERVICE], [IInterpreterLocatorService, WorkspaceVirtualEnvService, WORKSPACE_VIRTUAL_ENV_SERVICE], [IInterpreterLocatorService, PipEnvService, PIPENV_SERVICE], - [IPipEnvService, PipEnvService], [IInterpreterLocatorService, WindowsRegistryService, WINDOWS_REGISTRY_SERVICE], [IInterpreterLocatorService, KnownPathsService, KNOWN_PATH_SERVICE], diff --git a/src/test/interpreters/virtualEnvManager.unit.test.ts b/src/test/interpreters/virtualEnvManager.unit.test.ts index 96afb7ff872f..d4c3de8c7851 100644 --- a/src/test/interpreters/virtualEnvManager.unit.test.ts +++ b/src/test/interpreters/virtualEnvManager.unit.test.ts @@ -10,7 +10,7 @@ import { Uri, WorkspaceFolder } from 'vscode'; import { IWorkspaceService } from '../../client/common/application/types'; import { IFileSystem } from '../../client/common/platform/types'; import { IProcessServiceFactory } from '../../client/common/process/types'; -import { IPipEnvService } from '../../client/interpreter/contracts'; +import { IInterpreterLocatorService, IPipEnvService, PIPENV_SERVICE } from '../../client/interpreter/contracts'; import { VirtualEnvironmentManager } from '../../client/interpreter/virtualEnvs'; import { IServiceContainer } from '../../client/ioc/types'; @@ -41,7 +41,7 @@ suite('Virtual environment manager', () => { expect(name).to.be.equal(virtualEnvFolderName); }); - test('Use workspacec name as env name', async () => { + test('Use workspace name as env name', async () => { const serviceContainer = TypeMoq.Mock.ofType(); const pipEnvService = TypeMoq.Mock.ofType(); pipEnvService @@ -51,7 +51,9 @@ suite('Virtual environment manager', () => { serviceContainer .setup((s) => s.get(TypeMoq.It.isValue(IProcessServiceFactory))) .returns(() => TypeMoq.Mock.ofType().object); - serviceContainer.setup((s) => s.get(TypeMoq.It.isValue(IPipEnvService))).returns(() => pipEnvService.object); + serviceContainer + .setup((s) => s.get(TypeMoq.It.isValue(IInterpreterLocatorService), TypeMoq.It.isValue(PIPENV_SERVICE))) + .returns(() => pipEnvService.object); serviceContainer .setup((s) => s.get(TypeMoq.It.isValue(IFileSystem))) .returns(() => TypeMoq.Mock.ofType().object); @@ -83,7 +85,9 @@ suite('Virtual environment manager', () => { pipEnvService .setup((w) => w.isRelatedPipEnvironment(TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns(() => Promise.resolve(isPipEnvironment)); - serviceContainer.setup((s) => s.get(TypeMoq.It.isValue(IPipEnvService))).returns(() => pipEnvService.object); + serviceContainer + .setup((s) => s.get(TypeMoq.It.isValue(IInterpreterLocatorService), TypeMoq.It.isValue(PIPENV_SERVICE))) + .returns(() => pipEnvService.object); const workspaceService = TypeMoq.Mock.ofType(); workspaceService.setup((w) => w.hasWorkspaceFolders).returns(() => false); if (resource) { diff --git a/src/test/interpreters/virtualEnvs/index.unit.test.ts b/src/test/interpreters/virtualEnvs/index.unit.test.ts index b0ab3c9139c8..7913dbb7edae 100644 --- a/src/test/interpreters/virtualEnvs/index.unit.test.ts +++ b/src/test/interpreters/virtualEnvs/index.unit.test.ts @@ -14,7 +14,12 @@ import { IFileSystem, IPlatformService } from '../../../client/common/platform/t import { IProcessService, IProcessServiceFactory } from '../../../client/common/process/types'; import { ITerminalActivationCommandProvider } from '../../../client/common/terminal/types'; import { ICurrentProcess, IPathUtils } from '../../../client/common/types'; -import { InterpreterType, IPipEnvService } from '../../../client/interpreter/contracts'; +import { + IInterpreterLocatorService, + InterpreterType, + IPipEnvService, + PIPENV_SERVICE +} from '../../../client/interpreter/contracts'; import { VirtualEnvironmentManager } from '../../../client/interpreter/virtualEnvs'; import { IServiceContainer } from '../../../client/ioc/types'; @@ -51,7 +56,9 @@ suite('Virtual Environment Manager', () => { serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IPathUtils))).returns(() => pathUtils.object); serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IFileSystem))).returns(() => fs.object); serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IWorkspaceService))).returns(() => workspace.object); - serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IPipEnvService))).returns(() => pipEnvService.object); + serviceContainer + .setup((c) => c.get(TypeMoq.It.isValue(IInterpreterLocatorService), TypeMoq.It.isValue(PIPENV_SERVICE))) + .returns(() => pipEnvService.object); serviceContainer .setup((c) => c.get(TypeMoq.It.isValue(ITerminalActivationCommandProvider), TypeMoq.It.isAny())) .returns(() => terminalActivation.object); diff --git a/src/test/serviceRegistry.ts b/src/test/serviceRegistry.ts index 57a133000870..5ee6414daa70 100644 --- a/src/test/serviceRegistry.ts +++ b/src/test/serviceRegistry.ts @@ -55,7 +55,6 @@ import { IInterpreterLocatorService, IInterpreterService, INTERPRETER_LOCATOR_SERVICE, - IPipEnvService, KNOWN_PATH_SERVICE, PIPENV_SERVICE, WINDOWS_REGISTRY_SERVICE, @@ -379,7 +378,6 @@ export class IocContainer { KnownPathsService, KNOWN_PATH_SERVICE ); - this.serviceManager.addSingleton(IPipEnvService, PipEnvService); this.serviceManager.addSingleton( IInterpreterLocatorHelper, From afb927b728e757eda260a389c73ee426c956bc9b Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 11 May 2020 13:49:18 -0700 Subject: [PATCH 5/8] Update extension version (#11730) * Update extension version * Update date in changelog. --- CHANGELOG.md | 4 ++-- package-lock.json | 2 +- package.json | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec36802066db..5156aae4e7a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## 2020.5.0-rc (5 May 2020) +## 2020.5.0 (12 May 2020) ### Enhancements @@ -79,7 +79,7 @@ 1. Ensure kernel daemons are disposed correctly when closing notebooks. ([#11579](https://github.com/Microsoft/vscode-python/issues/11579)) 1. When VS quits, make sure to save contents of notebook for next reopen. - ([#11557](https://github.com/Microsoft/vscode-python/issues/11557)) + ([#11557](https://github.com/Microsoft/vscode-python/issues/11557)) 1. Fix scrolling when clicking in the interactive window to not jump around. ([#11554](https://github.com/Microsoft/vscode-python/issues/11554)) diff --git a/package-lock.json b/package-lock.json index 3c950b27ef58..a87bf84d4bf8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "python", - "version": "2020.5.0-rc", + "version": "2020.5.0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 1c8bbf58fb27..96950926fd56 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "python", "displayName": "Python", "description": "Linting, Debugging (multi-threaded, remote), Intellisense, Jupyter Notebooks, code formatting, refactoring, unit tests, snippets, and more.", - "version": "2020.5.0-rc", + "version": "2020.5.0", "featureFlags": { "usingNewInterpreterStorage": true }, From d1b7b8eb03975fad5beb7e205b9bc02b3000d3c1 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Tue, 12 May 2020 11:09:39 -0700 Subject: [PATCH 6/8] Update change log with additional notes. (#11764) --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5156aae4e7a5..de2615faa33e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,7 @@ 1. Hide progress indicator once `Interactive Window` has loaded. ([#11065](https://github.com/Microsoft/vscode-python/issues/11065)) 1. Do not perform pipenv interpreter discovery on extension activation. + Fix for [CVE-2020-1171](https://portal.msrc.microsoft.com/en-us/security-guidance/advisory/CVE-2020-1171). ([#11127](https://github.com/Microsoft/vscode-python/issues/11127)) 1. Ensure arguments are included in log messages when using decorators. ([#11153](https://github.com/Microsoft/vscode-python/issues/11153)) @@ -82,6 +83,8 @@ ([#11557](https://github.com/Microsoft/vscode-python/issues/11557)) 1. Fix scrolling when clicking in the interactive window to not jump around. ([#11554](https://github.com/Microsoft/vscode-python/issues/11554)) +1. Setting "Data Science: Run Startup Commands" is now limited to being a user setting. + Fix for [CVE-2020-1192](https://portal.msrc.microsoft.com/en-us/security-guidance/advisory/CVE-2020-1192). ### Code Health From 4c78f3bd895a73787d8abc7b4923a5e76123b0c3 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 18 May 2020 17:20:20 -0700 Subject: [PATCH 7/8] Cherry picks and version updates for bug fix release (#11878) * Do not execute shebang as an interpreter until user has clicked on the codelens enclosing the shebang (#11816) * Do not execute shebang as an interpreter until user has clicked on the codelens enclosing the shebang * Rename * Oops * Update src/test/providers/shebangCodeLenseProvider.unit.test.ts Co-authored-by: Karthik Nadig Co-authored-by: Karthik Nadig * Update version and change log for bugfix release Co-authored-by: Kartik Raj --- CHANGELOG.md | 61 +++++++++++++++++++ package-lock.json | 2 +- package.json | 2 +- .../commands/setShebangInterpreter.ts | 3 +- src/client/interpreter/contracts.ts | 2 +- .../display/shebangCodeLensProvider.ts | 13 +++- .../shebangCodeLenseProvider.unit.test.ts | 30 +++++++-- 7 files changed, 100 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de2615faa33e..0a91b3dcc53a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,66 @@ # Changelog +## 2020.5.1 (19 May 2020) + +### Fixes + +1. Do not execute shebang as an interpreter until user has clicked on the codelens enclosing the shebang. + ([#11687](https://github.com/Microsoft/vscode-python/issues/11687)) + +### Thanks + +Thanks to the following projects which we fully rely on to provide some of +our features: + +- [debugpy](https://pypi.org/project/debugpy/) +- [isort](https://pypi.org/project/isort/) +- [jedi](https://pypi.org/project/jedi/) + and [parso](https://pypi.org/project/parso/) +- [Microsoft Python Language Server](https://github.com/microsoft/python-language-server) +- [ptvsd](https://pypi.org/project/ptvsd/) +- [exuberant ctags](http://ctags.sourceforge.net/) (user-installed) +- [rope](https://pypi.org/project/rope/) (user-installed) + +Also thanks to the various projects we provide integrations with which help +make this extension useful: + +- Debugging support: + [Django](https://pypi.org/project/Django/), + [Flask](https://pypi.org/project/Flask/), + [gevent](https://pypi.org/project/gevent/), + [Jinja](https://pypi.org/project/Jinja/), + [Pyramid](https://pypi.org/project/pyramid/), + [PySpark](https://pypi.org/project/pyspark/), + [Scrapy](https://pypi.org/project/Scrapy/), + [Watson](https://pypi.org/project/Watson/) +- Formatting: + [autopep8](https://pypi.org/project/autopep8/), + [black](https://pypi.org/project/black/), + [yapf](https://pypi.org/project/yapf/) +- Interpreter support: + [conda](https://conda.io/), + [direnv](https://direnv.net/), + [pipenv](https://pypi.org/project/pipenv/), + [pyenv](https://github.com/pyenv/pyenv), + [venv](https://docs.python.org/3/library/venv.html#module-venv), + [virtualenv](https://pypi.org/project/virtualenv/) +- Linting: + [bandit](https://pypi.org/project/bandit/), + [flake8](https://pypi.org/project/flake8/), + [mypy](https://pypi.org/project/mypy/), + [prospector](https://pypi.org/project/prospector/), + [pylint](https://pypi.org/project/pylint/), + [pydocstyle](https://pypi.org/project/pydocstyle/), + [pylama](https://pypi.org/project/pylama/) +- Testing: + [nose](https://pypi.org/project/nose/), + [pytest](https://pypi.org/project/pytest/), + [unittest](https://docs.python.org/3/library/unittest.html#module-unittest) + +And finally thanks to the [Python](https://www.python.org/) development team and +community for creating a fantastic programming language and community to be a +part of! + ## 2020.5.0 (12 May 2020) ### Enhancements diff --git a/package-lock.json b/package-lock.json index a87bf84d4bf8..aae95237a16e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "python", - "version": "2020.5.0", + "version": "2020.5.1", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 96950926fd56..4b8dc7e22bb9 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "python", "displayName": "Python", "description": "Linting, Debugging (multi-threaded, remote), Intellisense, Jupyter Notebooks, code formatting, refactoring, unit tests, snippets, and more.", - "version": "2020.5.0", + "version": "2020.5.1", "featureFlags": { "usingNewInterpreterStorage": true }, diff --git a/src/client/interpreter/configuration/interpreterSelector/commands/setShebangInterpreter.ts b/src/client/interpreter/configuration/interpreterSelector/commands/setShebangInterpreter.ts index 82834395f976..e3e63b299f83 100644 --- a/src/client/interpreter/configuration/interpreterSelector/commands/setShebangInterpreter.ts +++ b/src/client/interpreter/configuration/interpreterSelector/commands/setShebangInterpreter.ts @@ -37,7 +37,8 @@ export class SetShebangInterpreterCommand extends BaseInterpreterSelectorCommand protected async setShebangInterpreter(): Promise { const shebang = await this.shebangCodeLensProvider.detectShebang( - this.documentManager.activeTextEditor!.document + this.documentManager.activeTextEditor!.document, + true ); if (!shebang) { return; diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index 6013ff504bd2..55b203667ab7 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -114,7 +114,7 @@ export interface IInterpreterDisplay { export const IShebangCodeLensProvider = Symbol('IShebangCodeLensProvider'); export interface IShebangCodeLensProvider extends CodeLensProvider { - detectShebang(document: TextDocument): Promise; + detectShebang(document: TextDocument, resolveShebangAsInterpreter?: boolean): Promise; } export const IInterpreterHelper = Symbol('IInterpreterHelper'); diff --git a/src/client/interpreter/display/shebangCodeLensProvider.ts b/src/client/interpreter/display/shebangCodeLensProvider.ts index f545abe86de9..8737981c8369 100644 --- a/src/client/interpreter/display/shebangCodeLensProvider.ts +++ b/src/client/interpreter/display/shebangCodeLensProvider.ts @@ -19,7 +19,10 @@ export class ShebangCodeLensProvider implements IShebangCodeLensProvider { // tslint:disable-next-line:no-any this.onDidChangeCodeLenses = (workspaceService.onDidChangeConfiguration as any) as Event; } - public async detectShebang(document: TextDocument): Promise { + public async detectShebang( + document: TextDocument, + resolveShebangAsInterpreter: boolean = false + ): Promise { const firstLine = document.lineAt(0); if (firstLine.isEmptyOrWhitespace) { return; @@ -30,8 +33,12 @@ export class ShebangCodeLensProvider implements IShebangCodeLensProvider { } const shebang = firstLine.text.substr(2).trim(); - const pythonPath = await this.getFullyQualifiedPathToInterpreter(shebang, document.uri); - return typeof pythonPath === 'string' && pythonPath.length > 0 ? pythonPath : undefined; + if (resolveShebangAsInterpreter) { + const pythonPath = await this.getFullyQualifiedPathToInterpreter(shebang, document.uri); + return typeof pythonPath === 'string' && pythonPath.length > 0 ? pythonPath : undefined; + } else { + return typeof shebang === 'string' && shebang.length > 0 ? shebang : undefined; + } } public async provideCodeLenses(document: TextDocument, _token?: CancellationToken): Promise { return this.createShebangCodeLens(document); diff --git a/src/test/providers/shebangCodeLenseProvider.unit.test.ts b/src/test/providers/shebangCodeLenseProvider.unit.test.ts index 76edd0facae4..c878cb34a9fa 100644 --- a/src/test/providers/shebangCodeLenseProvider.unit.test.ts +++ b/src/test/providers/shebangCodeLenseProvider.unit.test.ts @@ -63,15 +63,33 @@ suite('Shebang detection', () => { return [doc, line]; } - test('Shebang should be empty when first line is empty', async () => { + test('Shebang should be empty when first line is empty when resolving shebang as interpreter', async () => { const [document, line] = createDocument(''); - const shebang = await provider.detectShebang(document.object); + const shebang = await provider.detectShebang(document.object, true); document.verifyAll(); line.verifyAll(); expect(shebang).to.be.equal(undefined, 'Shebang should be undefined'); }); + test('Shebang should be empty when first line is empty when not resolving shebang as interpreter', async () => { + const [document, line] = createDocument(''); + + const shebang = await provider.detectShebang(document.object, false); + + document.verifyAll(); + line.verifyAll(); + expect(shebang).to.be.equal(undefined, 'Shebang should be undefined'); + }); + test('Shebang should be returned as it is when not resolving shebang as interpreter', async () => { + const [document, line] = createDocument('#!HELLO'); + + const shebang = await provider.detectShebang(document.object, false); + + document.verifyAll(); + line.verifyAll(); + expect(shebang).to.be.equal('HELLO', 'Shebang should be HELLO'); + }); test('Shebang should be empty when python path is invalid in shebang', async () => { const [document, line] = createDocument('#!HELLO'); @@ -80,7 +98,7 @@ suite('Shebang detection', () => { .returns(() => Promise.reject()) .verifiable(typemoq.Times.once()); - const shebang = await provider.detectShebang(document.object); + const shebang = await provider.detectShebang(document.object, true); document.verifyAll(); line.verifyAll(); @@ -95,7 +113,7 @@ suite('Shebang detection', () => { .returns(() => Promise.resolve({ stdout: 'THIS_IS_IT' })) .verifiable(typemoq.Times.once()); - const shebang = await provider.detectShebang(document.object); + const shebang = await provider.detectShebang(document.object, true); document.verifyAll(); line.verifyAll(); @@ -113,7 +131,7 @@ suite('Shebang detection', () => { .returns(() => Promise.resolve({ stdout: 'THIS_IS_IT' })) .verifiable(typemoq.Times.once()); - const shebang = await provider.detectShebang(document.object); + const shebang = await provider.detectShebang(document.object, true); document.verifyAll(); line.verifyAll(); @@ -132,7 +150,7 @@ suite('Shebang detection', () => { .returns(() => Promise.resolve({ stdout: 'THIS_IS_IT' })) .verifiable(typemoq.Times.once()); - const shebang = await provider.detectShebang(document.object); + const shebang = await provider.detectShebang(document.object, true); document.verifyAll(); line.verifyAll(); From 3fbc26be7d6a11b6a69762005902dc281b583add Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Tue, 19 May 2020 11:48:25 -0700 Subject: [PATCH 8/8] Clean up news --- news/2 Fixes/11554.md | 1 - news/2 Fixes/11557.md | 1 - news/2 Fixes/11687.md | 1 - package.json | 6 ------ .../interactive-ipynb/nativeEditorStorage.ts | 13 +------------ .../nativeEditorStorage.unit.test.ts | 4 +--- 6 files changed, 2 insertions(+), 24 deletions(-) delete mode 100644 news/2 Fixes/11554.md delete mode 100644 news/2 Fixes/11557.md delete mode 100644 news/2 Fixes/11687.md diff --git a/news/2 Fixes/11554.md b/news/2 Fixes/11554.md deleted file mode 100644 index e2352638df84..000000000000 --- a/news/2 Fixes/11554.md +++ /dev/null @@ -1 +0,0 @@ -Fix scrolling when clicking in the interactive window to not jump around. \ No newline at end of file diff --git a/news/2 Fixes/11557.md b/news/2 Fixes/11557.md deleted file mode 100644 index e39f5d530b12..000000000000 --- a/news/2 Fixes/11557.md +++ /dev/null @@ -1 +0,0 @@ -When VS quits, make sure to save contents of notebook for next reopen. \ No newline at end of file diff --git a/news/2 Fixes/11687.md b/news/2 Fixes/11687.md deleted file mode 100644 index d34a4a5bccfc..000000000000 --- a/news/2 Fixes/11687.md +++ /dev/null @@ -1 +0,0 @@ -Do not execute shebang as an interpreter until user has clicked on the codelens enclosing the shebang. diff --git a/package.json b/package.json index 4540748ce4be..7245f8aa4549 100644 --- a/package.json +++ b/package.json @@ -1770,12 +1770,6 @@ "description": "Automatically scroll the interactive window to show the output of the last statement executed. If false, the interactive window will only automatically scroll if the bottom of the prior cell is visible.", "scope": "resource" }, - "python.dataScience.alwaysScrollOnNewCell": { - "type": "boolean", - "default": false, - "description": "Automatically scroll the interactive window to show the output of the last statement executed. If false, the interactive window will only automatically scroll if the bottom of the prior cell is visible.", - "scope": "resource" - }, "python.dataScience.enableScrollingForCellOutputs": { "type": "boolean", "default": true, diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index 2845e53e2d40..6780197d1317 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -8,14 +8,7 @@ import { concatMultilineStringInput, splitMultilineString } from '../../../datas import { createCodeCell } from '../../../datascience-ui/common/cellFactory'; import { traceError } from '../../common/logger'; import { IFileSystem } from '../../common/platform/types'; -import { - GLOBAL_MEMENTO, - ICryptoUtils, - IDisposableRegistry, - IExtensionContext, - IMemento, - WORKSPACE_MEMENTO -} from '../../common/types'; +import { GLOBAL_MEMENTO, ICryptoUtils, IExtensionContext, IMemento, WORKSPACE_MEMENTO } from '../../common/types'; import { noop } from '../../common/utils/misc'; import { PythonInterpreter } from '../../interpreter/contracts'; import { Identifiers, KnownNotebookLanguages, Telemetry } from '../constants'; @@ -156,10 +149,6 @@ class NativeEditorNotebookModel implements INotebookModel { // Forward onto our listeners if necessary if (changed || this.isDirty !== oldDirty) { this._changedEmitter.fire({ ...change, newDirty: this.isDirty, oldDirty }); - if (this.isDirty) { - // Save to temp storage so we don't lose the file if the user exits VS code - this.saveToStorage().ignoreErrors(); - } } // Slightly different for the event we send to VS code. Skip version and file changes. Only send user events. if ((changed || this.isDirty !== oldDirty) && change.kind !== 'version' && change.source === 'user') { diff --git a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts index ea6d102bacf9..1b034ecf8269 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts @@ -371,9 +371,7 @@ suite('DataScience - Native Editor Storage', () => { instance(crypto), context.object, globalMemento, - localMemento, - instance(workspace), - [] + localMemento ); return new NotebookStorageProvider(notebookStorage, [], instance(workspace));