vscode-jest icon indicating copy to clipboard operation
vscode-jest copied to clipboard

test-provider: adjust workspace location handling

Open compnerd opened this issue 1 year ago • 8 comments

The VSCode reported workspace path is unresolved (microsoft/vscode#18837). When the repository is hosted in a path substituted path (e.g. subst X: C:), the test path will not be properly translated as the drive is different. This may potentially also impact paths which are hosted within a NT junction. Explicitly resolve the workspace path to match the fact that jest will report paths which have been resolved to the real path.

compnerd avatar Dec 22 '22 04:12 compnerd

@connectdotz thanks for approving the CI run. I'm not sure how to make progress here. The failure on Ubuntu is particularly odd. Would you happen to have any suggestions?

compnerd avatar Dec 24 '22 16:12 compnerd

@compnerd, thanks for the PR.

Regarding the test, you would need to mock fs.realpathSync.native.

I have a few questions and am not so sure if we should make this change across the board just yet:

  • reading the related vscode issues (thanks for the link) furthered my concern about this symlink or no-symlink debate. vscode is taking the pain to change from realpath to the current version should be a telltale sign.
  • we have been using the unresolved workspace.fsPath for a long time and it works mostly. Changing this could cause regression for those who already adjusted or implemented the workaround... therefore, we should try to make it backward compatible. I suggest we use an option for user to opt-in, at least initially.
  • We used workspace.fsPath all over the places, is this the only code needed to change for a symlink project? I am not sure how involved it will be. Sounds like you have such a project, is there anything else broken regarding using this extension in your project?

connectdotz avatar Dec 24 '22 22:12 connectdotz

@compnerd, thanks for the PR.

My pleasure :)

Regarding the test, you would need to mock fs.realpathSync.native.

I have a few questions and am not so sure if we should make this change across the board just yet:

I think that resolving the questions is something we should do before addressing the tests and linter.

At the very least the commit message needs to be amended. This is not for a symlinked project, but for the case where the project itself is checked out to the moral equivalent of what on Linux would amount to a bind mount. On Windows you can create a drive from an existing path via the subst command. If you say do subst S: C:\Users\user\sources, and then work in S:, the path becomes a potential issue as the location that VSCode sees is just the canonicalized path without path resolution.

The reason that I am locally doing this resolution explicitly rather than at the stored path (which would be computationally better as it avoids unnecessary re-computation), is that while VSCode has opted to not perform the path resolution, the Jest command line tool will list the test locations with the resolved path. The result is that the path prefix will not match and we will fail to compute a project relative path.

  • reading the related vscode issues (thanks for the link) furthered my concern about this symlink or no-symlink debate. vscode is taking the pain to change from realpath to the current version should be a telltale sign.

The big thing here is a difference in behavior between jest and VSCode. The jest tool reports the location of the tests using the resolved path while VSCode reports the path using the unresolved path.

  • we have been using the unresolved workspace.fsPath for a long time and it works mostly. Changing this could cause regression for those who already adjusted or implemented the workaround... therefore, we should try to make it backward compatible. I suggest we use an option for user to opt-in, at least initially.

We will now resolve the path for the project, however, given that jest will report the resolved path, in either case this should be an improvement as we will now properly identify the repository relative path for the tests.

  • We used workspace.fsPath all over the places, is this the only code needed to change for a symlink project? I am not sure how involved it will be. Sounds like you have such a project, is there anything else broken regarding using this extension in your project?

So far the one thing that I immediately noticed trying to use the extension was that the tests were reported being nested under each path arc where the source tree was located in the resolved path rather than where VSCode was pointed to as the root of the project. It was frustrating enough that I didn't spend much time exploring the rest of the extension as the path is rather long and so having ~10 directories to iterate through to see the result was off-putting (which also unnecessary right shifts the entry).

compnerd avatar Dec 24 '22 23:12 compnerd

Sorry for the delayed response. This issue is interesting and I would like to play with it further to see if there is any other implication.

If you have time to explore it further that would be great: to ensure everything else works, and this is the only place that needs to be changed. If not, that's fine, too; I will probably get back to this after the next release. Thanks for your patience.

connectdotz avatar Jan 09 '23 15:01 connectdotz

From my testing for the functionality, it seemed to correct the issues that I was having with it.

compnerd avatar Jan 13 '23 01:01 compnerd

@connectdotz Hi, just following up on this, have you had a chance to play with this any further?

compnerd avatar Feb 08 '23 16:02 compnerd

@compnerd, yes, I have been thinking about this. Do you mind trying the extension with different jest.autoRun modes, i.e. 'watch', 'on-save', and 'off', to see if there are other places we could have problems...

I think we could do this with a new setting, let's say jest.useRealPath, default to false, so we can try this feature without impacting existing users, just to be safe in case there are other implications we have thought about.

connectdotz avatar Feb 10 '23 21:02 connectdotz

@connectdotz playing around with the settings, it seems that everything works as expected with the changes.

compnerd avatar Feb 18 '23 16:02 compnerd