Use cargo test for plugin integration tests

[cosmicexplorer]

Fixes #128.

Problem

We currently have a "fuzzer" binary exported in the ethersync crate, which is used for testing random input with the nvim plugin. There's nothing really wrong with this, but it means we have to pull several crates into [dependencies] instead of [dev-dependencies], and it's not clear that actors.rs is a testing framework instead of somehow part of ethersync itself. @blinry advised (#127 (comment)) making bin/fuzzer.rs into an integration test.

Solution

• Create tests/ directory for integration tests that exercise each plugin.
  • Move actors.rs into tests/common/mod.rs to be usable by integration tests.
  • Move the test submodule of actors.rs into tests/vim-plugin.rs, since #[test] functions are ignored in integration test sources.
  • Rename src/bin/fuzzer.rs to tests/random-input-nvim.rs and set harness = false in Cargo.toml so we can continue to use the same main() method.
    • As a result, we can also continue to add -v to its command line to get debugging output.
  • Modify .github/workflows/general.yml to run unit vs integration tests instead of calling cargo run.
• Move dependencies only used for integration tests into [dev-dependencies].
  • Also remove three unused dependencies: time-macros, local-ip-address, and public-ip.
  • Also select fewer features when possible on some dependencies, especially tokio.
  • Also sort the list of dependencies by name (this is done in a separate commit to make it easier to audit).
• Make clap into an optional dependency, and enable it under a new "executable-deps" feature (which defaults to on).
  • Modify .github/workflows/general.yml to use --no-default-features to avoid pulling in clap.

Result

• We can run our fuzzing tests as an integration test with cargo test!
  • cargo test --lib runs unit tests, and cargo test --test '*' runs integration tests.
• We avoid pulling in a few deps, and the ethersync binary is slightly smaller:

> git checkout main
> cargo build --release
> ls target/release/ethersync
-rwxr-xr-x   2 cosmicexplorer wheel  14M Aug 14 07:46 ethersync*
> git checkout -
> cargo build --release
> ls target/release/ethersync
-rwxr-xr-x   2 cosmicexplorer wheel  12M Aug 14 07:49 ethersync*

[zormit]

Thanks for trying to separate things where they belong. This has been slightly bothering me for a while, I think your PR is going in the right direction.

Unfortunately it currently doesn't fully work: The dev build via cargo build fails with a couple of errors. I have stopped looking at the code, so there's not a lot of feedback on that, but I also assume that you have mostly just moved stuff around.

Also it would be desirable that cargo test only runs the (fast) unit tests, I usually like to run them via cargo watch -x test or something. With you changes the integration tests also run by default. Do you know of a way to make that optional? Maybe through a bench or something?

Also, and here I'm not sure what's the Rust way, I think I would intuitively run unit tests with a dev build, while the fuzzer needs to run with an optimized (e.g. release) build, otherwise it times out. Just running cargo test will result in a timed out fuzzer for me.

Maybe, to break old habits, we also need to document to ourselves how we're expecting which tests to be run as it might change with this PR.

[zormit]

This seems to have been added above already? (line 43)

[cosmicexplorer]

Oops yes! What happened here is that I had sorted the dependencies first before splitting them off into source and dev deps, and then had to revert that sorting to make it easier to audit. Will look for other cases of this, although I will probably be refactoring this again anyway to make the separate plugin-fuzz subcrate mentioned in my other comment.

[zormit]

Maybe the correct semantic way is to actually "Request changes" :) See comment above on the request.

[cosmicexplorer]

Also it would be desirable that cargo test only runs the (fast) unit tests, I usually like to run them via cargo watch -x test or something. With you changes the integration tests also run by default. Do you know of a way to make that optional? Maybe through a bench or something?

So the way this is addressed is by doing cargo test --lib for only unit tests, whereas cargo test --test '*' runs only integration tests (see e.g. rust-lang/cargo#8396). I agree this is actually not ideal though, especially wrt your next comment:

Also, and here I'm not sure what's the Rust way, I think I would intuitively run unit tests with a dev build, while the fuzzer needs to run with an optimized (e.g. release) build, otherwise it times out. Just running cargo test will result in a timed out fuzzer for me.

So what I had actually done first here was to create a separate subcrate for the fuzzer, the way the zip crate does (https://github.com/zip-rs/zip2/tree/master/fuzz). We use a slightly different setup there since we invoke it through cargo fuzz instead of cargo test (see #130 where I wondered about doing the same here). cargo fuzz is a pretty specific harness though and wouldn't work immediately on our fuzz testing, although I believe it can be made to work (just need to get more familiar with it).

Setting all of that aside though, one easy way to solve this might be:

• Make a new subcrate plugin-fuzz for the fuzzer and actors.rs, so we can execute tests with cargo test like we're used to and not have to worry about triggering other tests that were kept separate for a reason.
  • This also achieves the separation of dependencies, although that's not really an issue we need to worry too much about at the moment.
  • In general, please do continue to push back if you feel like something is harshing your workflow vibes, cargo is actually not great at UX and workarounds are often necessary.
• In plugin-fuzz, set [profile.test] to use opt-level = 3 (this will allow us to avoid explicitly setting --release in our CI).

[cosmicexplorer]

Maybe, to break old habits, we also need to document to ourselves how we're expecting which tests to be run as it might change with this PR.

I think the "habit" of running cargo test and only running unit tests is perfectly reasonable and I don't think we need to give it up! I don't want to change that experience unless there's a very very good reason. But sure, documenting which tests are run is a good idea, and part of my goal with this change was definitely to document more clearly that actors.rs and the fuzzer are for testing, not running as part of ethersync itself.

[blinry]

I have a couple of suggestions related to renaming things! :D But overall, I'm also thankful that you're working on this separation of concerns! Not mixing the test code with the Ethersync code is wonderful.

[blinry]

I rebased this PR, and added a tiny README that explains how to run the tests!

Thanks for working on this! 🎉 I noticed that you even "minified" the Tokio features in both crates, that's pretty neat! (Any idea why the build didn't fail for me locally before afc77f7?)

[zormit]

if @blinry likes it, i like it! :-P (didn't take a closer look, but don't want to be the blocker)