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

Merged
merged 75 commits into from
May 28, 2024

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: f6b3e6c

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 78.02385% with 129 lines in your changes are missing coverage. Please review.

Project coverage is 60.80%. Comparing base (03ab3a6) to head (f6b3e6c).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3521      +/-   ##
==========================================
+ Coverage   55.32%   60.80%   +5.48%     
==========================================
  Files         115      120       +5     
  Lines        5352     5616     +264     
  Branches     1451     1487      +36     
==========================================
+ Hits         2961     3415     +454     
+ Misses       1965     1749     -216     
- Partials      426      452      +26     
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%) ⬆️
...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.43%> (+25.31%) ⬆️
...guage-service/src/interface/getHoverInformation.ts 79.64% <90.90%> (+15.49%) ⬆️
...hql-language-service-server/src/findGraphQLTags.ts 86.04% <84.61%> (+1.43%) ⬆️
...aphql-language-service-server/src/parsers/astro.ts 42.85% <0.00%> (+38.09%) ⬆️
... and 12 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 fb5285e to 798d782 Compare May 19, 2024 07:55
@acao acao force-pushed the recent-missing-coverage branch from c4098e5 to 7e32382 Compare May 19, 2024 10:11
@acao acao force-pushed the recent-missing-coverage branch 4 times, most recently from 5a03f38 to 0496caa Compare May 19, 2024 22:25
@acao acao force-pushed the recent-missing-coverage branch from 0496caa to 203a2c0 Compare May 19, 2024 22:25
@acao acao force-pushed the recent-missing-coverage branch from 745a642 to 496c364 Compare May 26, 2024 11:46
@acao acao force-pushed the recent-missing-coverage branch from 697e406 to 66b7bf2 Compare May 26, 2024 18:20
@acao acao force-pushed the recent-missing-coverage branch 2 times, most recently from 5773bcb to fb7d418 Compare May 27, 2024 19:04
@acao acao force-pushed the recent-missing-coverage branch from fb7d418 to e5ab31e Compare May 27, 2024 19:43
@acao acao merged commit aa6dbbb into main May 28, 2024
14 checks passed
@acao acao deleted the recent-missing-coverage branch May 28, 2024 06:27
@acao acao mentioned this pull request May 28, 2024
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.

2 participants