Added Pyenv file system watcher#14792
Conversation
5f47e52 to
56f9936
Compare
56f9936 to
f995200
Compare
| const kind = await getVirtualEnvKind(env); | ||
|
|
||
| const timeData = await getFileInfo(env); | ||
| if (virtualEnvKinds.includes(kind)) { |
There was a problem hiding this comment.
We shouldn't discard environments of Unknown type, hence this change. It was causing the watcher test to fail where we created a dummy environment (type Unknown).
There was a problem hiding this comment.
I can separate out the PR where I consolidate all this + this fix if that would be easier.
There was a problem hiding this comment.
What's the effect of the change outside of test? I'm assuming that the check was there for some purpose - if so, the dummy test environment should probably not be Unknown.
There was a problem hiding this comment.
the check was there for some purpose
I talked with Karthik about the original purpose, but it was something which just wasn't thought through. He agrees that it should be removed.
The effect of this change is that even if we're unable to detect environment type, we would now still report it.
bb6bfe9 to
df7a0fc
Compare
e10b00b to
76a6e83
Compare
c0970e4 to
be9ca8f
Compare
| assert.deepEqual(actualEvent!.type, FileChangeType.Created, 'Wrong event emitted'); | ||
| testLocatorWatcher(testWorkOnHomePath, createGlobalVirtualEnvironmentLocator); | ||
| suiteTeardown(() => { | ||
| process.env.WORKON_HOME = workonHomeOldValue; |
There was a problem hiding this comment.
I'm not sure what the preferred approach is, but in some other tests, we stub getEnvironmentVariable from common/utils/platform.
There was a problem hiding this comment.
This is a system test, so no stubbing here. The other tests were unit tests.
| async function setupLocator(onChanged: (e: PythonEnvsChangedEvent) => Promise<void>) { | ||
| locator = options?.arg ? await createLocatorFactoryFunc(options.arg) : await createLocatorFactoryFunc(); | ||
| // Wait for watchers to get ready | ||
| await sleep(1000); |
There was a problem hiding this comment.
This is a fairly long wait, given that it happens at the beginning of a bunch of tests. Can we expose something in the API so that the test can detect it instead? E.g. a promise indicating that the watchers are ready.
There was a problem hiding this comment.
Unfortunately there's no way, underlying VSCode API doesn't let us know when the watchers are ready. And other than the purpose of the test, we don't care.
Btw this is old code which was just moved, so most of this has already been reviewed.
| await run(argv, { cwd: this.root }); | ||
| } catch (err) { | ||
| throw new Error(`Failed to create Env ${path.basename(envName)} Error: ${err}`); | ||
| } |
There was a problem hiding this comment.
Since all the catch blocks here immediately rethrow with some context, perhaps it would be better for the class to log what it does, such that when exception happens, the requisite information would be immediately preceding in the log?
There was a problem hiding this comment.
For such a small class, all the context is already available in the error, and I don't think logging is useful unless the error happens. So I will like to keep it as it is.
3b3b0fe to
aef0baf
Compare
|
|
||
| suiteSetup(() => venvs.cleanUp()); | ||
|
|
||
| async function setupLocator(onChanged: (e: PythonEnvsChangedEvent) => Promise<void>) { |
There was a problem hiding this comment.
Will there be a follow-up PR or work item to replace all existing waitForChangeToBeDetected/isLocated/setupLocator declarations with this utility class?
There was a problem hiding this comment.
That's what this PR does: for all the venv locators, use this Venvs class instead.
|
@int19h @kimadeline I'm merging this now. If there're other concerns you can still comment here, and I'll address them separately. |
This is very similar to workspace virtualenv watcher #14780, and global virtual env as well. I have consolidated all of them in this PR.