-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Virtualenv locator #13893
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// 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 |
---|---|---|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, these are the case that I believe are appropriate for stubbing:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}); | ||
}); |
Uh oh!
There was an error while loading. Please reload this page.