Stream: t-compiler/wg-rls-2.0

Topic: Moving to external tests


matklad (Jan 30 2020 at 16:33, on Zulip):

Reading https://pingcap.com/blog/rust-compilation-model-calamity/ made me realised that a bunch of compile-time problems in rust-analyzer can be caused by extensive use of "unit-tests", which require building the code twice.

I wonder if we should maybe move all our unit-tests to integration tests, and maybe even make them more data-driven in the process?

matklad (Jan 30 2020 at 16:36, on Zulip):

There's also one thing which I don't know how to solve nicely:

Currently, each test is a separate #[test] function, and that is done primarily for the benefit of the debugging. I can imagine another setup, where most of the tests are just data-files, and the single #[test] just loops through them all. This is how our parser tests are implmented.

The problem would be, naturally, that you won't be able to execute a single test easily. But maybe that can be worked-around the same way as in the parser? Having a single smoke test into which you can paste specific test data?

https://github.com/rust-analyzer/rust-analyzer/blob/5f0b17b52dafa7b1eb1ba8934ce44857413410a7/crates/ra_syntax/src/tests.rs#L18-L28

std::Veetaha (Jan 30 2020 at 16:47, on Zulip):

In addition, FYI: currently if one of the data-driven tests fails, none of the others are run

std::Veetaha (Jan 30 2020 at 16:49, on Zulip):

As for the smoke test, I don't like this idea initially. Copy-pasting would be very tedious

std::Veetaha (Jan 30 2020 at 16:50, on Zulip):

Even if you want to follow it, i'd propose to extract test data for the test from source code so that we wouldn't have to recompile the project to change the smoke test.

matklad (Jan 30 2020 at 16:51, on Zulip):

If smoke test is in a separate crate, no recompilation would be necessary anyway

std::Veetaha (Jan 30 2020 at 16:54, on Zulip):

Sorry, maybe I used the wrong wording, but that smoke test you provided the link to contains the test data (i.e. the source code to parse) embedded in the test itself.
pasted image

If you want to change the smoke test you have to literally change that raw string literal. I'd propose to create a dedicated smoke-test.rs file in test-data directory for that.

std::Veetaha (Jan 30 2020 at 16:55, on Zulip):

It's just an enhancement that may be beside the point anyway...

std::Veetaha (Jan 30 2020 at 16:56, on Zulip):

I wish we ran data-driven test in separate processes so that all of them would run independently

matklad (Jan 30 2020 at 16:56, on Zulip):

the code is recompiled because the test is a unit tests which is a part of the whole crate.

if the test was in the separate crate, only the test crate itself would need to be reompiled

std::Veetaha (Jan 30 2020 at 16:57, on Zulip):

Yes, the recompilation could be avoided at all

std::Veetaha (Jan 30 2020 at 16:58, on Zulip):

Though you could argue that it takes very little time, but, to me, that is perceivable delay

std::Veetaha (Jan 30 2020 at 17:00, on Zulip):

To clarify, we don't have this problem at all now, since we never change that smoke-test (it least I don't do that). We are talking about the proposition to use such smoke-tests to extract the data of the tests that failed and need to be debugged.

std::Veetaha (Jan 30 2020 at 17:04, on Zulip):

My proposal is to change that fn parse_smoke_test() function body to the following:

#[test}
fn parse_smoke_test() {
    let code = read_text("test_data/smoke_test.rs"); // We change only `some_test.rs` and never ever do any kind of recompilation, the test data is loaded at runtime
    let expected = read_text("test_data/smoke_test.txt");
    assert_eq!(code, expected);
}
std::Veetaha (Jan 30 2020 at 17:06, on Zulip):

Anyway your main concern was about migrating most of our unit tests to data-driven tests approach...

std::Veetaha (Jan 30 2020 at 17:08, on Zulip):

I totally agree on that, we would just need to extract all inline source code strings into files on the filesystem and run a single test function that loads data from test_data directory and matches actual to expected

matklad (Jan 30 2020 at 17:11, on Zulip):

I guess we just violently agreeing that, with tests only in the integration-tests crate, changing the smoke test would only require recompilation of the tests crate itself (whcih woudl contain only two tests) and relinking, which is presumably much faster than today's situation of rectompilation of the whole crate under test.

matklad (Jan 30 2020 at 17:12, on Zulip):

Another concern I have is that the current marks infrastructure does use #[cfg(test)]. But I imagine we can just always unconditionally enabled test marks

std::Veetaha (Jan 30 2020 at 17:14, on Zulip):

@matklad, please correct me if I am wrong: when we change the test in a crate which a bunch of other crates depend on, we have to recompile the dependent crates too?

matklad (Jan 30 2020 at 17:17, on Zulip):

Yes, unless the tests are in the non-inline module #[cfg(test)] mod tests;. In that case, the file with tests does not get into the depinfo, and so Cargo turns out to be smart enough to not recompile

std::Veetaha (Jan 30 2020 at 17:19, on Zulip):

You know, we could pass some cli parameters (or via ENV varianbles, config files etc...) the specific test we would want to run to debug it

std::Veetaha (Jan 30 2020 at 17:20, on Zulip):

This is the approach to avoid copy-pasting to smoke-test

Jeremy Kolb (Jan 30 2020 at 17:27, on Zulip):

Currently, each test is a separate #[test] function, and that is done primarily for the benefit of the debugging.

This is incredibly convenient.

std::Veetaha (Jan 30 2020 at 17:30, on Zulip):

From my point of view, the current approach we take in ra_syntax with the test_data directory is viable.
We have a "non-inline" mod tests; changes to which don't cause the recompilation of the crate.
I'd propose to have a separate #[test] fn run_singe_test(test_name: &str) which would accept the test name and run it for the debugging purposes.
This setup can be applied to all crates where data-driven tests are possible

std::Veetaha (Jan 31 2020 at 17:03, on Zulip):

@matklad , @Jeremy Kolb, I've took a very brief look at the testing infrastructure of TypeScript compiler. They do something that I've proposed. They provide a way to debug a single test by passing the test name parameter to the cli. I am not sure which alternative for that parameter we could use for rust-analyzer, this needs an investigation. By the way, I see that they don't number their tests that much, they just ask people to come up with unique filenames. This looks like a mess since they have 4K+ tests at it was just a single folder of them, but I think they have a reason for not bothering with numbering tests anyway...

// Quote from TypeScript CONTRIBUTING.md
 Note that filenames here must be distinct from all other compiler testcase names, so you may have to work a bit to find a unique name if it's something common.
Jeremy Kolb (Jan 31 2020 at 17:11, on Zulip):

Cool I'll check it out later. Re numbering: it doesn't matter much to me as long as it's easy to run a group of related tests at once. For instance I like being able to run only tests related to "go to definition" which works today.

Jeremy Kolb (Jan 31 2020 at 17:16, on Zulip):

If we can move tests out which reduces the compilation cycle I am all for that provided we do not sacrifice usability and almost as important: discoverability. However it's sliced if I can't find the test I'm looking for (or where to place new tests) it's gonna be a bad time.

std::Veetaha (Jan 31 2020 at 17:18, on Zulip):

Also, I wish we could assign tags to files and not use folders of the filesystem to group tests by one characteristic, this way we could try to add an ability to run tests filtered by specific tags, but this might seem like a baby dream

Jeremy Kolb (Jan 31 2020 at 17:19, on Zulip):

I've noticed that flutter uses external tests and if you follow the main repo you'll notice that every 3rd commit is "revert because it breaks a test".

Jeremy Kolb (Jan 31 2020 at 17:20, on Zulip):

cargo test allows you to do it by prefix right? That may be as close as you can get without jamming more things into xtask which would be nice to avoid.

std::Veetaha (Jan 31 2020 at 17:22, on Zulip):

Yeah, that insert test script seems like fixing the symptoms

Laurențiu Nicola (Jan 31 2020 at 18:04, on Zulip):

cargo test allows you to do it by prefix right? That may be as close as you can get without jamming more things into xtask which would be nice to avoid.

substring, actually

std::Veetaha (Jan 31 2020 at 19:46, on Zulip):

Also, if you are curious, you can see one more approach that deno takes (FYI: deno is a new server-side JS runtime written in Rust). They have a very big integration_test.rs file where they just put #[test] functions per each data-driven test file. So each time they add test_smth.js and test_smth.js.outfile they add a #[test] function with a macro to be able to debug this one specific test if needed.
See this here:

macro_rules! itest(
  ($name:ident {$( $key:ident: $value:expr,)*})  => {
    #[test]
    fn $name() {
      (util::CheckOutputIntegrationTest {
        $(
          $key: $value,
         )*
        .. Default::default()
      }).run()
    }
  }
);

itest!(_001_hello {
  args: "run --reload 001_hello.js",
  output: "001_hello.js.out",
});

itest!(_002_hello {
  args: "run --reload 002_hello.ts",
  output: "002_hello.ts.out",
});
Jeremy Kolb (Jan 31 2020 at 19:56, on Zulip):

Makes sense. I kind of like the macro approach

std::Veetaha (Jan 31 2020 at 20:09, on Zulip):

Yes, the macro approach seems like an option, though I'd like to preserve that Debug test button to be able to run the test in VSCode debugger by just pressing it, but that would be simple if we tweak the macro a bit anyway...
pasted image

std::Veetaha (Jan 31 2020 at 20:20, on Zulip):

By, the way we could actually use codegen to create these #[test] function from files provided in test_data dir automatically.

std::Veetaha (Jan 31 2020 at 20:22, on Zulip):

But to me, having test somehow parameterized by the test data file name seems to be the best option

std::Veetaha (Jan 31 2020 at 20:30, on Zulip):

environment variable like RA_TEST_PATH="ra_syntax/test_data/lexer/ok/test_foo.rs" is the first thing that comes into my mind.

#[test]
fn lexer_tests() {
    // runs test on all test data files as it currently does
}
#[test]
fn run_single_test() {
    let test_path = std::env::var("RA_TEST_PATH");
    // Runs test at path defined by the env variable
}
Jeremy Kolb (Feb 01 2020 at 16:57, on Zulip):

I dislike having to set environment variables. It's not easily discoverable and IDEs can make that interaction difficult.

std::Veetaha (Feb 05 2020 at 23:37, on Zulip):

To revive this thread, my colleague advised me this shared compilation cache tool by Mozzilla to reduce compile times.

Laurențiu Nicola (Feb 06 2020 at 06:04, on Zulip):

It works nicely, but you need to clear its cache after toolchain updates or you sometimes end up with some strange errors. And you must remember to disable it when measuring the build times.

Laurențiu Nicola (Feb 06 2020 at 06:07, on Zulip):

If you're on non-Mac and want to speed up your builds, you can also switch to lld by putting -Clinker=lld in your RUSTFLAGS or ~/.cargo/config

Last update: Feb 25 2020 at 04:05UTC