Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

Translate cmd tests to use async LSP client #1244

Merged
merged 16 commits into from
Jan 22, 2019

Conversation

Xanewok
Copy link
Member

@Xanewok Xanewok commented Jan 20, 2019

This translates our cmd-based tests to use the LSP client introduced in #1223.

Some anecdotal benchmarks (Xubuntu 18.04, Ryzen 2600) before and after translating:

$ time cargo test cmd_
real    0m3,498s
user    0m8,379s
sys     0m2,710s

$ time cargo test cmd_ --release
real    0m3,059s
user    0m5,585s
sys     0m2,512s

$ time cargo test client_
real    0m3,465s
user    0m7,197s
sys     0m1,926s

$ time cargo test client_ --release
real    0m3,053s
user    0m4,977s
sys     0m1,825s

It seems that synchronization overhead caused by running multiple RLS and rustc instances in-process still outweighs spawning separate processes per test 🎉 That means we don't lose performance (and actually benefit) from switching over.

I plan on translating the remaining tests in tests_old as a separate PR.

r? @alexheretic

@alexheretic
Copy link
Member

Yep I can't easily run this at the moment with the compilation issues, but the direction looks good to me.

This is test code too so I'd just go ahead and merge.

@alexheretic
Copy link
Member

let bin = rls.future_diagnostics("src/main.rs");
let bin = rls.runtime().block_on(bin).unwrap();

Is it true that these test block forever now, instead of timing out? That seems bad, as the possibility of the message never arriving is definitely something that the test should handle in a useful way.

@Xanewok
Copy link
Member Author

Xanewok commented Jan 21, 2019 via email

@bors
Copy link
Contributor

bors commented Jan 22, 2019

☔ The latest upstream changes (presumably #1230) made this pull request unmergeable. Please resolve the merge conflicts.

Pulls lsp-codec 0.1.1 due to message send buffer reservation fix.
@Xanewok
Copy link
Member Author

Xanewok commented Jan 22, 2019

Instead of exposing bare Runtime, we now allow to block_on in test code where we always add a timeout, so the user test code can't block indefinitely.

@bors r=alexheretic

@bors
Copy link
Contributor

bors commented Jan 22, 2019

📌 Commit 663e437 has been approved by alexheretic

bors added a commit that referenced this pull request Jan 22, 2019
Translate cmd tests to use async LSP client

This translates our cmd-based tests to use the LSP client introduced in #1223.

Some anecdotal benchmarks (Xubuntu 18.04, Ryzen 2600) before and after translating:
```
$ time cargo test cmd_
real    0m3,498s
user    0m8,379s
sys     0m2,710s

$ time cargo test cmd_ --release
real    0m3,059s
user    0m5,585s
sys     0m2,512s

$ time cargo test client_
real    0m3,465s
user    0m7,197s
sys     0m1,926s

$ time cargo test client_ --release
real    0m3,053s
user    0m4,977s
sys     0m1,825s
```
It seems that synchronization overhead caused by running multiple RLS and rustc instances in-process still outweighs spawning separate processes per test 🎉 That means we don't lose performance (and actually benefit) from switching over.

I plan on translating the remaining tests in tests_old as a separate PR.

r? @alexheretic
@bors
Copy link
Contributor

bors commented Jan 22, 2019

⌛ Testing commit 663e437 with merge 968129d...

@bors
Copy link
Contributor

bors commented Jan 22, 2019

☀️ Test successful - checks-travis
Approved by: alexheretic
Pushing 968129d to master...

@bors bors merged commit 663e437 into rust-lang:master Jan 22, 2019
@Xanewok Xanewok deleted the translate-tests branch January 22, 2019 20:07
bors added a commit to rust-lang/rust that referenced this pull request Feb 2, 2019
submodule: update rls from c9d25b to e2145d

Update rls rust-lang/rls@c9d25b6...e2145d

rust-lang/rls#1276 - h-michael:clippy, r=Xanewok
rust-lang/rls#1269 - rust-lang:dependabot/cargo/rand-0.6.5, r=Xanewok
Remove extra backticks in contributing.md
rust-lang/rls#1267 from h-michael/contributingmd
rust-lang/rls#1268 from matthiaskrgr/rustup
rust-lang/rls#1262 from rust-lang/dependabot/cargo/tokio-0.1.15
rust-lang/rls#1264 - h-michael:pub-crate, r=alexheretic
rust-lang/rls#1261 - rust-lang:dependabot/cargo/tokio-timer-0.2.9, r=Xanewok
rust-lang/rls#1263 - Xanewok:update-clippy, r=Xanewok
rust-lang/rls#1257 from Xanewok/architecture
rust-lang/rls#1258 - rust-lang:dependabot/cargo/lsp-types-0.55.1, r=Xanewok
rust-lang/rls#1255 - Xanewok:you-only-complete-once-fool, r=Xanewok
rust-lang/rls#1252 - rust-lang:dependabot/cargo/cargo_metadata-0.7.0, r=alexheretic
rust-lang/rls#1253 - rust-lang:dependabot/cargo/lsp-types-0.55.0, r=Xanewok
rust-lang/rls#1254 - rust-lang:dependabot/cargo/serde_json-1.0.37, r=Xanewok
dependabot: Explicitly list default allowed_updates
dependabot: Add automerge strategy for clippy_lints
rust-lang/rls#1251 - Xanewok:translate-deglob-test, r=Xanewok
rust-lang/rls#1250 from alexheretic/master
rust-lang/rls#1244 - Xanewok:translate-tests, r=alexheretic
rust-lang/rls#1247 - alexheretic:register-more-clippy, r=Xanewok
rust-lang/rls#1230 - emilio:testing-testing, r=Xanewok
rust-lang/rls#1246 from alexheretic/did-save-manifest
Merge branch 'beta-version-bump' of https://github.com/rust-lang-nursery/rls
bors added a commit to rust-lang/rust that referenced this pull request Feb 3, 2019
submodule: update rls from c9d25b to f331ff7

Update rls rust-lang/rls@c9d25b6...e2145d

rust-lang/rls#1276 - h-michael:clippy, r=Xanewok
rust-lang/rls#1269 - rust-lang:dependabot/cargo/rand-0.6.5, r=Xanewok
Remove extra backticks in contributing.md
rust-lang/rls#1267 from h-michael/contributingmd
rust-lang/rls#1268 from matthiaskrgr/rustup
rust-lang/rls#1262 from rust-lang/dependabot/cargo/tokio-0.1.15
rust-lang/rls#1264 - h-michael:pub-crate, r=alexheretic
rust-lang/rls#1261 - rust-lang:dependabot/cargo/tokio-timer-0.2.9, r=Xanewok
rust-lang/rls#1263 - Xanewok:update-clippy, r=Xanewok
rust-lang/rls#1257 from Xanewok/architecture
rust-lang/rls#1258 - rust-lang:dependabot/cargo/lsp-types-0.55.1, r=Xanewok
rust-lang/rls#1255 - Xanewok:you-only-complete-once-fool, r=Xanewok
rust-lang/rls#1252 - rust-lang:dependabot/cargo/cargo_metadata-0.7.0, r=alexheretic
rust-lang/rls#1253 - rust-lang:dependabot/cargo/lsp-types-0.55.0, r=Xanewok
rust-lang/rls#1254 - rust-lang:dependabot/cargo/serde_json-1.0.37, r=Xanewok
dependabot: Explicitly list default allowed_updates
dependabot: Add automerge strategy for clippy_lints
rust-lang/rls#1251 - Xanewok:translate-deglob-test, r=Xanewok
rust-lang/rls#1250 from alexheretic/master
rust-lang/rls#1244 - Xanewok:translate-tests, r=alexheretic
rust-lang/rls#1247 - alexheretic:register-more-clippy, r=Xanewok
rust-lang/rls#1230 - emilio:testing-testing, r=Xanewok
rust-lang/rls#1246 from alexheretic/did-save-manifest
Merge branch 'beta-version-bump' of https://github.com/rust-lang-nursery/rls
bors added a commit that referenced this pull request Feb 24, 2019
Translate remaining tests

Continuation of #1244.

Here are some anecdotal benchmarks (used `time` under Xubuntu 18.04, AMD Ryzen 2600).

* master branch

`$ time cargo test --release client_` 5.5 real (2.5 sys)
`$ time cargo test --release test_ `8.65 real (3.2 sys)

* translate-tests branch

`$ time cargo test --release client_` 5.7 real (6.3 sys)
`$ time cargo test --release test_` 1.3 real (0.8 sys)

Unfortunately it seems that two tests post-translation fail very often (which I disabled for now):
- client_hover_after_src_line_change (sometimes has to fall back to Racer, which is bad)
- client_find_all_refs_test (sometimes doesn't pick up reference from `#[cfg(test)]` code - maybe analysis chokes when coalescing defs from the 2 crates with same origin but different cfgs?)

This is alarming since the new approach should isolate the behaviour better (we start a new process in a scratchpad directory), which means that either I'm not seeing what I'm doing wrong with the new approach or it uncovered some bugs with the rls-analysis (seems like an obvious culprit for both of these failures).

r? @alexheretic
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.

3 participants