Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Virtualenv locator #13893

Merged
merged 4 commits into from
Sep 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/client/pythonEnvironments/common/environmentIdentifier.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import { isCondaEnvironment } from '../discovery/locators/services/condaLocator';
import { isVenvEnvironment } from '../discovery/locators/services/venvLocator';
import { isVirtualenvEnvironment } from '../discovery/locators/services/virtualenvLocator';
import { isWindowsStoreEnvironment } from '../discovery/locators/services/windowsStoreLocator';
import { EnvironmentType } from '../info';

Expand Down Expand Up @@ -42,6 +43,10 @@ export async function identifyEnvironment(interpreterPath: string): Promise<Envi
return EnvironmentType.Venv;
}

if (await isVirtualenvEnvironment(interpreterPath)) {
return EnvironmentType.VirtualEnv;
}

// additional identifiers go here

return EnvironmentType.Unknown;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
ericsnowcurrently marked this conversation as resolved.
Show resolved Hide resolved
// Licensed under the MIT License.

import * as fsapi from 'fs-extra';
import * as path from 'path';
kimadeline marked this conversation as resolved.
Show resolved Hide resolved

/**
* Checks if the given interpreter belongs to a virtualenv based environment.
* @param {string} interpreterPath: Absolute path to the python interpreter.
* @returns {boolean} : Returns true if the interpreter belongs to a virtualenv environment.
*/
export async function isVirtualenvEnvironment(interpreterPath:string): Promise<boolean> {
// Check if there are any activate.* files in the same directory as the interpreter.
//
// env
// |__ activate, activate.* <--- check if any of these files exist
// |__ python <--- interpreterPath
const directory = path.dirname(interpreterPath);
const files = await fsapi.readdir(directory);
const regex = /^activate(\.([A-z]|\d)+)?$/;

return files.find((file) => regex.test(file)) !== undefined;
ericsnowcurrently marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -109,4 +109,21 @@ suite('Environment Identifier', () => {
assert.deepEqual(envType, EnvironmentType.Venv);
});
});

suite('Virtualenv', () => {
const activateFiles = [
{ folder: 'virtualenv1', file: 'activate' },
{ folder: 'virtualenv2', file: 'activate.sh' },
{ folder: 'virtualenv3', file: 'activate.ps1' },
];

activateFiles.forEach(({ folder, file }) => {
test(`Folder contains ${file}`, async () => {
const interpreterPath = path.join(TEST_LAYOUT_ROOT, folder, 'bin', 'python');
const envType = await identifyEnvironment(interpreterPath);

assert.deepStrictEqual(envType, EnvironmentType.VirtualEnv);
});
});
});
});
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import * as assert from 'assert';
import * as fsapi from 'fs-extra';
import * as path from 'path';
import * as sinon from 'sinon';
import { isVirtualenvEnvironment } from '../../../../client/pythonEnvironments/discovery/locators/services/virtualenvLocator';

suite('Virtualenv Locator Tests', () => {
const envRoot = path.join('path', 'to', 'env');
const interpreter = path.join(envRoot, 'python');
Comment on lines +11 to +12
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a unit test then there should be no dependence on the behavior of the path module. Instead we would either stub that (or the functions) out or we would directly use one of the OS-specific implementations (path.posix or path.win32).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this test depends on the behavior of the path module. If in the rest of the code we let path do what ever it does, then in the test we should let path do whatever it does. we are not testing path here, and the fact that path gets used anywhere at all is an internal implementation detail that this test should not care about.

Copy link
Member

@gramster gramster Sep 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, functional testing is testing that the system as a whole functions as intended. So acceptance tests for user stories would be functional tests. Unit tests can test at levels of abstraction that are not the same as functional tests. But that does not mean that all dependencies should be stubbed. Stub dependencies that are stateful or have performance implications. Don't stub things just because they are dependencies.

You should always be focused on outcomes. Are the tests hermetic? Are they fast? (Those two would be reasons to stub stateful or slow dependencies). Do they find bugs? (I'd argue here that stubbing path is not going to help you find more bugs - and in fact if a bug was introduced in path you'd want to know about it). Are they brittle? (Here, over-reliance on implementation details would make the test brittle - and stubbing path would be an example of that).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, these are the case that I believe are appropriate for stubbing:

  • stateful shared dependencies
  • slow dependencies
  • non-deterministic components
  • resource-intensive components
  • hard to initialize/configure components
  • components where you want to generate boundary test values that would be hard to create with the real code
  • in TDD, stubbing dependencies that don't yet exist (until they do)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That's good info. My only concern is that, in general, we do not know if a dependency is stateful unless that project promises it is (and we trust them) or we inspect the code (and assume it doesn't become stateful later). FWIW, I agree that in the case of path we have a high level of confidence that it is neither stateful nor slow.

I guess my question is if it's more valuable to be consistent about strongly separating external dependencies or to allow exceptions. On the one hand, using test doubles adds complexity to tests, has a tendency to couple tests more tightly to code (making them more fragile), and moves them a step further from the actual runtime behavior. On the other hand, it means we don't have to spend the time to figure out (or guess) if a dependency needs to be stubbed out, nor do we have to worry about that behavior changing in the future.

So the way I see it, having a consistent separation from external dependencies allows us to always focus just on the logic of the unit under test, which from my perspective is the essential purpose of unit tests. For me, when we run into the downsides of test doubles (for external dependencies), it is almost always an indicator that the unit-under-test is trying to do too much, rather than the test doubles being the problem.

Regardless, I don't think this is a critical issue, especially since I doubt there will be many stateless+fast external dependencies. So this isn't something we need to spend much more time on. However, I do want to have a better sense of the actual tradeoffs. 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case it wasn't clear, I'm not blocking the PR on this. 😄

let readDirStub: sinon.SinonStub;

setup(() => {
readDirStub = sinon.stub(fsapi, 'readdir');
});

teardown(() => {
readDirStub.restore();
});

test('Interpreter folder contains an activate file', async () => {
readDirStub.resolves(['activate', 'python']);

assert.ok(await isVirtualenvEnvironment(interpreter));
});

test('Interpreter folder does not contain any activate.* files', async () => {
readDirStub.resolves(['mymodule', 'python']);

assert.strictEqual(await isVirtualenvEnvironment(interpreter), false);
});
});