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

Files imported from F5 tests are loaded twice on Windows (once via 'C:\', once via 'c:\') #58388

Closed
StephenWeatherford opened this issue Sep 10, 2018 · 26 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues verified Verification succeeded

Comments

@StephenWeatherford
Copy link

StephenWeatherford commented Sep 10, 2018

Issue Type: Bug

NOTE: This did not repro in 1.26.1

This is causing issues because I end up with multiple versions of module variables.

  1. Clone this repo: https://github.com/microsoft/vscode-docker
  2. npm I
  3. open in vscode
  4. open extensionVariables.ts
  5. Add the following lines to the top of the line:

console.error('ext loaded');
// tslint:disable-next-line:no-debugger
debugger;

  1. set launch configuration to 'Launch Tests' and press F5
    !) the breakpoint is hit twice
    !) If you let the test continue, it fails with "Extension not activated", which is a direct result of having two different version of 'extensionVariables.ts' loaded.

If you check the callstack from Module._compile on the breakpoint, you'll see that
First call:
id starts with "C:" (this is the path given to Mocha by the testrunner using glob
image
Second call:
id starts with "c:"

Therefore the cached version is not returned.

VS Code version: Code 1.27.1 (5944e81, 2018-09-06T09:21:18.328Z)
OS version: Windows_NT x64 10.0.17134

System Info
Item Value
CPUs Intel(R) Xeon(R) CPU E31230 @ 3.20GHz (4 x 3193)
GPU Status 2d_canvas: enabled
checker_imaging: disabled_off
flash_3d: enabled
flash_stage3d: enabled
flash_stage3d_baseline: enabled
gpu_compositing: enabled
multiple_raster_threads: enabled_on
native_gpu_memory_buffers: disabled_software
rasterization: enabled
video_decode: enabled
video_encode: enabled
webgl: enabled
webgl2: enabled
Memory (System) 15.96GB (2.38GB free)
Process Argv C:\Program Files\Microsoft VS Code\Code.exe
Screen Reader no
VM 0%
Extensions (24)
Extension Author (truncated) Version
azds azu 0.1.120180829
mongogo bag 0.1.2
regex chr 0.2.0
vscode-markdownlint Dav 0.20.0
githistory don 0.4.2
jupyter don 1.1.4
tslint eg2 1.0.38
vscode-bump fab 1.6.0
docthis joe 0.7.1
vscode-antlr4 mik 2.0.4
vscode-azureappservice ms- 0.9.1
vscode-azurefunctions ms- 0.10.1
vscode-azurestorage ms- 0.4.1
vscode-cosmosdb ms- 0.8.0
python ms- 2018.8.0
azure-account ms- 0.4.3
csharp ms- 1.16.0
azurerm-vscode-tools msa 0.4.2
vscode-docker Pet 0.2.0
vscode-versionlens pfl 0.21.1
yo sam 0.9.3
extension-api spy 0.3.1
python tht 0.2.3
code-lens-roundup wad 1.0.0
@vscodebot vscodebot bot removed the new release label Sep 11, 2018
@weinand weinand added the debug Debug viewlet, configurations, breakpoints, adapter issues label Sep 11, 2018
@weinand weinand added the bug Issue identified by VS Code Team member as probable bug label Sep 11, 2018
@weinand weinand removed their assignment Sep 11, 2018
@weinand
Copy link
Contributor

weinand commented Sep 11, 2018

@isidorn drive letter casing differences should never result in non-matching files!

@grork
Copy link

grork commented Sep 11, 2018

For a work around, if you pass the paths in your launch.json explicitly as absolute paths, and lower case the drive letter, you'll be able to execute the tests.

@StephenWeatherford
Copy link
Author

StephenWeatherford commented Sep 12, 2018

Surprisingly, that doesn't work:

		"args": [
			"C:/Repos/vscode-docker/test/test.code-workspace",
			"--extensionDevelopmentPath=C:/Repos/vscode-docker",
			"--extensionTestsPath=C:/Repos/vscode-docker/out/test"
		],
		"outFiles": [
			"C:/Repos/vscode-docker/out/**/*.js" //"${workspaceRoot}/out/**/*.js"
		],

-> C: ended up as c:, so tried to load "c:/Repos/vscode-docker..."

@grork
Copy link

grork commented Sep 12, 2018

As I mentioned; lower case the drive letter in your launch.json; that's unblocked me.

@weinand weinand added this to the September 2018 milestone Sep 12, 2018
@StephenWeatherford
Copy link
Author

You're right, I misinterpreted which way to convert. That'll work as a temporary work-around, thanks.

@DanTup
Copy link
Contributor

DanTup commented Sep 17, 2018

I think this problem is breaking all sorts of things on Windows:

#58815
#58388

I have some other failing tests too which I hadn't raised issues about, but putting absolute paths with lowercase drive letters into launch.json fixes them. However I can't do this on my CI builds easily.

@DanTup
Copy link
Contributor

DanTup commented Sep 18, 2018

Setting env.CODE_TESTS_PATH and env.CODE_TESTS_WORKSPACE to have lowercase drive letters in my test script appears to solve the issue on AppVeyor too.

DanTup added a commit to Dart-Code/Dart-Code that referenced this issue Sep 18, 2018
I believe thie issue is because of microsoft/vscode#58388 and the previous workaround should fix this.
DanTup added a commit to Dart-Code/Dart-Code that referenced this issue Sep 19, 2018
I believe thie issue is because of microsoft/vscode#58388 and the previous workaround should fix this.
@isidorn
Copy link
Contributor

isidorn commented Sep 19, 2018

Ok, after some analysis it seems the issue is that the extensionDevelopmentPath, extensionTestsPath or outFiles are now comparing paths differently.
Did you change something in this area recently @bpasero @roblourens @weinand

We have not changed anything in how we handle source files in debug land which would break this.

@StephenWeatherford following your steps I get a weird error. Are there some other prerequests that I should setup for this to work
screen shot 2018-09-19 at 15 51 33

@DanTup your test from the other issue fail for me both on mac and windows. Shouldn't it work on mac?

@weinand
Copy link
Contributor

weinand commented Sep 19, 2018

I'm not aware of "outFiles" changed in node-debug or node-debug2.
But the extensionDevelopmentPath now supports URIs (thanks to @aeschli).

@bpasero
Copy link
Member

bpasero commented Sep 19, 2018

@isidorn you should talk to @aeschli and @sandy081. quite possible that something changed when moving from fsPath to URI or for remote scenarios.

@weinand
Copy link
Contributor

weinand commented Sep 19, 2018

I'd hope that the outcome of the move towards URIs is that URI comparisons are more lenient...

@DanTup
Copy link
Contributor

DanTup commented Sep 19, 2018

@DanTup your test from the other issue fail for me both on mac and windows. Shouldn't it work on mac?

Which test do you mean? I forced the drive letter to lowercase in my test script (see Dart-Code/Dart-Code@11a6f5e) and a bunch of my Windows issues linked here went away. What was the error you saw?

@StephenWeatherford
Copy link
Author

@isidorn No, no other requirements. The tests won't fully pass without docker installed, but you should be able to get far enough to verify the problem (just tried this on my home machine without docker). Can you not open the test\test.code-workspace workspace in vscode?

@isidorn
Copy link
Contributor

isidorn commented Sep 19, 2018

@DanTup I mean following your steps from here #58815 (comment)
@StephenWeatherford I can repro now following your stpes, the breakpoint gets hit wtice but both times I see capital C:.

Anyways let's wait for @aeschli and @sandy081 to comment

@StephenWeatherford
Copy link
Author

FYI, when I did my original investigation if I remember correctly I believe the earlier vscode ended up with paths where the casing matched, whereas with the newer vscode they didn't. I.e., I'm not sure that the comparison logic changed, but rather that the path casings being passed in to load module changed (the ones brought in by Mocha and the ones brought in by vscode).

@DanTup
Copy link
Contributor

DanTup commented Sep 20, 2018

@isidorn

I mean following your steps from here #58815 (comment)

So you cloned that repro and ran the test on macOS but it failed? You didn't get the same result as my screenshot in that comment? 🤔

I can't explain that - it passes here on macOS, fails on Windows, but if I change {workspaceFolder} to a full FS path on Windows it will a) pass if the drive letter is lowercase and b) fail if the drive letter is uppercase.

Note: I'm testing with Stable, as Insiders seems broken (fails at startup complaining about merge-conflict extension not being found?).

@isidorn
Copy link
Contributor

isidorn commented Sep 20, 2018

@StephenWeatherford @DanTup I pushed a potential fix for this. Can you please try with latest insiders and let me know if it still repros. The latest insiders should not fail at startup.

@isidorn isidorn added the info-needed Issue requires more information from poster label Sep 21, 2018
@StephenWeatherford
Copy link
Author

Still repros on "1.28.0-insider (user setup), Commit: 2558a72"

@StephenWeatherford
Copy link
Author

Weirdly, I have another machine where I am seeing this same issue from the command line (i.e. not F5), which runs tests like this (non-insiders 1.27.2):

"scripts": {
"test": "npm run build && cross-env CODE_TESTS_WORKSPACE=./test/test.code-workspace DEBUGTELEMETRY=1 node ./node_modules/vscode/bin/test"
}

I don't know what the difference between the two machines is. On the other one, npm test from command line always succeeds.

@DanTup
Copy link
Contributor

DanTup commented Sep 22, 2018

@StephenWeatherford

Given you have a relative path, may it depend on the casing of the drive letter in the current working directory when that command is executed? If you added pwd before that command and ran it on each machine, what does the path look like?

@StephenWeatherford
Copy link
Author

Machine where it succeeds: C:\Repos\vscode-docker
Machine where it fails: C:\Users<username(lowercase)>\Repos\vscode-docker

So how is this supposed to work? I'm assuming the module loading code of treating files including drive names as case-sensitive is the desired behavior and shouldn't be changed, right?

Again, I'm pretty sure this was a change in behavior between vscode 1.26.1 and 1.27.x. I'm willing to go back to 1.26.1 to verify.

@weinand
Copy link
Contributor

weinand commented Sep 24, 2018

@isidorn @bpasero we must fix this: the drive letter casing should not affect the path matching in VS Code. We need a way to avoid opening files twice if the drive letter case differs. This is not rocket science!

@isidorn I suggest that you first verify that this happens when a DA returns stopped events with a "source" value that differs in drive letter casing. If you can verify this -- and only then -- you should fix this in VS Code (because drive letter casing consistency is not part of the DAP specification).

@isidorn
Copy link
Contributor

isidorn commented Sep 26, 2018

The issue with showing multiple sources with different casing is already fixed via a commit I pusehd 7 days ago which stores sources independent on casing unless on linux

The reason why tests were failing in this issue is that with the URI refactoring the extensionDevelopmentPath would be upper cased. Which @aeschli and me fixed via cec1733
@StephenWeatherford I have followed your steps and now all is good for me

As for comparing uris in workbench and case sensitivity here's the issue tracking that #12448

@isidorn isidorn removed the info-needed Issue requires more information from poster label Sep 26, 2018
@mjbvz mjbvz added the verified Verification succeeded label Sep 28, 2018
@DanTup
Copy link
Contributor

DanTup commented Oct 1, 2018

Sorry for the delay in responding, I've been OOO. I've testd both my simple logging sample from above and also #57513 and the issue is resolved for me, it's behaving as it used to 👍

@isidorn
Copy link
Contributor

isidorn commented Oct 1, 2018

@DanTup thanks for letting us know.

@StephenWeatherford
Copy link
Author

Works for me, too. Thanks!

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

9 participants