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

Pin and test all vscode packages #899

Merged
merged 9 commits into from Nov 10, 2023

Conversation

bockstaller
Copy link
Contributor

As mentioned by @chadlwilson, vscode and the associated LSP projects aren't adhering to semver.

Mismatching @types/vscode and engines['vscode'] versions are caught by the build process.
This isn't the case for a engines['vscode'] value that is lower than the @types/vscode version required by vscode-jsonrpc, vscode-languageclient, and vscode-languageclient-protocol.

Starting the plugin with a "compatible" vscode version (according to engines['vscode']) will fail during runtime with the following error
Error: The language client requires VS Code version ^1.82.0 but received version 1.63.1.

What

This PR adds:

  1. A new npm script, npm run compatibilityTest, which executes the test suite using the minimum vscode version specified in package.json. This is kept separate in order to avoid doubling test execution time locally. (The failure mode looks like this)
  2. Uses npm run compatibilityTest in the vscode.yaml github action
  3. Pins all vscode and lsp types to create a working configuration. This required rearranging some imports in src/protocol/gauge.proposed.ts and src/gaugeWorkspace.proposed.ts
  4. Adds missing sinon types.

this commit adds a new testing mode whereby the extension
is tested against the stated minimum vscode compatibility

this test fails in this commit due to a minimum @types/vscode
requirement of the lsp packages that breaks during runtime

Signed-off-by: Lukas Bockstaller <lukas.bockstaller@posteo.de>
the compatibility tests (and using the plugin with any version
<1.82.0) currently fails with
Error: The language client requires VS Code version ^1.82.0 but received version 1.63.1
This commit pins the versions so that doesn't occur anymore.

Rationale for the versions:

We need at least @types/vscode=~1.67.0 to use the newer client start api:

const client: LanguageClient = ...;
await client.start();

instead of

const client: LanguageClient = ...;
client.start();
await client.onReady();

and the LanguageClient#dispose method.

But @types/vscode=~1.67.0 has an issue where running tests
modifies the package.json (microsoft/vscode#148975)
which is fixed in ~1.71.0.

The LSP packages are then selected to keep backwards compatibility with
~1.71.0

Signed-off-by: Lukas Bockstaller <lukas.bockstaller@posteo.de>
Signed-off-by: Lukas Bockstaller <lukas.bockstaller@posteo.de>
@bockstaller
Copy link
Contributor Author

I am very sorry for introducing this much turmoil into your release process. Having this test would have caught the issues earlier

Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
@gaugebot
Copy link

gaugebot bot commented Nov 9, 2023

@bockstaller Thank you for contributing to gauge-vscode. Your pull request has been labeled as a release candidate 🎉🎉.

Merging this PR will trigger a release.

Please bump up the version as part of this PR.

Instructions to bump the version can found at CONTRIBUTING.md

If the CONTRIBUTING.md file does not exist or does not include instructions about bumping up the version, please looks previous commits in git history to see what changes need to be done.

@zabil
Copy link
Member

zabil commented Nov 9, 2023

Hope you don't mind me pushing a commit ee8a8f4

To remove references as a part of this PR. Please feel free to bump up the version. I've marked this PR as a release candidate and hopefully this time the plugin will published on merge 🤞

Signed-off-by: Lukas Bockstaller <lukas.bockstaller@posteo.de>
@bockstaller
Copy link
Contributor Author

Hope you don't mind me pushing a commit ee8a8f4

Otherwise, I wouldn't activate the option 👍

@chadlwilson
Copy link
Contributor

chadlwilson commented Nov 9, 2023

Can you please summarise the reason for the major changes here, i.e what the reason for needing to bump to 1.71 is, motivation for the choice of various languageclient and jsonrpc versions? It seems if we're getting this wrong, we should be doing it with some king of philosophy driven from vscode extension recommendation? And even ask if we care about maintaining compatibility with VS Code versions older than a year or so?

For example, right now there are mixed versions of vscode-languageserver-protocol - some dependencies are using 3.17.3, some are now using 3.17.5. That means there are actually mixed versions of vscode-jsonrpc right now - some are using 8.1.0 and some using 8.2.0. That doesn't seem desirable, so there is something not correct with this right now - probably that if you need a specific "patch" version, the version needs to be declared as 3.17.3 exactly.

More relevant again, it seems the project shouldn't be declaring direct dependencies on multiple of these libraries and should just be using vscode-languageclient directly as do the sample extensions to avoid having to synchronise all the combinations like this.

@bockstaller
Copy link
Contributor Author

Regarding the selection of minimum version.

From f95c9db

The compatibility tests (and using the plugin with any version
<1.82.0) currently fails with
Error: The language client requires VS Code version ^1.82.0 but received version 1.63.1
This commit pins the versions so that doesn't occur anymore.

Rationale for the versions:

We need at least @types/vscode=~1.67.0 to use the newer client start api:

const client: LanguageClient = ...;
await client.start();

instead of

const client: LanguageClient = ...;
client.start();
await client.onReady();

and the LanguageClient#dispose method.

But @types/vscode=~1.67.0 has an issue where running tests
modifies the package.json (microsoft/vscode#148975)
which is fixed in ~1.71.0.

The LSP packages are then selected to keep backwards compatibility with
~1.71.0

I selected the oldest compatible vscode version as the base for compatibility as it excludes as few clients as possible.
Further tightening the range of supported clients is certainly doable.
I wouldn't upgrade to the latest LSP client version as they tighten the supported vscode version to 1.82, which is quite recent.
Is that rationale reasonable?
(Personally, I am interested in keeping backward compatibility with v1.77.3 as my colleagues are currently blocked from upgrading their system due to other circumstances.)

Regarding the direct dependencies. I am able to remove them by changing some imports

…-protocol

Signed-off-by: Lukas Bockstaller <lukas.bockstaller@posteo.de>
Signed-off-by: Lukas Bockstaller <lukas.bockstaller@posteo.de>
@chadlwilson
Copy link
Contributor

I selected the oldest compatible vscode version as the base for compatibility as it excludes as few clients as possible.
Further tightening the range of supported clients is certainly doable.
I wouldn't upgrade to the latest LSP client version as they tighten the supported vscode version to 1.82, which is quite recent.
Is that rationale reasonable?

I suppose so.

Regarding the direct dependencies. I am able to remove them by changing some imports

I'm not sure that is strictly necessary since you can import from a transitive, but OK

Signed-off-by: Chad Wilson <chadw@thoughtworks.com>
Signed-off-by: Chad Wilson <chadw@thoughtworks.com>
@chadlwilson chadlwilson merged commit 8e3c73c into getgauge:master Nov 10, 2023
5 of 7 checks passed
@chadlwilson
Copy link
Contributor

This is published now as 0.1.4 at https://marketplace.visualstudio.com/items?itemName=getgauge.gauge

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

Successfully merging this pull request may close these issues.

None yet

3 participants