Add test-util features to hide test utilities

[tgeoghegan]

We had a handful of items intended for use exclusively in tests that
were marked pub so that targets like monolithic_integration_test can
use them, but tagged #[doc(hidden)] because they aren't part of the
proper public interface to any component of Janus. We now instead hide
those test utilities behind a new test-util feature defined on crates
janus_core, janus_client and janus_server. We could also have
moved items to the test_util crate but there's something to be said
for having e.g. the code that inserts tasks into the datastore be right
next to all the other datastore code, instead of test_util/lib.rs
becoming a dumping ground of items unrelated to each other save that
they are used in tests. Additionally, if we ever want to "promote" any
of these test_util items to being used in production configurations,
that's a simple matter of deleting a #[cfg(feature = "test-util")]
instead of having to move a function and annihilate its git history.

Resolves #141

[branlwyd]

This is a great idea, I was starting to implement this on the task-provisioner PR as well. :)

Another upside of putting the functionality in a test_util module rather than a separate crate is that the module can access non-public items from the module it is embedded in.

[branlwyd]

N.B. I think we should go further, e.g. I think we can replace all of the janus_test_util crate with feature-controlled test_util modules, but this PR is fine as a starting point. The rest can be done incrementally.

[tgeoghegan]

N.B. I think we should go further, e.g. I think we can replace all of the janus_test_util crate with feature-controlled test_util modules, but this PR is fine as a starting point. The rest can be done incrementally.

Well, this seems like an opportune time to take that one, so I'll take a stab at this and see what it looks like.

[branlwyd]

OK -- I think the trickiest bit will be moving the ephemeral_datastore & related functionality to live in a single place -- I suspect there may be some circular-dependency issues there that may be tricky to resolve.

I think everything else should be relatively straightforward?

[tgeoghegan]

There's something here that worries me and that I don't understand. With the current commit (4463aed), if I run cargo --verbose -p janus_server build, I get:

Fresh unicode-xid v0.2.2
       Fresh version_check v0.9.4
<snip many dependencies>
   Compiling janus_core v0.1.2 (/home/timg/source/janus/janus_core)
     Running `rustc --crate-name janus_core --edition=2021 janus_core/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debuginfo=2 --cfg 'feature="bytes"' --cfg 'feature="database"' --cfg 'feature="postgres-protocol"' --cfg 'feature="postgres-types"' --cfg 'feature="test-util"' <snip many flags>`
   Compiling janus_server v0.1.0 (/home/timg/source/janus/janus_server)
     Running `rustc --crate-name janus_server --edition=2021 janus_server/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debuginfo=2 --cfg 'feature="test-util"' <snip many flags>`
<snip more janus targets>
    Finished dev [unoptimized + debuginfo] target(s) in 15.99s

I don't understand why we have --cfg 'feature="test-util"' when building janus targets. Since we're not building tests, the dev-dependencies shouldn't kick in, and thus nothing should enable feature test-util.

[tgeoghegan]

I'm starting to think this test-util thing was an existing problem. Here's a log from building main earlier today (i.e., without, this change): https://github.com/divviup/janus/runs/7220966325?check_suite_focus=true#step:7:295

Running `rustc --crate-name tokio --edition=2018 /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.19.2/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debuginfo=2 --cfg 'feature="bytes"' --cfg 'feature="default"' --cfg 'feature="fs"' --cfg 'feature="full"' --cfg 'feature="io-std"' --cfg 'feature="io-util"' --cfg 'feature="libc"' --cfg 'feature="macros"' --cfg 'feature="memchr"' --cfg 'feature="mio"' --cfg 'feature="net"' --cfg 'feature="num_cpus"' --cfg 'feature="once_cell"' --cfg 'feature="parking_lot"' --cfg 'feature="process"' --cfg 'feature="rt"' --cfg 'feature="rt-multi-thread"' --cfg 'feature="signal"' --cfg 'feature="signal-hook-registry"' --cfg 'feature="socket2"' --cfg 'feature="sync"' --cfg 'feature="test-util"' --cfg 'feature="time"' --cfg 'feature="tokio-macros"' --cfg 'feature="tracing"' --cfg 'feature="winapi"'  <snip>

note the rustc line includes --cfg 'feature="test-util"'. Why? We only enable that feature on tokio in janus_server/Cargo.toml's dev-dependencies, and this is cargo build?

[tgeoghegan]

Ah, it seems we need resolver = "2" in the top-level Cargo.toml to avoid having features unified across dependencies and dev-dependencies: rust-lang/cargo#4866

[branlwyd]

Note that the text from your second message is from compiling the tokio crate, which had a preexisting test-util feature that we enable -- in dev-dependencies only. [1]

I think this is OK, though; the behavior I saw when playing with the test-util feature is that it will be enabled for non---release builds. Do you see the same behavior for --release builds?

edit: never mind, doing cargo build --release --verbose still shows the test-util feature as being enabled! I think you're correct that we need to go to resolver = "2". The test I was doing was checking if optional dependencies were being pulled in based on the feature being enabled -- it appeared they weren't, which leaves me confused as to how compilation succeeded if the test-util feature was enabled.

[1]

janus/janus_server/Cargo.toml

Line 72 in 40a176d

tokio = { version = "*", features = ["test-util"] }

[tgeoghegan]

All right, everything seems to be working now that we have resolver = "2" in the top-level Cargo.toml. I removed crate test_util and instead made it a module in janus_core that's hidden behind feature test-util. I then also moved MockClock from the root of the test_util module and into janus_core::time::test_util, a new module also hidden behind test-util. I chose not to move the other items in janus_core::test_util (the ephemeral datastore macro, the fake VDAF and a test trace subscriber) because there isn't an obvious place to move them to like there was for MockClock, and this PR is already quite large, so we'll call that good enough for now.

[branlwyd]

I think this should be dep:lazy_static & dep:testconatiners to avoid exposing implicit features lazy_static & testcontainers that consumers of the janus_server crate could enable.

From https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies:

In some cases, you may not want to expose a feature that has the same name as the optional dependency. For example, perhaps the optional dependency is an internal detail, or you want to group multiple optional dependencies together, or you just want to use a better name. If you specify the optional dependency with the dep: prefix anywhere in the [features] table, that disables the implicit feature.

... but it looks like this feature is not available until Rust 1.60 & our Cargo.toml currently specifies Rust 1.58. If you're (understandably) not comfortable also upgrading our Rust version in this PR, file an issue? Would be a <5 minute cleanup once our Rust version gets bumped, and I think it would be nice to not expose features we don't expect folks to turn on.

[tgeoghegan]

I don't think there's much risk to bumping MSRV to 1.60, since the Rust toolchain installed in GH actions runner is already 1.61, and our container builds are already using rust:1.62.0-alpine.

[branlwyd]

Should the files under src/ in this directory also be removed? I think the code in those files got moved out to various test_util mods.

[tgeoghegan]

The other files were git mved into janus_core/src/test_util. In my checked out copy of branch timg/test-util-features, directory $(SRCROOT)/test_util no longer exists.