Added integration tests, added back test_simple_workspace #607

bvinc · 2017-12-01T08:15:02Z

This should be a path to fixing #524, and a fix for #455. I have borrowed some code from cargotest, and from the current test harness, and I've created a new test integration test harness.

Each test is isolated and has it's own working directory.
The files for each project can be specified right in the test.
The integration test directory gets cleaned up before each test run.
Each test launches a process of rls, with complete control over its own environment variables.
Because of these things, each test should run in parallel without any problems.
This tests at the stdin/stdout protocol layer which tests a little bit more of the code, for example, a stray println would break rls, but the previous tests would not fail.
Tests from the other test harness should convert into these integration tests easily, if you wanted to do that.
I was unable to come up with a good way to time-limit the tests.

Xanewok · 2017-12-01T21:43:13Z

Nice! I think that's the best approach here, using cargotest, generating test data in the target/ directory and spawning separate rls processes. It fixes/does all the thing we wanted, good job!

Integration tests are nice, but it's a shame we lose helper types and are back to unstructured JSON messages. At some point I believe it'd be good to split it into bin/lib, so we can at least the types here.

Just wanted to chime in to thank you for working on this 👍 . At first glance this looks good for me, but I'll come back and do a more detailed review in the morning, if you don't mind.

Xanewok · 2017-12-02T12:35:22Z

tests/support/harness.rs

+pub struct RlsHandle {
+    child: Child,
+    stdin: ChildStdin,
+    stdout: BufReader<ChildStdout>,


Why is this buffered? So we can easily only read a message when a whole message is ready to be read?

So that I can search for a \n using read_line when reading the headers like Content-Length: XX

Xanewok · 2017-12-02T12:37:14Z

tests/support/mod.rs

+        }
+
+        let ProjectBuilder{ name: _, root, files: _, .. } = self;
+        root


Aren't lines 92-93 just equivalent to returning self.root here?

Yeah you seem to be right. I'll simplify this when I get a chance tomorrow.

bvinc · 2017-12-04T05:17:23Z

I simplified that one line of code.

Integration tests are nice, but it's a shame we lose helper types and are back to unstructured JSON messages. At some point I believe it'd be good to split it into bin/lib, so we can at least the types here.

Aren't the types in the crate languageserver_types? I don't see why they can't be used in the tests. I personally think, for a program like rls, the stdin/stdout protocol layer seems to be a fairly simple and stable place to do integration tests. You also want the tests to be complete, from end-to-end, ie, you should be able to send it incorrect JSON if you wanted to, and be able to analyze every byte printed to stdout.

BTW, my biggest concern with this PR is that I didn't spend much time thinking about the different environments/OSes the tests might be ran, and how exactly to launch the rls binary in each instance. For example, I didn't even worry about setting LD_LIBRARY_PATH before launching rls. But since I've never seen it fail, and the CI is successful, I guess cargo is already taking care of that.

Xanewok · 2017-12-06T10:26:34Z

You also want the tests to be complete, from end-to-end, ie, you should be able to send it incorrect JSON if you wanted to, and be able to analyze every byte printed to stdout.

I agree. Just wanted to note that sometimes you want to focus the test on different parts than the input handling, where it's convenient to assume correct input, hence why reusing existing functionality, e.g. being able to pass structured input and analyze received structured output is convenient.
Recently languageserver-types merged a trait facilitating parsing generic messages, so that's mostly not an issue anymore.

BTW, my biggest concern with this PR is that I didn't spend much time thinking about the different environments/OSes the tests might be ran, and how exactly to launch the rls binary in each instance. For example, I didn't even worry about setting LD_LIBRARY_PATH before launching rls.

So what we're most concerned with here is how the tests are ran in the Rust CI environment. I did dig around and talked with @alexcrichton about that - it seems that we basically execute cargo test with LD_LIBRARY_PATH set to point at built rustc - neat!

Also checked with a local Rust build and it seems to work, however with a small caveat. Because we don't implement a timeout here, my test harness just hung on a test_simple_workspace, possibly because I'm using the current version where workspace mode is disabled by default.

We need to ensure that the test suite doesn't hang in the Rust CI, so for this to land we also need to implement the timeout. I'll play around and see if we can easily bolt on some timeouts here.

bvinc · 2017-12-07T04:54:16Z

I implemented a timeout at the top of each test. I guess it's a good place for a timeout and it can cover any possible reason the test might take a while. I had previously tried to add a timeout to the read_message function and that wasn't working out very well.

bvinc · 2017-12-07T05:45:00Z

I've updated simple_workspace_test so that it passes when workspace mode is off by default. I've also included comments of how to change it if it's ever reenabled.

nrc · 2017-12-10T22:02:23Z

You also want the tests to be complete, from end-to-end, ie, you should be able to send it incorrect JSON if you wanted to, and be able to analyze every byte printed to stdout.

I'm not sure about this. On one hand, yes, we kind of do to make it a proper integration test. On the other hand, the stdin/stdout layer is pretty trivial and unlikely to go wrong, so I'd rather make it more ergonomic to write tests for the common case, rather than be able to test this.

nrc

Thanks for this PR! It looks good. I left a few comments inline, but I'll wait for Xanewok to re-review too. In particular, we need to be sure this will work inside the Rust repo.

nrc · 2017-12-10T22:04:11Z

tests/support/mod.rs

+
+use support::paths::TestPathExt;
+
+pub mod harness;


Why are some things here and some things in harness, istm they are all kind of related and should be in the same module, or maybe split in some other way.

nrc · 2017-12-10T22:05:31Z

tests/tests.rs

 mod support;
 use support::{basic_bin_manifest, project};
 use support::harness::{ExpectedMessage, RlsHandle};

+fn timeout<F>(dur: Duration, func: F)


this function should probably be in one of the support modules

nrc · 2017-12-10T22:06:38Z

tests/tests.rs

 mod support;
 use support::{basic_bin_manifest, project};
 use support::harness::{ExpectedMessage, RlsHandle};

+fn timeout<F>(dur: Duration, func: F)
+    where F: FnOnce() + Send + 'static {
+    let pair = Arc::new((Mutex::new(false), Condvar::new()));


Making this pair doesn't seem worthwhile to me, you could just clone the mutex and condvar separately in fewer lines of code.

I'm not sure what you mean here. I don't actually want to clone the mutex and condvar and I don't see how I could do it in less lines. BTW, this code follows the rust stdlib example code for how to work with CondVar.

I meant you could write:

let mutex = Arc::new(Mutex::new(...)); let condvar = Arc::new(Condvar::new());

(and clone the Arcs, not the mutex/condvar). I guess having a pair saves an Arc but it seems a bit convoluted to me.

Either way is fine, though.

Read the stdlib regarding Condvar + examples and additionally pthread API - this is surprisingly unergonomic to use! I tried to simplify it somehow, but couldn't. I'd expect it to be much easier to run a task in thread with a timeout, but it feels like one has to do the all the machinery by hand, just like in C.

nrc · 2017-12-10T22:07:21Z

tests/tests.rs

-    ]);
-
-    rls.shutdown_exit();
+    timeout(Duration::from_secs(300), ||{


Could you factor the timeout into a const please?

Xanewok · 2017-12-19T18:56:06Z

Sorry I took so long to review!
Regarding the Rust repo, this should still work as expected, as per my previous #607 (comment).
With nits by @nrc fixed and all CI passes greenlit, to be extra sure, I think we could land this and possibly improve it from there, if needed.

Xanewok · 2017-12-21T21:22:17Z

Just as a note, with rustc 1.24.0-nightly (dc39c3169 2017-12-17) , I get this when running cargo test --verbose:

running 28 tests
test server::test::test_parse_as_notification ... ok
test server::test::test_use_root_path ... ok
test server::test::test_use_root_uri ... ok
test build::cargo::test::test_dedup_flags ... ok
test actions::test::test_find_word_at_pos ... ok
error: process didn't exit successfully: `/home/xanewok/repos/rls/target/debug/deps/rls-0e215db6b7f4a1c9` (signal: 4, SIGILL: illegal instruction)

on Ubuntu 17.10 x86_64 4.13.0-19-generic. I'll run rustup update and see if this helps.
EDIT: Fixed with 2333a28 already.

nrc · 2018-01-12T04:42:39Z

@Xanewok @bvinc Sorry this PR has hung around a bit. Is it OK to merge now? It looks like all my comments have been addressed and CI is OK

Xanewok · 2018-01-12T09:58:54Z

@nrc workspace mode is on now by default, so this will need to change still. I'll open a second PR with this commit

Add integration tests based on #607

Xanewok reviewed Dec 2, 2017

View reviewed changes

bvinc force-pushed the int_tests branch from 62da6cc to f8728bd Compare December 3, 2017 22:25

bvinc added 2 commits December 6, 2017 21:37

Added integration tests, added back test_simple_workspace

4b8265d

Added a timeout to the integration tests

98e9bda

bvinc force-pushed the int_tests branch from f8728bd to 98e9bda Compare December 7, 2017 04:48

Fixed simple_workspace_test for workspace_mode being off

8a2bcca

nrc reviewed Dec 10, 2017

View reviewed changes

Refactored integration tests

5223549

Xanewok mentioned this pull request Jan 12, 2018

Add integration tests based on #607 #654

Merged

Xanewok merged commit 5223549 into rust-lang:master Jan 12, 2018

Xanewok added a commit that referenced this pull request Jan 12, 2018

Merge pull request #654 from Xanewok/int_tests2

b3126a8

Add integration tests based on #607

This was referenced Jan 12, 2018

Running tests in parallel #524

Closed

Workspace tests #455

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added integration tests, added back test_simple_workspace #607

Added integration tests, added back test_simple_workspace #607

bvinc commented Dec 1, 2017

Xanewok commented Dec 1, 2017

Xanewok Dec 2, 2017

bvinc Dec 3, 2017

Xanewok Dec 2, 2017

bvinc Dec 3, 2017

bvinc commented Dec 4, 2017

Xanewok commented Dec 6, 2017

bvinc commented Dec 7, 2017 •

edited

Loading

bvinc commented Dec 7, 2017

nrc commented Dec 10, 2017

nrc left a comment

nrc Dec 10, 2017

nrc Dec 10, 2017

nrc Dec 10, 2017

bvinc Dec 13, 2017

nrc Dec 14, 2017

nrc Dec 14, 2017

Xanewok Dec 19, 2017

nrc Dec 10, 2017

Xanewok commented Dec 19, 2017

Xanewok commented Dec 21, 2017 •

edited

Loading

nrc commented Jan 12, 2018

Xanewok commented Jan 12, 2018

Added integration tests, added back test_simple_workspace #607

Added integration tests, added back test_simple_workspace #607

Conversation

bvinc commented Dec 1, 2017

Xanewok commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvinc commented Dec 4, 2017

Xanewok commented Dec 6, 2017

bvinc commented Dec 7, 2017 • edited Loading

bvinc commented Dec 7, 2017

nrc commented Dec 10, 2017

nrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok commented Dec 19, 2017

Xanewok commented Dec 21, 2017 • edited Loading

nrc commented Jan 12, 2018

Xanewok commented Jan 12, 2018

bvinc commented Dec 7, 2017 •

edited

Loading

Xanewok commented Dec 21, 2017 •

edited

Loading