-
Notifications
You must be signed in to change notification settings - Fork 19
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
Fix #989 vscode v1_92 breaking api change when spawning .bat or .cmd files #990
base: master
Are you sure you want to change the base?
Conversation
Thanks! Do you know which vscode version this is compatible with? iIRC the tests run against the min version declared compatible at Line 14 in 6a2f0cd
Should be possible to gain confidence in both versions. |
I'm not an expert in the history of this project, however I don't quite understand why the |
I think the reason might be that the gaugeCommand we searched in the test environment is not endswith |
@Zoupers can you sign you commits? |
I'd prefer if were possible to get a failing test that is fixed by this, also to make sure we haven't regressed something else :( The tests here with GHA use the gauge GitHub action to install gauge on windows but not sure how that is different to a regular gauge install for end users. Ideally it wouldn't be. |
VS Code recommends using insiders edition version for running the tests. I think that could be the best way. |
But you did mention that it does not fail on 1.92 🤔 when technically it should. So maybe there's an issue with the test. |
Yes, that's kinda what I'm saying. So combining this change with the as-is tests creates the risk that it is also not exposing/validating properly something that might break on older VSCode versions which have older Node integrated. |
Signed-off-by: Zoupers <[email protected]>
Signed-off-by: Chad Wilson <[email protected]>
@zabil @chadlwilson I have tested the reason I think it might be by this . It failed indeed. |
The logic I modified is here. if (platform() === 'win32') {
validExecExt = [".bat", ".cmd"]
}; |
Ahh OK, thanks! So what I infer from this is that on GHA by defualt it is finding the This probably varies based on whether you install via installer vs npm vs zip file vs build-from-source. The github action tests here build Bit confused about which install mechanism will end up relying on a @Zoupers how did you install Gauge? Which version? |
@chadlwilson I install gauge via npm.
|
Does it still fail on the current/latest version from npm? (Probably does, but worth a go) |
Yes, I tried this, still fail.
|
Ok thx. I'll play with the tests to double check if we can catch this differently. |
Follow https://code.visualstudio.com/updates/v1_92#_electron-30-update and https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows