More tests

karthiknadig · karthiknadig · commit c7ec7ea14b0a · 2020-09-29T12:26:36.000-07:00
diff --git a/src/client/pythonEnvironments/common/environmentIdentifier.ts b/src/client/pythonEnvironments/common/environmentIdentifier.ts
@@ -7,8 +7,8 @@ import { isPyenvEnvironment } from '../discovery/locators/services/pyenvLocator'
 import { isVenvEnvironment } from '../discovery/locators/services/venvLocator';
 import { isVirtualenvEnvironment } from '../discovery/locators/services/virtualenvLocator';
 import { isVirtualenvwrapperEnvironment } from '../discovery/locators/services/virtualenvwrapperLocator';
+import { isWindowsStoreEnvironment } from '../discovery/locators/services/windowsStoreLocator';
 import { EnvironmentType } from '../info';
-import { isWindowsStoreEnvironment } from './windowsUtils';
 
 /**
  * Gets a prioritized list of environment types for identification.
diff --git a/src/client/pythonEnvironments/common/windowsUtils.ts b/src/client/pythonEnvironments/common/windowsUtils.ts
@@ -2,8 +2,6 @@
 // Licensed under the MIT License.
 
 import * as path from 'path';
-import { traceWarning } from '../../common/logger';
-import { getEnvironmentVariable } from '../../common/utils/platform';
 
 /**
  * Checks if a given path ends with python*.exe
@@ -22,77 +20,3 @@ export function isWindowsPythonExe(interpreterPath:string): boolean {
 
     return windowsPythonExes.test(path.basename(interpreterPath));
 }
-
-/**
- * Gets path to the Windows Apps directory.
- * @returns {string} : Returns path to the Windows Apps directory under
- * `%LOCALAPPDATA%/Microsoft/WindowsApps`.
- */
-export function getWindowsStoreAppsRoot(): string {
-    const localAppData = getEnvironmentVariable('LOCALAPPDATA') || '';
-    return path.join(localAppData, 'Microsoft', 'WindowsApps');
-}
-
-/**
- * Checks if a given path is under the forbidden windows store directory.
- * @param {string} interpreterPath : Absolute path to the python interpreter.
- * @returns {boolean} : Returns true if `interpreterPath` is under
- * `%ProgramFiles%/WindowsApps`.
- */
-export function isForbiddenStorePath(interpreterPath:string):boolean {
-    const programFilesStorePath = path
-        .join(getEnvironmentVariable('ProgramFiles') || 'Program Files', 'WindowsApps')
-        .normalize()
-        .toUpperCase();
-    return path.normalize(interpreterPath).toUpperCase().includes(programFilesStorePath);
-}
-
-/**
- * Checks if the given interpreter belongs to Windows Store Python environment.
- * @param interpreterPath: Absolute path to any python interpreter.
- *
- * Remarks:
- * 1. Checking if the path includes `Microsoft\WindowsApps`, `Program Files\WindowsApps`, is
- * NOT enough. In WSL, `/mnt/c/users/user/AppData/Local/Microsoft/WindowsApps` is available as a search
- * path. It is possible to get a false positive for that path. So the comparison should check if the
- * absolute path to 'WindowsApps' directory is present in the given interpreter path. The WSL path to
- * 'WindowsApps' is not a valid path to access, Windows Store Python.
- *
- * 2. 'startsWith' comparison may not be right, user can provide '\\?\C:\users\' style long paths in windows.
- *
- * 3. A limitation of the checks here is that they don't handle 8.3 style windows paths.
- * For example,
- *     `C:\Users\USER\AppData\Local\MICROS~1\WINDOW~1\PYTHON~2.EXE`
- * is the shortened form of
- *     `C:\Users\USER\AppData\Local\Microsoft\WindowsApps\python3.7.exe`
- *
- * The correct way to compare these would be to always convert given paths to long path (or to short path).
- * For either approach to work correctly you need actual file to exist, and accessible from the user's
- * account.
- *
- * To convert to short path without using N-API in node would be to use this command. This is very expensive:
- * `> cmd /c for %A in ("C:\Users\USER\AppData\Local\Microsoft\WindowsApps\python3.7.exe") do @echo %~sA`
- * The above command will print out this:
- * `C:\Users\USER\AppData\Local\MICROS~1\WINDOW~1\PYTHON~2.EXE`
- *
- * If we go down the N-API route, use node-ffi and either call GetShortPathNameW or GetLongPathNameW from,
- * Kernel32 to convert between the two path variants.
- *
- */
-export async function isWindowsStoreEnvironment(interpreterPath: string): Promise<boolean> {
-    const pythonPathToCompare = path.normalize(interpreterPath).toUpperCase();
-    const localAppDataStorePath = path
-        .normalize(getWindowsStoreAppsRoot())
-        .toUpperCase();
-    if (pythonPathToCompare.includes(localAppDataStorePath)) {
-        return true;
-    }
-
-    // Program Files store path is a forbidden path. Only admins and system has access this path.
-    // We should never have to look at this path or even execute python from this path.
-    if (isForbiddenStorePath(pythonPathToCompare)) {
-        traceWarning('isWindowsStoreEnvironment called with Program Files store path.');
-        return true;
-    }
-    return false;
-}
diff --git a/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts
@@ -4,17 +4,92 @@
 import * as fsapi from 'fs-extra';
 import * as path from 'path';
 import { Event, EventEmitter } from 'vscode';
-import { Architecture } from '../../../../common/utils/platform';
+import { traceWarning } from '../../../../common/logger';
+import { Architecture, getEnvironmentVariable } from '../../../../common/utils/platform';
 import {
     PythonEnvInfo, PythonEnvKind, PythonReleaseLevel, PythonVersion,
 } from '../../../base/info';
 import { parseVersion } from '../../../base/info/pythonVersion';
 import { ILocator, IPythonEnvsIterator } from '../../../base/locator';
 import { PythonEnvsChangedEvent } from '../../../base/watcher';
 import { getFileInfo } from '../../../common/externalDependencies';
-import { getWindowsStoreAppsRoot, isWindowsPythonExe, isWindowsStoreEnvironment } from '../../../common/windowsUtils';
+import { isWindowsPythonExe } from '../../../common/windowsUtils';
 import { IEnvironmentInfoService } from '../../../info/environmentInfoService';
 
+/**
+ * Gets path to the Windows Apps directory.
+ * @returns {string} : Returns path to the Windows Apps directory under
+ * `%LOCALAPPDATA%/Microsoft/WindowsApps`.
+ */
+export function getWindowsStoreAppsRoot(): string {
+    const localAppData = getEnvironmentVariable('LOCALAPPDATA') || '';
+    return path.join(localAppData, 'Microsoft', 'WindowsApps');
+}
+
+/**
+ * Checks if a given path is under the forbidden windows store directory.
+ * @param {string} interpreterPath : Absolute path to the python interpreter.
+ * @returns {boolean} : Returns true if `interpreterPath` is under
+ * `%ProgramFiles%/WindowsApps`.
+ */
+export function isForbiddenStorePath(interpreterPath:string):boolean {
+    const programFilesStorePath = path
+        .join(getEnvironmentVariable('ProgramFiles') || 'Program Files', 'WindowsApps')
+        .normalize()
+        .toUpperCase();
+    return path.normalize(interpreterPath).toUpperCase().includes(programFilesStorePath);
+}
+
+/**
+ * Checks if the given interpreter belongs to Windows Store Python environment.
+ * @param interpreterPath: Absolute path to any python interpreter.
+ *
+ * Remarks:
+ * 1. Checking if the path includes `Microsoft\WindowsApps`, `Program Files\WindowsApps`, is
+ * NOT enough. In WSL, `/mnt/c/users/user/AppData/Local/Microsoft/WindowsApps` is available as a search
+ * path. It is possible to get a false positive for that path. So the comparison should check if the
+ * absolute path to 'WindowsApps' directory is present in the given interpreter path. The WSL path to
+ * 'WindowsApps' is not a valid path to access, Windows Store Python.
+ *
+ * 2. 'startsWith' comparison may not be right, user can provide '\\?\C:\users\' style long paths in windows.
+ *
+ * 3. A limitation of the checks here is that they don't handle 8.3 style windows paths.
+ * For example,
+ *     `C:\Users\USER\AppData\Local\MICROS~1\WINDOW~1\PYTHON~2.EXE`
+ * is the shortened form of
+ *     `C:\Users\USER\AppData\Local\Microsoft\WindowsApps\python3.7.exe`
+ *
+ * The correct way to compare these would be to always convert given paths to long path (or to short path).
+ * For either approach to work correctly you need actual file to exist, and accessible from the user's
+ * account.
+ *
+ * To convert to short path without using N-API in node would be to use this command. This is very expensive:
+ * `> cmd /c for %A in ("C:\Users\USER\AppData\Local\Microsoft\WindowsApps\python3.7.exe") do @echo %~sA`
+ * The above command will print out this:
+ * `C:\Users\USER\AppData\Local\MICROS~1\WINDOW~1\PYTHON~2.EXE`
+ *
+ * If we go down the N-API route, use node-ffi and either call GetShortPathNameW or GetLongPathNameW from,
+ * Kernel32 to convert between the two path variants.
+ *
+ */
+export async function isWindowsStoreEnvironment(interpreterPath: string): Promise<boolean> {
+    const pythonPathToCompare = path.normalize(interpreterPath).toUpperCase();
+    const localAppDataStorePath = path
+        .normalize(getWindowsStoreAppsRoot())
+        .toUpperCase();
+    if (pythonPathToCompare.includes(localAppDataStorePath)) {
+        return true;
+    }
+
+    // Program Files store path is a forbidden path. Only admins and system has access this path.
+    // We should never have to look at this path or even execute python from this path.
+    if (isForbiddenStorePath(pythonPathToCompare)) {
+        traceWarning('isWindowsStoreEnvironment called with Program Files store path.');
+        return true;
+    }
+    return false;
+}
+
 /**
  * Gets paths to the Python executable under Windows Store apps.
  * @returns: Returns python*.exe for the windows store app root directory.
@@ -59,7 +134,7 @@ export class WindowsStoreLocator implements ILocator {
 
     public async resolveEnv(env: string | PythonEnvInfo): Promise<PythonEnvInfo | undefined> {
         const executablePath = typeof env === 'string' ? env : env.executable.filename;
-        if (isWindowsStoreEnvironment(executablePath)) {
+        if (await isWindowsStoreEnvironment(executablePath)) {
             const interpreterInfo = await this.envService.getEnvironmentInfo(executablePath);
             if (interpreterInfo) {
                 const data = await getFileInfo(executablePath);
@@ -68,7 +143,6 @@ export class WindowsStoreLocator implements ILocator {
                     ...data,
                 };
                 return Promise.resolve({
-                    id: '',
                     name: '',
                     location: '',
                     kind: this.kind,
@@ -100,7 +174,6 @@ export class WindowsStoreLocator implements ILocator {
             };
         }
         return {
-            id: '',
             name: '',
             location: '',
             kind: this.kind,
diff --git a/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts b/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts
@@ -156,7 +156,7 @@ suite('Windows Store', () => {
                     const data = pathToData.get(k);
                     if (data) {
                         return {
-                            id: '',
+
                             name: '',
                             location: '',
                             kind: PythonEnvKind.WindowsStore,
@@ -182,7 +182,7 @@ suite('Windows Store', () => {
         test('resolveEnv(string)', async () => {
             const python38path = path.join(testStoreAppRoot, 'python3.8.exe');
             const expected = {
-                id: '',
+
                 name: '',
                 location: '',
                 kind: PythonEnvKind.WindowsStore,
@@ -200,7 +200,7 @@ suite('Windows Store', () => {
         test('resolveEnv(PythonEnvInfo)', async () => {
             const python38path = path.join(testStoreAppRoot, 'python3.8.exe');
             const expected = {
-                id: '',
+
                 name: '',
                 location: '',
                 kind: PythonEnvKind.WindowsStore,
@@ -210,7 +210,6 @@ suite('Windows Store', () => {
 
             // Partially filled in env info object
             const input:PythonEnvInfo = {
-                id: '',
                 name: '',
                 location: '',
                 kind: PythonEnvKind.WindowsStore,
@@ -236,5 +235,15 @@ suite('Windows Store', () => {
 
             assertEnvEqual(actual, expected);
         });
+        test('resolveEnv(string): Non store python', async () => {
+            // Use a non store root path
+            const python38path = path.join(testLocalAppData, 'python3.8.exe');
+
+            const envService = new EnvironmentInfoService();
+            const locator = new WindowsStoreLocator(envService);
+            const actual = await locator.resolveEnv(python38path);
+
+            assert.deepStrictEqual(actual, undefined);
+        });
     });
 });
