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

Bump version of VSCode used in tests #911

Merged

Conversation

movermeyer
Copy link
Contributor

@movermeyer movermeyer commented Apr 26, 2023

What are you trying to accomplish?

The i18n-ally tests are failing, since the source.vue files are being given the language identifier of plaintext (the default for unknown languages).

There is an override in test/fixtures/vue/.vscode/settings.json, but that is ignored unless vue is a known identifier.

Previously, i18n-ally used the johnsoncodehk.volar extension to add recognition of the vue language identifier. At some point, johnsoncodehk.volar was renamed to vue.volar. But vue.volar cannot be installed on VSCode v1.52.0:

Installing extensions...
Installing extension 'vue.volar' v1.5.4...
Server returned 404
Failed Installing Extensions: vue.volar

What approach did you choose and why?

The most straightforward fix is to bump the version of VSCode used in the tests to a more modern one.

What should reviewers focus on?

These tests wil break once again in the future if/when vue.volar can no longer be installed on v1.77. But I don't have a nicer solution at this time.

Other options include:

  • Create an otherwise empty extension that registers the vue identifier, and use that
    • A minimal extension is less likely to break with a future version of VSCode.
  • Having i18n-ally itself register the vue identifier

The impact of these changes

CI will pass again. 🙌

@movermeyer movermeyer marked this pull request as ready for review April 26, 2023 19:02
@movermeyer movermeyer force-pushed the movermeyer/update_version_of_vscode_in_tests branch from f0a7ec2 to a30ae58 Compare April 26, 2023 19:12
@terales terales merged commit 01ad55f into lokalise:main Apr 26, 2023
@kibertoad
Copy link
Collaborator

@movermeyer Mac is still failing.

@terales
Copy link
Collaborator

terales commented Apr 27, 2023

@kibertoad it's a flaky test -- worked well after merge:
image

huacnlee pushed a commit to huacnlee/i18n-ally that referenced this pull request Aug 28, 2023
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.

3 participants