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

Provide LS analysis progress display in the status bar #2099

Merged
merged 9 commits into from
Jul 10, 2018
Merged

Provide LS analysis progress display in the status bar #2099

merged 9 commits into from
Jul 10, 2018

Conversation

MikhailArkhipov
Copy link

Fixes #1591
Also fixes issue when LS extension loading failed b/c occasionally languageClient.onReady() never fires.

This pull request:

  • Has a title summarizes what is changing
  • Includes a news entry file (remember to thank yourself!)
  • Has unit tests & code coverage is not adversely affected (within reason)
  • Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
  • Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)

@codecov
Copy link

codecov bot commented Jul 5, 2018

Codecov Report

Merging #2099 into master will increase coverage by 0.45%.
The diff coverage is 29.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2099      +/-   ##
==========================================
+ Coverage    79.4%   79.85%   +0.45%     
==========================================
  Files         307      308       +1     
  Lines       14083    14106      +23     
  Branches     2499     2502       +3     
==========================================
+ Hits        11183    11265      +82     
+ Misses       2888     2829      -59     
  Partials       12       12
Flag Coverage Δ
#MacOS 74.06% <29.03%> (?)
#Windows 74.11% <29.03%> (?)
Impacted Files Coverage Δ
src/client/common/types.ts 100% <ø> (ø) ⬆️
src/client/activation/progress.ts 28.57% <28.57%> (ø)
src/client/activation/analysis.ts 26.13% <30%> (+0.27%) ⬆️
src/client/debugger/PythonProcess.ts 58.33% <0%> (-0.84%) ⬇️
src/client/debugger/Main.ts 66.91% <0%> (-0.25%) ⬇️
src/client/providers/symbolProvider.ts 86.53% <0%> (+1.92%) ⬆️
src/client/interpreter/serviceRegistry.ts 100% <0%> (+2.04%) ⬆️
src/client/interpreter/virtualEnvs/index.ts 93.61% <0%> (+2.12%) ⬆️
...nterpreter/locators/services/currentPathService.ts 93.47% <0%> (+2.17%) ⬆️
... and 13 more

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 2f989b7...7787290. Read the comment docs.

@qubitron
Copy link

qubitron commented Jul 6, 2018

I can't seem to find it in your set of changes, what is the message you are presenting to users? Should be something like "Loading IntelliSense results..."

Copy link

@d3r3kk d3r3kk left a comment

Choose a reason for hiding this comment

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

This all looks just fine, simple reporting of progress through the window status areas and all that.

However, I could not find any tests (unit or otherwise) that pertain to this change, nor to the language server setup at all.

I think it would be prudent to put in at least some unit tests around the AnalysisExtensionActivator, if not some system testing. Don's provided us with some good mock items from vscode if that helps.

@MikhailArkhipov
Copy link
Author

MikhailArkhipov commented Jul 10, 2018

@qubitron - it looks like Analyzing workspace, X items remaining.... Actual string in in the LS, see microsoft/PTVS#4483

@MikhailArkhipov
Copy link
Author

MikhailArkhipov commented Jul 10, 2018

@d3r3kk - they are under src/test/activation. LS setup is covered by LS tests ex src/test/definitions/hover.ptvs.test.ts. These tests typically have (Language Server) or (Analysis Engine) in their names. The is a bunch and they definitely fail if LS is not properly setup. Platform selection and activation sequence have tests as above.

Yes, there are some gaps, specifically around downloader and general LS message traffic simulation. The latter is much harder to do due to the fact code works directly with progress module and the file system. A bit too much to abstract atm.

@d3r3kk
Copy link

d3r3kk commented Jul 10, 2018

Ok, I didn't realize there was coverage in place, so I am glad to hear it is there. Testing the message queue is less important to me than the language server setup/etc.

@MikhailArkhipov MikhailArkhipov merged commit a1b2c55 into microsoft:master Jul 10, 2018
@MikhailArkhipov MikhailArkhipov deleted the progress branch July 10, 2018 22:12
@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report analysis progress from the engine
3 participants