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

Fix #989 vscode v1_92 breaking api change when spawning .bat or .cmd files #990

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

@chadlwilson
Copy link
Contributor

chadlwilson commented Aug 5, 2024

Thanks! There are some test failures. (edit: fixed)

Do you know which vscode version this is compatible with? iIRC the tests run against the min version declared compatible at

"vscode": "^1.71.0"
as well as the latest stable version.

Should be possible to gain confidence in both versions.

@chadlwilson chadlwilson linked an issue Aug 5, 2024 that may be closed by this pull request
@chadlwilson
Copy link
Contributor

I'm not an expert in the history of this project, however I don't quite understand why the master tests are passing on Windows right now, despite running against VSCode 1.92. I'd have thought they'd be failing, but perhaps it depends on how Gauge is installed :-/

https://github.com/getgauge/gauge-vscode/actions/runs/10244438966/job/28337428223

@Zoupers
Copy link
Author

Zoupers commented Aug 5, 2024

I think the reason might be that the gaugeCommand we searched in the test environment is not endswith .bat or .cmd.

@zabil
Copy link
Member

zabil commented Aug 6, 2024

@chadlwilson
Copy link
Contributor

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.

@zabil
Copy link
Member

zabil commented Aug 6, 2024

I'd prefer if were possible to get a failing test that is fixed by this,

VS Code recommends using insiders edition version for running the tests. I think that could be the best way.

@zabil
Copy link
Member

zabil commented Aug 6, 2024

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.

@chadlwilson
Copy link
Contributor

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.

@Zoupers
Copy link
Author

Zoupers commented Aug 6, 2024

@zabil @chadlwilson I have tested the reason I think it might be by this . It failed indeed.

@Zoupers
Copy link
Author

Zoupers commented Aug 6, 2024

The logic I modified is here.

        if (platform() === 'win32') {
            validExecExt = [".bat", ".cmd"]
        };

src/cli.ts Show resolved Hide resolved
@chadlwilson
Copy link
Contributor

chadlwilson commented Aug 6, 2024

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 .exe version that of course works fine as is not a shell command. The current ordering is a bit weird to me, it should probably be exe > cmd > bat in a general sense, but not the major issue here.

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 gauge:master from source (i.e dont use a released version) and that means only the exe will be available - so it works.

Bit confused about which install mechanism will end up relying on a .bat or .cmd though, as I thought all versions were distributed as a basic executable now. Knowing this would probably allow us to validate this reliably?

@Zoupers how did you install Gauge? Which version?

@Zoupers
Copy link
Author

Zoupers commented Aug 7, 2024

@chadlwilson I install gauge via npm.

Gauge version: 1.5.6
Commit Hash: a5e7eb63

@chadlwilson
Copy link
Contributor

@chadlwilson I install gauge via npm.

Gauge version: 1.5.6
Commit Hash: a5e7eb63

Does it still fail on the current/latest version from npm? (Probably does, but worth a go)

@Zoupers
Copy link
Author

Zoupers commented Aug 7, 2024

@chadlwilson I install gauge via npm.

Gauge version: 1.5.6
Commit Hash: a5e7eb63

Does it still fail on the current/latest version from npm? (Probably does, but worth a go)

Yes, I tried this, still fail.

Gauge version: 1.6.7
Commit Hash: 684d870

@chadlwilson
Copy link
Contributor

Ok thx. I'll play with the tests to double check if we can catch this differently.

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

Successfully merging this pull request may close these issues.

Gauge extension loses executable on vsCode version 1_92
3 participants