Allow to change huge project file limit

[jakubfijalkowski]

Hello!

I've recently stumbled upon the huge project file limit. On project with ~1.1k files (~100k LOC) and i7-7700HQ it works reasonably well, so configuring it would be good. This PR adds the omnisharp.hugeProjectLimit option.

Fixes #838 and #1525.

[TheRealPiotrP]

@DustinCampbell how do you feel about the configuration name? Did we intend to use huge as the descriptor in public surface area?

[jakubfijalkowski]

Also, I'm not quite sure if using null to disable the feature and ignoring values less than zero (setting it to zero or less will treat every project huge) is good - wouldn't it be better to remove the null and using non-positive values instead?

[jakubfijalkowski]

Sorry for the delay. Yes, it might huge project might be too internal (although I got used to the name - it makes me feel like my project really is big :) ) I've updated the name to the one you have proposed.

[gregg-miskelly]

@DustinCampbell should this be reviewed again?

[jakubfijalkowski]

Hey,

This has bitten me once again, so I decided to review the PR. I've re-applied the changes (after the comments from @DustinCampbell) on the current master. May I ask for re-review?

I've tried to add some test suite, but my TS/vscode-fu is less than optimal and failed miserably. I'm willing to add them, but need some guidance (e.g. how to properly check for diagnostics that, AFAIK, are populated asynchronously).

[codecov]

Codecov Report

Merging #1877 into master will increase coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1877      +/-   ##
==========================================
+ Coverage   65.04%   65.27%   +0.22%     
==========================================
  Files          99       99              
  Lines        4320     4328       +8     
  Branches      630      631       +1     
==========================================
+ Hits         2810     2825      +15     
+ Misses       1321     1316       -5     
+ Partials      189      187       -2

Flag	Coverage Δ
#integration	53.18% <100%> (+0.25%)	⬆️
#unit	84.84% <100%> (+0.02%)	⬆️

Impacted Files	Coverage Δ
src/main.ts	92.24% <100%> (+0.2%)	⬆️
src/omnisharp/extension.ts	79.64% <100%> (-0.18%)	⬇️
src/omnisharp/options.ts	100% <100%> (ø)	⬆️
src/features/diagnosticsProvider.ts	78.66% <100%> (+1.95%)	⬆️
src/omnisharp/server.ts	70.4% <0%> (+0.68%)	⬆️
src/omnisharp/requestQueue.ts	80.3% <0%> (+4.54%)	⬆️

---

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 be0ec9a...1460622. Read the comment docs.

[akshita31]

@jakubfijalkowski Apologies for the delayed response here. We will have to add an integrtaion test for the diagnostics provider. You can look up similar files in the integration test folder to see how they wait on the appropriate response from vscode. You can read more about writing the integration tests here : https://github.com/OmniSharp/omnisharp-vscode/wiki/Writing-tests#integration-tests.

[jakubfijalkowski]

@akshita31 Thank you for the links! I'll try to write them after the weekend.

[jakubfijalkowski]

I've tried to write the tests, but I don't think it is easily possible with current Advisor/DiagnosticsProvider design. None of these is published outside of activate in omnisharp/extension.ts and, as far as I know, vscode does not provide "wait for diagnostics to be generated" command. "Diagnostics providers" are not first-class citizens, but just background processes (or at least they are in omnisharp-vscode) that, in omnisharp-vscode case, do not report completion (setTimeout handlers in both _validateDocument and _validateProject do not raise any events). This makes it terrible to test - we would need to wait for the diagnostics to show for some undefined time (i.e. the tests would be non-deterministic).

My "simple" idea was to just test Advisor (as this PR changes just that class) by exporting it from omnisharp/extension.ts using global variable, but as you can see this fails in the Travis build (and I don't know why yet - I assume this has something to do with node version, as on my machine this works and I have node 11). This isn't pretty, but would have gotten the job done.

I think that a better approach would be to return the Advisor and/or DiagosticsProvider from activate in omnisharp/extension the same way OmniSharpServer is returned, and then expose both on CSharpExtensionExports. This would make the code cleaner and would not require hacky global variables, but is broader in scope.

What do you think, which way would be more appropriate?

[jakubfijalkowski]

Okay, just for tests I went with the correct approach and extended CSharpExtensionExports with getAdvisor. I've also rebased on current master. Tests pass now, but they are still for Advisor only - testing DiagnosticsProvider would be good, but I think separate PR would better serve this purpose. I am willing to do this, but I it would require much thorough changes to DiagnosticsProvider (so the check finish can be determined reliably).

[akshita31]

@jakubfijalkowski Should we be more specific with the tests and call out that we are testing the "Advisor" and not the "Diagnostic Provider" itself ?

Something like Advisor.ShouldValidateDocument returns true when...

[jakubfijalkowski]

@akshita31 You're right. Fixed!

[akshita31]

We would need a changelog update for this one. @jakubfijalkowski If you are interested you might do that in this or a separate PR.

[jakubfijalkowski]

@akshita31 Done, I've added the entry in this PR.

Resolved