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

test-provider: adjust workspace location handling #972

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

compnerd
Copy link

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.

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
Copy link
Author

@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?

@connectdotz
Copy link
Collaborator

@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?

@compnerd
Copy link
Author

@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).

@connectdotz
Copy link
Collaborator

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.

@compnerd
Copy link
Author

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

@compnerd
Copy link
Author

compnerd commented Feb 8, 2023

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

@connectdotz
Copy link
Collaborator

@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.

@compnerd
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants