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

LSP Rewrite Step 1: fix many bugs, more LSP test coverage! #3521

Open
wants to merge 54 commits into
base: main
Choose a base branch
from

Conversation

acao
Copy link
Member

@acao acao commented Jan 27, 2024

this fixes multiple cacheing bugs, on writing some in-depth integration coverage for the LSP server.
it also solves several bugs regarding loading config types, and properly restarts the server when there are config changes

Bugfix Summary

  • jump to definition in embedded files offset bug (correct file, wrong range)
  • cache invalidation for fragments (fragment definitions change when they are supposed to)
  • schema cache invalidation for schema files (schema doesn't change when a watched schema file changes)
  • schema definition lookups & autocomplete crossing into the wrong workspace (project name wasn't being resolved when building the cache keys! literally just one line!)
  • type validation works in more places it didn't work before because of the fixed fragment and schema type cache bugs!
  • autocompletion for SDL type fields and input type fields is restored! 🎉
  • the lsp server only reloads the config when/if package.json contains a graphql entry

Known Bugs Fixed

Test Improvements

  • new, high level integration spec suite for the LSP with a matching test utility
  • more unit test coverage
  • total increased test coverage of about 23% in the LSP server codebase.
  • many "happy paths" covered for both schema and code first contexts
  • many bugs revealed (and their source)

What Happens After This?:

  • full schema lifecycle for "code first"/ cached schema file context (i.e. schema from URL updates when it changes and re-saves the file, at least with a configurable timeout)

Is there a Prerelease? #

Try it out here! Just install the .vsix in the .zip. If you still have any issues, please report them in the applicable bug ticket, noting there is still an issue with the new 0.10.x release. We will try to capture all edge cases in the new test suite

https://github.com/graphql/graphiql/files/14475534/vscode-bugfixes.zip

Test methodology thoughts

overall, this new quasi behavioral integration spec plane serves as a learning tool for contributors, a refactoring tool (I may have a stash for a new chained branch ready to go haha) and a tool for discovering bugs and readily identifying their sources, even allowing us to use a BDD approach to fixing bugs (and soon adding features!) 😍 !!

the only major caveat of this testing approach is that, like with many high level, quasi-e2e integration tests, these rely on our assumptions of how LSP clients are using our service interfaces. This is something that changes ever so slightly and even in semi-breaking ways in vscode (like with the introduction of multi-root workspaces), so careful readings of the vscode-languageserver changelog and probably also the LSP protocol spec will be required to maintain this spec, and not cause mysterious false positives

Copy link

changeset-bot bot commented Jan 27, 2024

🦋 Changeset detected

Latest commit: 672a83d

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codecov bot commented Jan 27, 2024

Codecov Report

Attention: Patch coverage is 65.92593% with 138 lines in your changes are missing coverage. Please review.

Project coverage is 58.70%. Comparing base (9d42a25) to head (672a83d).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3521      +/-   ##
==========================================
+ Coverage   55.33%   58.70%   +3.36%     
==========================================
  Files         115      117       +2     
  Lines        5349     5555     +206     
  Branches     1450     1481      +31     
==========================================
+ Hits         2960     3261     +301     
+ Misses       1963     1849     -114     
- Partials      426      445      +19     
Files Coverage Δ
packages/codemirror-graphql/src/hint.ts 88.23% <100.00%> (+0.73%) ⬆️
packages/codemirror-graphql/src/info.ts 0.81% <ø> (ø)
packages/graphiql-react/src/editor/query-editor.ts 59.88% <ø> (ø)
...aphql-language-service-server/src/parsers/babel.ts 94.44% <100.00%> (+16.66%) ⬆️
...anguage-service/src/interface/autocompleteUtils.ts 84.74% <100.00%> (+0.26%) ⬆️
...guage-service/src/utils/validateWithCustomRules.ts 80.00% <ø> (ø)
...ckages/codemirror-graphql/src/utils/getTypeInfo.ts 0.00% <0.00%> (ø)
...raphql-language-service-server/src/GraphQLCache.ts 74.07% <97.36%> (+25.31%) ⬆️
...graphql-language-service-server/src/startServer.ts 2.32% <0.00%> (ø)
...hql-language-service-server/src/findGraphQLTags.ts 86.04% <84.61%> (+1.43%) ⬆️
... and 10 more

... and 2 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented Jan 27, 2024

The latest changes of this PR are not available as canary, since there are no linked changesets for this PR.

@acao acao force-pushed the recent-missing-coverage branch 2 times, most recently from 5b8e336 to 1df8b2e Compare January 27, 2024 21:19
@acao acao changed the title test after coverage for recent features/changes 😬 more LSP test coverage! Jan 28, 2024
@acao acao force-pushed the recent-missing-coverage branch 3 times, most recently from 2ff4224 to c22b0c6 Compare February 5, 2024 01:18
@acao acao changed the title more LSP test coverage! fix many bugs, more LSP test coverage! Feb 11, 2024
@acao acao force-pushed the recent-missing-coverage branch 3 times, most recently from 6d549a2 to a69c816 Compare February 16, 2024 07:50
@acao acao force-pushed the recent-missing-coverage branch 2 times, most recently from bfd95e0 to 02d6a9c Compare February 27, 2024 20:37
@acao acao force-pushed the recent-missing-coverage branch 3 times, most recently from de8b2ec to 6839f3f Compare March 4, 2024 01:16
@acao
Copy link
Member Author

acao commented Mar 4, 2024

For anyone who wants to give it a try, here is a zip of the 0.10.0 vsix!
vscode-bugfixes.zip

Copy link
Contributor

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small, human-facing

.changeset/rotten-seahorses-fry.md Outdated Show resolved Hide resolved
.changeset/rotten-seahorses-fry.md Outdated Show resolved Hide resolved
.changeset/rotten-seahorses-fry.md Show resolved Hide resolved
@acao acao changed the title fix many bugs, more LSP test coverage! LSP Rewrite Step 1: fix many bugs, more LSP test coverage! Mar 17, 2024
@acao acao force-pushed the recent-missing-coverage branch from f24bb65 to ea57543 Compare May 1, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants