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

Force recalc of line offsets for each document change #405

Merged
merged 3 commits into from Oct 30, 2019

Conversation

non-Jedi
Copy link
Member

@non-Jedi non-Jedi commented Oct 28, 2019

Closes #403

The problem is that Document._line_offsets isn't updated until all changes from a particular rpc message have been processed. I think this bug hasn't been seen before because textDocument/didChange with multiple changes are rare in practice. There may be a more performant way of updating the offsets for each change, but this should at least fix the problem.

@non-Jedi
Copy link
Member Author

Side note: I could write some quick and dirty tests that confirm this is working correctly, but all of the existing tests seem to be much more end-to-end in scope. Should I go ahead and write said tests even if they don't fit very well with those already existing?

Side side note: hard to see much benefit to writing an additional test when the tests and CI have been broken for so long.

@davidanthoff
Copy link
Member

Should I go ahead and write said tests even if they don't fit very well with those already existing?

Yes, I think that would be great! In general, it would actually be fantastic if we didn't just have these end-to-end tests, but proper unit tests.

hard to see much benefit to writing an additional test when the tests and CI have been broken for so long

I just went through the whole packages, tagged some releases and tried to fix as much CI problems as I can right now. Much is passing now, with the following exceptions: Julia 1.3 32 bit is broken (not our fault), Julia 1.0 and 1.1 are broken for anything downstream from SymbolServer.jl due to julia-vscode/SymbolServer.jl#49. I think all other tests should be passing right now on all the master branches, so further tests should definitely help! And hopefully we can get to green everywhere soon, once we have a working Julia 1.3 build on windows and fixed that one bug in SymbolServer.jl :)

@non-Jedi
Copy link
Member Author

non-Jedi commented Oct 29, 2019

> Much is passing now, with the following exceptions: Julia 1.3 32 bit is broken (not our fault), Julia 1.0 and 1.1 are broken for anything downstream from SymbolServer.jl due to [julia-vscode/SymbolServer.jl#49](https://github.com/julia-vscode/SymbolServer.jl/issues/49). I think all other tests should be passing right now on all the `master`

I can't get tests to pass on Julia 1.2 locally (linux AMD64), and it looks like they're also failing on CI: https://travis-ci.org/julia-vscode/LanguageServer.jl/jobs/603967242. The interesting bit is that if you simply include("test_communication.jl") instead of running it with Pkg.test(), no error is thrown.

EDIT: I see there have been additional changes that got a lot of tests running since I opened the PR. Will rebase and let CI run again. Please ignore this comment, and thanks for all your work in getting the tests working again. It's much appreciated.

@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

Merging #405 into master will increase coverage by 0.16%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #405      +/-   ##
==========================================
+ Coverage   32.97%   33.14%   +0.16%     
==========================================
  Files          22       22              
  Lines        1219     1219              
==========================================
+ Hits          402      404       +2     
+ Misses        817      815       -2
Impacted Files Coverage Δ
src/requests/textdocument.jl 55.34% <100%> (+1.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52f936b...c664531. Read the comment docs.

@non-Jedi non-Jedi force-pushed the fix-multiple-didChange branch 2 times, most recently from 2a7dc09 to f6965e8 Compare October 29, 2019 12:36
@non-Jedi
Copy link
Member Author

This should be good to go. I feel much better about the correctness of the change since it unbroke a broken test.

@davidanthoff davidanthoff reopened this Oct 29, 2019
@davidanthoff davidanthoff added this to In progress in Current via automation Oct 29, 2019
@davidanthoff davidanthoff added this to the v0.7.0 milestone Oct 29, 2019
@davidanthoff
Copy link
Member

@ZacLN same thing as with the others: I'm merging this, but would be good if you could review after the fact.

@davidanthoff davidanthoff merged commit f31248d into julia-vscode:master Oct 30, 2019
Current automation moved this from In progress to Done Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Current
  
Done
Development

Successfully merging this pull request may close these issues.

LangaugeServer.jl crashing on a "didChange" request when using with eglot (Emacs)
2 participants